Rework purity flag setting for constant variables accessed via member access. by rodiazet · Pull Request #16376 · argotorg/solidity

rodiazet · 2025-12-23T15:51:00Z

In this PR:

TypeChecker rework for MemberAccess implementation
- Move isPure setting for VariableDeclaration of constant variable to the context of accessing it via TypeType, not via this.
- Rework TypeType case for ContractType for to properly handle events, errors, structs and enums when accessing via contract name.
- The same as above but in context of module.
- Enable removed previously sanity check which did not work
Rework nativeMembers implementation and typeViaContractName to return function type with call kind properly set for any context.
Add and update tests for member access.

~~Depends on #16448~~
Depends on: #16626

cameel · 2026-01-16T19:26:26Z

+			auto const* typeTypeMember = dynamic_cast<TypeType const*>(annotation.type);
+			typeTypeMember &&
+			(
+				typeTypeMember->actualType()->category() == Type::Category::Struct ||
+				typeTypeMember->actualType()->category() == Type::Category::Enum ||
+				typeTypeMember->actualType()->category() == Type::Category::Contract
+			)


I would add UserDefinedValueType as well.

And if you add that I think this covers pretty much every category that is possible inside a module via language syntax. So I'd simply make it always true for TypeType and just keep the condition as an assert to make sure we're correct about this assumption.

Same thing for the contract case. UserDefinedValueType and definitions are allowed inside contracts. Contract is not, but if it was, I should be marked pure as well, so I'd include it.

This makes the code the same as for modules. And this sounds about right - I don't think this logic is in any way specific to modules. TypeType should generally always be constant. This is what we already do when type-checking Identifier:

solidity/libsolidity/analysis/TypeChecker.cpp

Lines 3701 to 3702 in 2673c4b

else if (dynamic_cast<TypeType const*>(annotation.type))

annotation.isPure = true;

cameel · 2026-01-16T19:43:59Z

+		if (auto const* functionTypeMember = dynamic_cast<FunctionType const*>(annotation.type);
+			functionTypeMember &&
+			(
+				functionTypeMember->isPure() ||
+				functionTypeMember->kind() == FunctionType::Kind::Internal ||
+				functionTypeMember->kind() == FunctionType::Kind::Event
+			)


I think this is missing Error. Same for the contract case.

And, again, with that this covers all possible function types you can have as members on a module, so you can just assert and reduce the condition to if (functionTypeMember).

Well, technically you could try to tack an External function onto a module this way:

import "m.sol" as M; library L { function f(uint x) external {} } contract C { using L for *; function test() public { M.f; // Error: Member "f" not found or not visible after argument-dependent lookup in module "m.sol". } }

using for attaches the function to all types, even those unnameable, so you can use this to attach functions even to things which cannot otherwise be the type of x, e.g. to literals. And we cannot filter out non-matching types simply by checking if the type is the same, because we have to take into account implicit conversions so there is a chance that you could attach your function to weird things this way, even if you won't be able to call it.

Not sure if we're doing more complex filtering here or if it's just hardcoded to only accept some specific types, but either way this fortunately does not compile, so we do not have to worry about external functions after all.

Error and Declaration are cover by isPure.

Oh. Maybe we should add events there as well? Both errors and events by themselves should be pure (and even theoretically usable to initialize constants, though there's nothing you can do with them currently). If we introduce proper error/event objects, those should not require relaxing purity unless you actually send them to the outside in some way. It's only specifically emit and revert that should have higher requirements.

cameel · 2026-01-16T20:06:39Z

+			// See `ContractType::nativeMembers` for details.
+			solAssert(annotation.referencedDeclaration);
 			annotation.isLValue = annotation.referencedDeclaration->isLValue();
+			// Expressions like `C.foo`, `C.Ev` are pure and they must generate `Statement has no effect.` warning.


What about function pointers?

contract C { function() internal returns (uint) fi; function() internal returns (uint) gi; function() external returns (uint) fe; function() external returns (uint) ge; function test() public { C.fi = C.gi; C.fe = C.ge; } }

And generally it looks like you can refer to member variables via contract name, which means that you can have non-pure things as contract type members:

contract C { uint x; uint y; function test() public { C.x = C.y; } }

The comment was a little unclear. I was thinking of C.foo;, C.Ev; statements. The same should apply to C.fi;, C.gi; ,C.fe;, C.ge;, C.x; and C.y;, the warning should be generated. Problem is that it's generated based on isPure flag, but this flag is also used to mark that an expression can be used to initialize a constant (compile-time) variable.
If we set that all the expression have isPure == true, it makes

contract C { uint y; uint constant x = C.y; }

compiles. Which is obviously wrong.

I have decided to leave internal call kind function as not pure. So C.foo is never pure. In the #8263 the pure flag was set to true for external call kind functions accessed via contract name. Should we revert this or it's ok? @cameel
For the function pointers we mark them pure only if the pointer variable is constant. (Similar to other variables.) For now it never happens because there is no way to initialize constant function pointer (#16339).

I have decided to leave internal call kind function as not pure.

I'm fine with not touching them in this PR, but I'd also be fine with giving them isPure = true here. That's the end state I'd like to get eventually anyway (#16422).

I had doubts about that when I reported #16339, but that was because I was looking at constant as something that should be become comptime. Now I think we should keep it runtime constant and having inline functions be runtime constant as well would be perfectly in line with that.

In the #8263 the pure flag was set to true for external call kind functions accessed via contract name. Should we revert this or it's ok?

Seems fine to me. They're a weird construct, since they're not associated with an address, but I can't see any reason why we would not treat them as runtime constant.

cameel · 2026-01-16T21:46:01Z

+			// In case `Base.value` or `Lib.value` and when `value` is constant, the expression is pure.
+			else if (
+				auto const* varDeclMember = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration);
+				varDeclMember &&
+				varDeclMember->isConstant()
 			)
-				annotation.isPure = *_memberAccess.expression().annotation().isPure;
+				annotation.isPure = true;


Currently you're checking isConstant() only on contract types and modules, assuming that the only other possible case is access via a contract variable (which means it must be a getter function). This is probably true, but we should guard it with an assert which will fail if we missed any other case.

Or it might be simpler to just keep the check as it was and only specifically exclude the contract variable case.

I prefer to add separated asset.

BTW this can be also a variable declaration in a struct, accessed via a member access.

cameel · 2026-01-16T22:14:56Z

 	/// @returns the type for members of the containing contract type that refer to this declaration.
 	/// This can only be called once types of variable declarations have already been resolved.
-	virtual Type const* typeViaContractName() const { return type(); }
+	virtual Type const* typeViaContractName(bool) const { return type(); }


The new argument should be documented.

Also, what exactly does it mean? inDerivingScope sounds a little ambiguous, maybe the name should be adjusted? Is it saying whether we're still within the scope that contains the definition or something? EDIT: After reading further I see that's a preexisting name and literally refers to the derived contract. How about calling the parameter inherited?

I would also consider making it an enum. These boolean args we already have are not very readable at the point of call and I usually suggest adding a comment that mentions the parameter name when calling the function anyway. An enum would make that unnecessary.

Reworked. I separated additional function typeViaForeignContractName and removed the flag.

cameel · 2026-01-16T22:34:04Z

+Type const* FunctionDefinition::typeViaContractName(bool inDerivingScope) const
 {
+	solAssert(
+		libraryFunction() || visibility() != Visibility::Private,
+		"Private function members are not available via contract type name."
+	);


Maybe also assert that if inDerivedScope, the function cannot be a library function? That combination would not make sense.

And isVisibleInDerivedContracts() must also be true for that function. On the other hand when not in derived scope, isVisibleInContract() should still be true.

Also, when libraryFunction(), assert that isVisibleAsLibraryMember() is true. This will not let you call this with private library functions, but those are not callable as Lib.foo(), only foo() anyway.

And that isImplemented() is true for library functions.

There are cases when accessing private library members is allowed. I.e. in using statement.

We also cannot isImplemented at this stage as it's checked later in TypeChecker::visit(FunctionDefinition const& _function). Test triggering this error syntaxTests/nameAndTypeResolution/229_call_to_library_function

cameel · 2026-01-16T22:57:03Z

+				{
+					// In case of regular contract being accessed from by external contract (not in deriving scope),
+					// add only externally visible members.
+					if (declaration->isVisibleViaContractTypeAccess())


We should rename this function to something like isVisibleViaContractInstance(). I thought it was referring to access like C.foo() until I looked at the definition.

cameel · 2026-01-16T23:02:44Z

+					// In case of regular contract being accessed from by external contract (not in deriving scope),
+					// add only externally visible members.
+					if (declaration->isVisibleViaContractTypeAccess())
+						members.emplace_back(declaration, declaration->typeViaContractName(inDerivingScope));


All three branches here now call typeViaContractName(), so you could simplify it all into a single condition.

Not any more. :)

cameel · 2026-01-16T23:06:20Z

+					// In case of regular contract (not library) and member is in the same deriving scope, add all
+					// members which are not private. Private members cannot be accessed via contract type name
+					// i.e C.fooPrivate.
+					if (declaration->visibility() > Visibility::Private)


I think this will add external functions while previously isVisibleInDerivedContracts() would filter them out.

Yes. And the Declaration kind was set below. Now it's handled by the typeViaContractName(ContractNameAccessKind _accessKind)

cameel

Here's another batch of comments.

Though my general feeling is that it would be good to extract some things into a refactor PR to make it easier to see what's actually changing and what's existing behavior (see my comment below).

cameel · 2026-02-02T08:53:26Z

+		} else
+		{
+			solAssert(
+				exprType->category() == Type::Category::Module ||
+				exprType->category() == Type::Category::Struct ||
+				exprType->category() == Type::Category::Contract,
+				"Variable member is only available for module, contract or struct");
+		}


Braces are unnecessary here (and IMO just make it less readable).

The message could also be more precise. On the other hand it's superfluous to have it in both asserts as well as in a comment. I'd leave it just in the comment.

Suggested change

} else

{

solAssert(

exprType->category() == Type::Category::Module ||

exprType->category() == Type::Category::Struct ||

exprType->category() == Type::Category::Contract,

"Variable member is only available for module, contract or struct");

}

else

solAssert(

exprType->category() == Type::Category::Module ||

exprType->category() == Type::Category::Struct ||

exprType->category() == Type::Category::Contract,

"Only module, struct and contract instances as well as contract types can have accessible variables."

);

Overall, assert messages are optional and I'd omit them for brevity in cases where the condition is obvious or already explained. And if it's not I'd usually rather try to make it clearer by introducing some intermediate variables with good names (though TBH naming in this part of code is pretty bad; e.g. exprType tells you nothing out of context, you have to find the declaration to figure out which particular expression it represents).

cameel · 2026-02-02T09:40:55Z

+			);
+		}
+		else if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))
+			annotation.isPure = *_memberAccess.expression().annotation().isPure && varDecl->isConstant();


This looks wrong. Did you mean ||? The way it is now, only constants accessed via a constant contract instance will be marked constant.

This means this will not compile, because c.x won't be considered constant, even though the thing you're accessing is really a getter, not the constant itself:

contract C { uint public x; } contract D { C constant c; function() external returns (uint) constant fPtr = c.x; }

Also, currently the only things I can think of that would fall into this branch are getters. I'd add an assert that annotation.type is FunctionType to verify that.

Actually, we'd now have it in both branches so we can pull it out of the if. It sounds about right. The only things you can access via a contract instance are its external functions (explicitly declared or getters) or library functions attached with using for.

cameel · 2026-02-02T10:09:46Z

+	{
 		annotation.isLValue = !structType->dataStoredIn(DataLocation::CallData);
+
+		if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))


This being called varDecl (sometimes even vDecl) is confusing me to no end every time I read this code, especially now that you're using it a lot more than it was before. With these names being this vague it's hard to keep track of what refers to the member, what to the underlying expression and what to the member access expression as a whole.

It's a bit awkward, because we don't have clear terms to distinguish these things, but maybe declarationOfAccessedVariable would be good enough for this one. And not just here but in all cases inside in this function.

Suggested change

if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))

if (auto const* memberAccessVariableDeclaration = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))

Similarly, I'd rename funcType/functionTypeMember to accessedFunctionType.

And exprType would have been much better off as underlyingExpressionType or something, though I'd probably not change it here because it's used a lot in the part of the function you're not touching. Doing that would introduce too much noise to the diff here and make reviewing it harder. So just FYI.

Actually, can this condition even fail? Is it possible to have a struct-type expression without an associated struct definition? Shouldn't this be an assert?

It's not about the struct, but the member of a structure. It can be also a function, which btw it not cover here. The function can be attached with using statement and a library. Have to add this too.

cameel · 2026-02-02T10:18:34Z

+		if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))
+		{
+			annotation.isPure = varDecl->isConstant();
+			solAssert(!varDecl->isConstant(), "Struct member variables cannot be declared as constant.");


I'd use solUnimplemented here to make it clear that it's not something that should not happen, but just something that we have not enabled yet.

Suggested change

solAssert(!varDecl->isConstant(), "Struct member variables cannot be declared as constant.");

solUnimplementedAssert(!varDecl->isConstant(), "Constant structs are not yet implemented.");

Also, why set isPure based on isConstant() in the first place? _memberAccess.expression().annotation().isPure seems more appropriate and future-proof to me. If we happen to have a constant struct instance, its members should also be constant. It would not require making assumptions about why it's constant (whether it's because isConstant() or due to some other mechanism we might introduce in the future).

cameel · 2026-02-02T10:35:43Z

We already have else if (TypeType ...) above. Perhaps we should merge them so that it's easier to follow?

In the same vein, the FunctionType and MagicType cases below look like they should really be just cases of that big if above. In fact, it should probably be just a switch on annotation.type->category().

Actually, what would you say to do a quick refactor of this in a separate PR to clean it up first and then rebase this PR on it? One problem I have with these conditions being restructured is that it will be hard to accurately tell which things visible to the user actually changed. We'll need that for changelog and docs. So it would be best if we could leave only actual behavioral changes here.

I'd also put the newly added tests in it. Then here we'll be able to easily see how the PR affects them.

Good point. I was also thinking of it, because this PR is already getting too big.

cameel · 2026-02-02T10:54:21Z

+		else if (annotation.type->category() == ModuleType::Category::Module)
+			annotation.isPure = true;
+		else if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))
+			annotation.isPure = varDecl->isConstant();


Here I'd also initialize it from isPure of the referenced declaration. We could keep isConstant() as a sanity check, though.

VariableDeclaration does not have isPure field, also in its annotation. Previously it was taken from the parent expression, not from the member, which does not make sense in case when the parent is a module.

cameel · 2026-02-02T11:01:40Z

The PR is missing a changelog entry. With how much it's changing, we'll probably need more than one.

- Foreign function declaration access via contract name are pure. - Functions declaration from derived scope are pure. Can be used to initialise constant function pointer. - Internal library function declaration are pure. Can be used to initialise constant function pointer. - struct, enum, UDVT declarations are pure. - Constant variable declaration in contract and library are pure. - event declaration is pure

…re than one value.

rodiazet · 2026-04-20T15:32:02Z

This PR is going to be splitted into a couple of smaller PRs.

github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jan 7, 2026

rodiazet force-pushed the fix-sanity-check branch from 43069b6 to ac1500e Compare January 7, 2026 16:06

github-actions Bot removed the stale The issue/PR was marked as stale because it has been open for too long. label Jan 8, 2026

rodiazet force-pushed the fix-sanity-check branch 8 times, most recently from 5a4a8e4 to 154b9bc Compare January 14, 2026 15:56

argotorg deleted a comment from github-actions Bot Jan 14, 2026

rodiazet marked this pull request as ready for review January 15, 2026 14:55

rodiazet force-pushed the fix-sanity-check branch from 154b9bc to 157e1bb Compare January 15, 2026 14:56

rodiazet requested a review from cameel January 15, 2026 15:24

cameel added this to the 0.8.34 milestone Jan 16, 2026

cameel reviewed Jan 16, 2026

View reviewed changes

rodiazet mentioned this pull request Jan 20, 2026

Function references are not considered constant #16339

Open

rodiazet force-pushed the fix-sanity-check branch 2 times, most recently from 20bf12c to 800369b Compare January 21, 2026 14:02

rodiazet requested a review from cameel January 21, 2026 14:53

argotorg deleted a comment from stackenbotten3000 Feb 2, 2026

cameel reviewed Feb 2, 2026

View reviewed changes

rodiazet force-pushed the fix-sanity-check branch from 800369b to 4408110 Compare February 5, 2026 16:32

rodiazet mentioned this pull request Feb 6, 2026

Refactor TypeChecker visitor for MemberAccess expression. #16448

Merged

cameel requested review from clonker and matheusaaguiar February 11, 2026 16:00

rodiazet added the has dependencies The PR depends on other PRs that must be merged first label Feb 25, 2026

rodiazet force-pushed the type-checker-member-access-refactoring branch from 6d58f1a to f18bf0f Compare April 3, 2026 16:48

cameel force-pushed the type-checker-member-access-refactoring branch from f4a0090 to dc9f632 Compare April 14, 2026 14:41

rodiazet force-pushed the type-checker-member-access-refactoring branch from dc9f632 to 23050cd Compare April 16, 2026 14:02

rodiazet force-pushed the fix-sanity-check branch from c4a4019 to 553d804 Compare April 17, 2026 12:53

cameel modified the milestones: 0.8.34, 0.8.35 Apr 18, 2026

rodiazet force-pushed the type-checker-member-access-refactoring branch from 23050cd to 89b3d26 Compare April 20, 2026 12:23

cameel force-pushed the type-checker-member-access-refactoring branch from 89b3d26 to d97b44c Compare April 20, 2026 12:46

rodiazet force-pushed the type-checker-member-access-refactoring branch from d97b44c to e903693 Compare April 20, 2026 13:32

Base automatically changed from type-checker-member-access-refactoring to develop April 20, 2026 14:51

rodiazet force-pushed the fix-sanity-check branch from 553d804 to f378a9c Compare April 20, 2026 14:57

rodiazet removed the has dependencies The PR depends on other PRs that must be merged first label Apr 20, 2026

rodiazet mentioned this pull request Apr 20, 2026

Purity flag: Rework native members #16625

Open

rodiazet changed the base branch from develop to rework-native-members April 20, 2026 15:09

rodiazet added has dependencies The PR depends on other PRs that must be merged first labels Apr 20, 2026

rodiazet force-pushed the fix-sanity-check branch from f378a9c to d5d961b Compare April 20, 2026 15:10

rodiazet added 9 commits April 20, 2026 17:23

Member access via contract instance.

c6f24a5

Member access via module type.

7d52e33

Add accessible variables sanity check.

6f2ea1b

Add error arguments purity tests.

c0f17f7

Add constant access getter test which reproduces the bug.

39b8826

Add purity tests of different types and usage.

b2dfa8b

Fix constantValueFunction to properly support function returning mo…

0f34897

…re than one value.

Add semantic tests.

7b0984f

rodiazet force-pushed the fix-sanity-check branch from d5d961b to 7b0984f Compare April 20, 2026 15:24

rodiazet changed the base branch from rework-native-members to purity-flag-struct April 20, 2026 15:24

rodiazet mentioned this pull request Apr 20, 2026

Purity flag: Fix purity flag for struct instance member access. #16626

Open

	else if (dynamic_cast<TypeType const*>(annotation.type))
	annotation.isPure = true;

	if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))
	if (auto const* memberAccessVariableDeclaration = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))

	solAssert(!varDecl->isConstant(), "Struct member variables cannot be declared as constant.");
	solUnimplementedAssert(!varDecl->isConstant(), "Constant structs are not yet implemented.");

Conversation

rodiazet commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet commented Dec 23, 2025 •

edited

Loading

rodiazet Jan 19, 2026 •

edited

Loading

rodiazet Jan 19, 2026 •

edited

Loading

cameel Feb 2, 2026 •

edited

Loading

cameel Jan 16, 2026 •

edited

Loading

rodiazet Jan 21, 2026 •

edited

Loading

rodiazet Jan 21, 2026 •

edited

Loading

rodiazet Feb 5, 2026 •

edited

Loading

cameel Feb 2, 2026 •

edited

Loading