Refactor `TypeChecker` visitor for `MemberAccess` expression. by rodiazet · Pull Request #16448 · argotorg/solidity

rodiazet · 2026-02-06T14:13:53Z

This PR refactors implementation of the TypeChecker visitor for MemberAccess expression. It is needed based on the outcome from the discussion on changes in #16376 (comment).

Intentionally no logic has changed.

stackenbotten3000 · 2026-02-06T14:14:19Z

There was an error when running chk_coding_style for commit 63d0c0750dbcb9c4c8982bc4931defae244044ef:

Coding style error:
libsolidity/analysis/TypeChecker.cpp:26:#include "range/v3/view/any_view.hpp"

Please check that your changes are working as intended.

Copilot

Pull request overview

This PR refactors the TypeChecker::visit(MemberAccess) implementation by extracting three helper methods and reorganizing the code into a more structured switch-case pattern. The changes aim to improve code maintainability and readability without altering the underlying logic.

Changes:

Extracted three helper methods: performOverloadedResolution, handleUnresolvedMemberAccessErrors, and validateAccessMemberFunctionType
Reorganized the main visitor logic using exhaustive switch statements for better structure
Improved variable naming for clarity (e.g., exprType → expressionObjectType)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
libsolidity/analysis/TypeChecker.h	Adds declarations for three new helper methods with detailed documentation
libsolidity/analysis/TypeChecker.cpp	Refactors MemberAccess visitor by extracting helpers and reorganizing with switch statements; removes unused includes

However, I found several critical and moderate issues:

Critical: Null pointer dereference at line 3278 when dereferencing arguments without checking if it's set
Moderate: Four instances of unsafe reinterpret_cast usage instead of the codebase's standard dynamic_cast pattern
Nit: Confusing variable naming with double "Type" suffix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libsolidity/analysis/TypeChecker.cpp

nikola-matic · 2026-02-09T10:07:53Z

libsolidity/analysis/TypeChecker.cpp

-	size_t const initialMemberCount = possibleMembers.size();
-	if (initialMemberCount > 1 && arguments)
+	// do overload resolution
+	for (auto it = _possibleMembers.begin(); it != _possibleMembers.end();)


std::remove_if(_possibleMembers, [](auto const& member) { return ....; })?

Of course, you're not liable to change this since it was already like that, and would make the review a bit more difficult then just comparing the moved around code snippets.

I plan to make it i a separated PR/PRs.

cameel · 2026-02-17T16:02:45Z

libsolidity/analysis/TypeChecker.h

+	/// @param _location The source location where the member function is accessed.
+	/// @param _hasArguments Indicates whether the member function is accessed without arguments.
+	/// @param _isDefined Specifies if the member function is fully defined or abstract.
+	void validateAccessMemberFunctionType(


"access" -> "accessed"

Also, we usually do not refer to the analysis checks as "validations", so "check" would be a more typical verb in this context.

Restricting it to just "type" part is also unnecessary. The function looks like a good place for any checks related to that member we might have here.

Suggested change

void validateAccessMemberFunctionType(

void checkMemberFunctionAccess(

Good point, I think checkAccessedMemberFunction is even better.

cameel · 2026-02-17T16:09:36Z

libsolidity/analysis/TypeChecker.cpp

+		// Validate an accessed member function type.
+		validateAccessMemberFunctionType(


I'd leave out comments when they are this obvious. It's all literally in the function name now :)

The comments should explain why we are doing it. I would only restate what we are doing if the code is complex enough and names bad enough that it's not obvious at a glance.

For example in:

// Lookup type required to find the function declaration. Set default to `static`. VirtualLookup requiredLookup = VirtualLookup::Static;

the first part adds useful context, but the second is already obvious from the code.

cameel · 2026-02-17T16:17:11Z

libsolidity/analysis/TypeChecker.cpp

+			accessedMemberFunctionType,
+			expressionObjectType,
+			memberName,
+			_memberAccess.location(),
+			arguments && (*arguments).numArguments() > 0,
+			accessedMemberAnnotation.referencedDeclaration != nullptr


All of this information is trivially obtainable from _memberAccess. I'd pass in only that variable and let the function get other values from it on its own. It does repeat some info you already have available in locals here, but none of it requires a significant calculation. It will make it easier to understand the function in isolation.

cameel · 2026-02-17T16:33:29Z

libsolidity/analysis/TypeChecker.cpp

+		handleUnresolvedMemberAccessErrors(
+			_memberAccess,
+			expressionObjectType,
+			memberName,
+			possibleMemberCountBeforeOverloading
 		);


And here I would reduce it to these parameters:

Suggested change

handleUnresolvedMemberAccessErrors(

_memberAccess,

expressionObjectType,

memberName,

possibleMemberCountBeforeOverloading

);

handleUnresolvedMemberAccessErrors(_memberAccess, possibleMembers);

TBH I prefer not introducing locals for expressions that are already short and not expensive to compute. Finding good names for them is not always simple and you often end up with either a short one that's ambiguous/vague or one that's as long as the original expression. For example I'd much rather read type(_memberAccess.expression()) than expressionObjectType. The latter forces me to look up the definition to check what it is.

cameel · 2026-02-17T16:41:17Z

libsolidity/analysis/TypeChecker.cpp

-		auto [errorId, description] = [&]() -> std::tuple<ErrorId, std::string> {
-			std::string errorMsg = "Member \"" + memberName + "\" not found or not visible "
-				"after argument-dependent lookup in " + exprType->humanReadableName() + ".";
+	auto [errorId, description] = [&]() -> std::tuple<ErrorId, std::string> {


This is another thing that could be refactored here. Instead of this confusing pattern where you define a lambda and immediately call it, it would be much simpler to instead define a small helper lambda that gets the error code and description and runs m_errorReporter.fatalTypeError().

cameel · 2026-02-17T16:55:12Z

libsolidity/analysis/TypeChecker.cpp

+	MemberList::MemberMap possibleMembers = expressionObjectType->members(currentDefinitionScope()).membersByName(memberName);
+	size_t const possibleMemberCountBeforeOverloading = possibleMembers.size();
+	if (possibleMemberCountBeforeOverloading > 1 && arguments)
+		performOverloadedResolution(expressionObjectType, possibleMembers, *arguments);
+
+	accessedMemberAnnotation.isConstant = false;
+
+	if (possibleMembers.empty())
+		handleUnresolvedMemberAccessErrors(
+			_memberAccess,
+			expressionObjectType,
+			memberName,
+			possibleMemberCountBeforeOverloading
 		);
-	}
 	else if (possibleMembers.size() > 1)
 		m_errorReporter.fatalTypeError(
 			6675_error,
 			_memberAccess.location(),
 			"Member \"" + memberName + "\" not unique "
-			"after argument-dependent lookup in " + exprType->humanReadableName() +
+			"after argument-dependent lookup in " + expressionObjectType->humanReadableName() +
 			(memberName == "value" ? " - did you forget the \"payable\" modifier?" : ".")
 		);

-	annotation.referencedDeclaration = possibleMembers.front().declaration;
-	annotation.type = possibleMembers.front().type;
+	accessedMemberAnnotation.referencedDeclaration = possibleMembers.front().declaration;
+	accessedMemberAnnotation.type = possibleMembers.front().type;


All of this is still a part of what I'd consider overload resolution. I'd combine it with what you have in handleUnresolvedMemberAccessErrors() into a single function that returns possibleMembers.front() if resolution is successful and reports a fatal error if it's not.

performOverloadedResolution() (or maybe resolveOverloads()) would actually be a good name for this function. The function that is now has this name would be better off as something more specific, like filterOutOverloadsNotMatchingArguments().

Remove unnecessary includes

stackenbotten3000 · 2026-02-24T14:41:55Z

libsolidity/analysis/TypeChecker.cpp

-				TypePointers const& t = funType->returnParameterTypes();
+	// If no detailed error were reported, issues a general unresolved member error.
+	// TODO: Consider merging `storage` member case from the top of this function with the rest of the checks.
+	if(errorCount == m_errorReporter.errorCount())


Coding style error

Again \< does not match TAB \t on macos, but it matches TAB on GNU.

rodiazet force-pushed the type-checker-member-access-refactoring branch from cf38fdd to fe24189 Compare February 6, 2026 14:20

rodiazet mentioned this pull request Feb 6, 2026

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

Open

rodiazet force-pushed the type-checker-member-access-refactoring branch from 1131cb9 to bf9c42e Compare February 6, 2026 14:39

rodiazet changed the title ~~Refactor TypeChecker visitor od MemberAccess expression.~~ Refactor TypeChecker visitor for MemberAccess expression. Feb 6, 2026

rodiazet requested a review from Copilot February 6, 2026 14:52

Copilot started reviewing on behalf of rodiazet February 6, 2026 14:52 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

rodiazet force-pushed the type-checker-member-access-refactoring branch 3 times, most recently from cc730b5 to 60443be Compare February 9, 2026 08:31

rodiazet requested review from cameel, nikola-matic and r0qs February 9, 2026 08:38

nikola-matic reviewed Feb 9, 2026

View reviewed changes

cameel requested changes Feb 17, 2026

View reviewed changes

rodiazet added 9 commits February 24, 2026 15:41

Extract self-contained code fragments to a functions.

f88bb3b

Move the first part to switch statement

d309a3b

Move the second part to switch statement

fe36bfb

Remove unnecessary includes

Switch statements cleanup.

c85a004

Move Address case to the common switch.

b6b6a8d

Extract function type member validation to a function.

c7a0049

Better doc

e4148a0

Add doc strings and improve naming.

4a122c9

Address PR review comments.

e8bcb5c

rodiazet force-pushed the type-checker-member-access-refactoring branch from 737b9e4 to c8b5654 Compare February 24, 2026 14:41

stackenbotten3000 reviewed Feb 24, 2026

View reviewed changes

rodiazet mentioned this pull request Feb 24, 2026

Make the check_style.sh script great on macos as well. #16487

Closed

rodiazet force-pushed the type-checker-member-access-refactoring branch from c8b5654 to bcb45e9 Compare February 24, 2026 15:16

Address PR review comments.

43465c5

Address PR review comments.

70b945e

rodiazet force-pushed the type-checker-member-access-refactoring branch from bcb45e9 to 70b945e Compare February 24, 2026 15:17

rodiazet requested review from cameel and nikola-matic February 27, 2026 12:06

	void validateAccessMemberFunctionType(
	void checkMemberFunctionAccess(

		// Validate an accessed member function type.
		validateAccessMemberFunctionType(

Conversation

rodiazet commented Feb 6, 2026

Uh oh!

stackenbotten3000 commented Feb 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

cameel Feb 17, 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!

cameel Feb 17, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cameel Feb 17, 2026 •

edited

Loading

cameel Feb 17, 2026 •

edited

Loading