Purity flag: Rework native members by rodiazet · Pull Request #16625 · argotorg/solidity

rodiazet · 2026-04-20T15:06:28Z

This is a part of bigger Rework purity flag setting for constant variables accessed via member access. which was splitted into a couple of smaller PRs.
This PR:

Improves a function name to make it consistent with what the function does.
Reworks Declaration::typeViaContractName implementation to make it easy to integrate by the following PRs.

…nce`

matheusaaguiar

LGTM

cameel

Sorry for a ton of comments, the changes I'm actually requesting are not big (mainly I think we need a separate helper for attached functions), but this reflects how annoying these checks are to review. It's a real rabbit hole to figure out why we're doing some things the way we are and whether they make sense.

cameel · 2026-04-21T10:12:42Z

+	/// @Local means access via contract name from local contract scope or deriving contract.
+	/// @Foreign means access via contract name from foreign (unrelated) contract.
+	/// @Library means access via library name.
+	enum class ContractNameAccessKind { Local, Foreign, Library };


I think enums are more readable when you put each item on its own line. It's not like you're saving any space anyway because the docstring eats it.

Suggested change

/// @Local means access via contract name from local contract scope or deriving contract.

/// @Foreign means access via contract name from foreign (unrelated) contract.

/// @Library means access via library name.

enum class ContractNameAccessKind { Local, Foreign, Library };

enum class ContractNameAccessKind {

Local, ///< Via contract name from local contract scope or deriving contract.

Foreign, ///< Via contract name from foreign (unrelated) contract.

Library, ///< Via library name.

};

Also, it's not like we stick to Doxygen all that closely, but is something like @Local even valid markup?

cameel · 2026-04-21T12:06:54Z

 			auto const* functionType = dynamic_cast<FunctionType const*>(
-				functionDefinition.libraryFunction() ? functionDefinition.typeViaContractName() : functionDefinition.type()
+				functionDefinition.libraryFunction() ?
+				functionDefinition.typeViaContractName(Declaration::ContractNameAccessKind::Library) :
+				functionDefinition.type()
 			);


It seems a bit weird to me that we have to account for library functions here since only free functions are usable as operator definitions. We should be able to just use functionDefinition.type() (or even functionDefinition.functionType(true /* _internal */)) here and just assert isFree() which excludes library and contract functions.

Turns out we can't because the error in TypeChecker::endVisit(UsingForDirective) is non-fatal and it may call operatorDefinitions() with that condition not being satisfied. This happens e.g. when you have using for inside a library and try to use it to attach an external function to an operator.

Still, why is this not the case for contracts as well? After all you can put using for in a contract too and type() has an assert against external functions. Well, because of this:

solidity/libsolidity/analysis/DeclarationTypeChecker.cpp

Lines 550 to 558 in e10eb96

if (!functionDefinition->isFree() && !(

dynamic_cast<ContractDefinition const*>(functionDefinition->scope()) &&

dynamic_cast<ContractDefinition const*>(functionDefinition->scope())->isLibrary()

))

m_errorReporter.typeError(

4167_error,

function->location(),

"Only file-level functions and library functions can be attached to a type in a \"using\" statement"

);

Basically, we have a more general check in DeclarationTypeChecker for all bound functions (not just operators) and that one specifically only allows non-free functions inside libraries. Which is an odd exception and I'm not sure why it was done - if it was for backwards-compatibility I'd expect the same for contracts. Since this is a separate analysis phase, even a non-fatal error will prevent us from reaching TypeChecker.

Eventually, we should probably just move all the checks against non-free function operators from TypeChecker::endVisit(UsingForDirective) to DeclarationTypeChecker to make this uniform and easier to follow. But that's a separate refactor and I don't want us to go too deep. What I'd suggest now is to assert that the library case is an error and skip it. I.e. wrap this in if (!functionDefinition.libraryFunction()):

solidity/libsolidity/analysis/TypeChecker.cpp

Line 4229 in e10eb96

std::set<FunctionDefinition const*, ASTNode::CompareByID> matchingDefinitions = usingForType->operatorDefinitions(

Then simplify operatorDefinitions() as I described at the beginning.

Thinking about it more, it may be fine to leave it as is if you go with my suggestion below to create the typeWhenAttached() helper. My concern is mostly that we explicitly have to make a somewhat nonsensical decision about the type here, which makes the logic more convoluted. But if that decision is hidden inside the helper and just a side-effect of it having a broader applicability, it's a different matter.

cameel · 2026-04-21T12:45:09Z

+	/// @Local means access via contract name from local contract scope or deriving contract.
+	/// @Foreign means access via contract name from foreign (unrelated) contract.
+	/// @Library means access via library name.
+	enum class ContractNameAccessKind { Local, Foreign, Library };


One thing that I find suspicious is that we are never using this function in the codegen or even storing the result in an annotation. This means that the choices it makes (especially whether to use the internal or external version) may be inconsistent with choices made in those places. And this is likely how we got #13908.

Just FYI since fixing that goes outside the scope of this refactor (would likely be a bugfix instead). I'll let it be for now.

cameel · 2026-04-21T13:11:06Z

 	return dynamic_cast<FunctionType const*>(
 		userDefinedFunction->libraryFunction() ?
-		userDefinedFunction->typeViaContractName() :
+		userDefinedFunction->typeViaContractName(Declaration::ContractNameAccessKind::Library) :
 		userDefinedFunction->type()
 	);


We should put this in a helper on FunctionDefinition. Something like typeViaUsingFor() or typeWhenAttached().

Possibly with an assert that it must be either a library or free function - may not be possible if we're sometimes calling the functions that do this during analysis in presence of non-fatal errors.

It's not long or complicated but it encodes an important design decision - how to get the type for a function used in using {f} for T/using {f as op} for T. That decision should be in a single place with a clear label rather than spread over multiple places, risking that one of them gets it wrong (TBH I'm almost certain we got it wrong somewhere already).

cameel · 2026-04-21T13:15:41Z

+	bool isVisibleViaContractInstance() const override
 	{
 		solAssert(!isFree(), "");
 		return isOrdinary() && visibility() >= Visibility::Public;


Suggested change

return isOrdinary() && visibility() >= Visibility::Public;

return isPartOfExternalInterface();

I'm not sure we even need isVisibleViaContractTypeAccess()/isVisibleViaContractInstance(). isPartOfExternalInterface() seems clear enough. Do they diverge in some case or always match for all AST node types? If they match, I'd keep only one of them.

They do.
isPartOfExternalInterface() return default (false) value for
StructDefinition,
EnumDefinition,
UserDefinedValueTypeDefinition,
EventDefinition and
ErrorDefinition,
where isVisibleViaContractInstance() returns true.

isPartOfExternalInterface() returns true for VariableDeclaration, where isVisibleViaContractInstance() returns default false. Which is weird. (??)

They both are the same for FunctionDefinition, but isVisibleViaContractInstance() additionally assert that we it's not a free function.

So in this case I would leave both and use isPartOfExternalInterface(); in FunctionDefinition::isVisibleViaContractInstance()

cameel · 2026-04-21T13:30:26Z

+			// In case of library contract, member call kind depends on its visibility.
+			if (isPublic())
+				// When Lib.foo is public or external, an external call (delegate call) is used.
+				return FunctionType(*this).asExternallyCallableFunction(true);


Suggested change

return FunctionType(*this).asExternallyCallableFunction(true);

return FunctionType(*this).asExternallyCallableFunction(true /* _inLibrary */);

cameel · 2026-04-21T13:32:25Z

+		default:
+			solAssert(false, "Unimplemented contract member access kind.");


No need for default. The switch is exhaustive.

rodiazet added 2 commits April 20, 2026 16:59

Rename isVisibleViaContractTypeAccess -> `isVisibleViaContractInsta…

193ea90

…nce`

Rework typeViaContractName and nativeMembers fragment.

b1e82f4

rodiazet requested review from cameel, clonker and matheusaaguiar April 20, 2026 15:06

This was referenced Apr 20, 2026

Rework purity flag setting for constant variables accessed via member access. #16376

Open

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

Open

matheusaaguiar approved these changes Apr 21, 2026

View reviewed changes

cameel reviewed Apr 21, 2026

View reviewed changes

cameel added the refactor label Apr 21, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purity flag: Rework native members#16625

Purity flag: Rework native members#16625
rodiazet wants to merge 2 commits intodevelopfrom
rework-native-members

rodiazet commented Apr 20, 2026 •

edited

Loading

Uh oh!

matheusaaguiar left a comment

Uh oh!

cameel left a comment

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

rodiazet Apr 21, 2026 •

edited

Loading

Uh oh!

cameel Apr 21, 2026

Uh oh!

cameel Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (!functionDefinition->isFree() && !(
	dynamic_cast<ContractDefinition const*>(functionDefinition->scope()) &&
	dynamic_cast<ContractDefinition const*>(functionDefinition->scope())->isLibrary()
	))
	m_errorReporter.typeError(
	4167_error,
	function->location(),
	"Only file-level functions and library functions can be attached to a type in a \"using\" statement"
	);

	return isOrdinary() && visibility() >= Visibility::Public;
	return isPartOfExternalInterface();

	return FunctionType(*this).asExternallyCallableFunction(true);
	return FunctionType(this).asExternallyCallableFunction(true / _inLibrary */);

		default:
		solAssert(false, "Unimplemented contract member access kind.");

Conversation

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

Uh oh!

Uh oh!

matheusaaguiar left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Apr 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rodiazet commented Apr 20, 2026 •

edited

Loading

rodiazet Apr 21, 2026 •

edited

Loading