Refactor TypeChecker visitor for MemberAccess expression.

[rodiazet]

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]

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:

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

---

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

[Copilot]

Potential null pointer dereference: arguments is dereferenced without checking if it's set. The arguments field is of type std::optional<FuncCallArguments> and is only set when the MemberAccess is used in a function call context. When accessing a member function without calling it (e.g., array.push instead of array.push()), arguments will not be set, causing undefined behavior. This is a pre-existing issue that was carried over during refactoring. Consider guarding this with arguments && (*arguments).numArguments() == 0 or passing arguments ? (*arguments).numArguments() == 0 : true to provide a safe default.

Suggested change

	(*arguments).numArguments() == 0,
	arguments ? (*arguments).numArguments() == 0 : true,

[Copilot]

This refactoring changes the cast from dynamic_cast (used in the original code and throughout the codebase) to reinterpret_cast, which is unsafe and deviates from established codebase conventions. The original code would have used dynamic_cast<StructType const*> with a null check. While the switch statement provides some guarantee about the category, reinterpret_cast bypasses all type safety and could lead to undefined behavior if the type hierarchy or category logic changes. This contradicts the PR description stating "Intentionally no logic has changed". Please revert to using dynamic_cast to maintain consistency with the rest of the codebase (see usage at lines 2742, 4308).

Suggested change

	isAccessedMemberLValue = !reinterpret_cast<StructType const*>(expressionObjectType)->dataStoredIn(DataLocation::CallData);
	break;
	case Type::Category::Function:
	{
	if (
	auto const* expressionObjectFunctionType = reinterpret_cast<FunctionType const*>(expressionObjectType);
	{
	if (auto const* structType = dynamic_cast<StructType const*>(expressionObjectType))
	isAccessedMemberLValue = !structType->dataStoredIn(DataLocation::CallData);
	break;
	}
	case Type::Category::Function:
	{
	if (
	auto const* expressionObjectFunctionType = dynamic_cast<FunctionType const*>(expressionObjectType);
	expressionObjectFunctionType &&

[Copilot]

This refactoring changes the cast from dynamic_cast to reinterpret_cast, which is unsafe and deviates from established codebase conventions. Please revert to using dynamic_cast as in the original code to maintain type safety and consistency.

Suggested change

	isAccessedMemberLValue = !reinterpret_cast<StructType const*>(expressionObjectType)->dataStoredIn(DataLocation::CallData);
	break;
	case Type::Category::Function:
	{
	if (
	auto const* expressionObjectFunctionType = reinterpret_cast<FunctionType const*>(expressionObjectType);
	isAccessedMemberLValue = !dynamic_cast<StructType const*>(expressionObjectType)->dataStoredIn(DataLocation::CallData);
	break;
	case Type::Category::Function:
	{
	if (
	auto const* expressionObjectFunctionType = dynamic_cast<FunctionType const*>(expressionObjectType);

[Copilot]

This refactoring changes the cast from dynamic_cast to reinterpret_cast, which is unsafe and deviates from established codebase conventions. Please revert to using dynamic_cast as in the original code to maintain type safety and consistency.

Suggested change

	auto const* expressionObjectTypeType = reinterpret_cast<TypeType const*>(expressionObjectType);
	auto const* expressionObjectTypeType = dynamic_cast<TypeType const*>(expressionObjectType);

[Copilot]

This refactoring changes the cast from dynamic_cast to reinterpret_cast, which is unsafe and deviates from established codebase conventions. Please revert to using dynamic_cast as in the original code to maintain type safety and consistency.

Suggested change

	auto const* expressionObjectMagicType = reinterpret_cast<MagicType const*>(expressionObjectType);
	auto const* expressionObjectMagicType = dynamic_cast<MagicType const*>(expressionObjectType);

[Copilot]

The variable name accessedMemberFunctionTypeType is confusing with "Type" appearing twice. Since this variable holds a pointer to the FunctionType of the accessed member, it should be named accessedMemberFunctionType for clarity. The double "Type" doesn't add meaningful information and makes the code harder to read.