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.

[nikola-matic]

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.

[rodiazet]

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

[clonker]

I couldn't find discrepancies between develop and the refactor when tracing through the code paths. I'd feel better if someone else did the same before we merge. The changes are quite convoluted at times :)

What I found:

• checkAccessedMemberFunction is called after requiredLookup = Super - develop does it before. These are independent operations but the reordering made it harder to review ofc.
• if -> solAssert for StringConcat/BytesConcat is technically not equivalent. Develop silently skips isPure = true if the condition fails and with this PR we get an abort.
• varDecl check is moved below MagicType handling. This is equivalent today but a potential bug source if MagicType members ever change. Guess this should be folded to where it actually belongs in a follow-up (as the todo states)
• The general reordering throughout made it hard to track code paths

I am sure that none of these lead to wrong results but at least the second and third point aren't strictly equivalent.

[cameel]

• if -> solAssert for StringConcat/BytesConcat is technically not equivalent. Develop silently skips isPure = true if the condition fails and with this PR we get an abort.

It's equivalent if you look at the broader context. bytes/string types are not supposed to have other members (and we excluded the case of a bound function).

I'd feel better if someone else did the same before we merge. The changes are quite convoluted at times :)

I did already. I compared both versions and made sure nothing was lost or added. Now everything I found back then seems fixed and I went through it again just in case (though in much less detail). If we both did that then I think we'll be find here.

And yeah, this is indeed a hell to review. Though TBH it's just that the logic here was convoluted to begin with, with many overlapping cases and multiple responsibilities stuffed into one monster function. I'll take cleaning this up once and for all over having to do this in my head every time I go over #16376 and any other change related to constants in the future :)

[cameel]

The code is fine now. Approving, but please don't forget to squash the review fixes before merging.

Would not hurt to add an assert for #16448 (comment) too.

[clonker]

It's equivalent if you look at the broader context. bytes/string types are not supposed to have other members (and we excluded the case of a bound function).

Makes sense, thx for the clarification!

If we both did that then I think we'll be fine here.

Good to go then :)

And yeah, this is indeed a hell to review. Though TBH it's just that the logic here was convoluted to begin with, with many overlapping cases and multiple responsibilities stuffed into one monster function.

Agree! With core solidity on the horizon not a thing anymore, but I think that perhaps there's something to be gained from rethinking the c++ types that encode solidity types. I suspect at least some of the repeated checks here could be encoded in type invariants.

[cameel]

There's definitely value in improving things even with Core Solidity on the horizon. We will still have to maintain Classic for quite a while and must not let it deteriorate and end up with some crits in the process.

I agree about the types. We could use fewer manual checks for every specific thing and more general properties on them.

One example of this that still sits in my head is that we may have avoided the recent transient storage codegen bug if we had Type::identifier() properly reflect the location. Requiring the user of the class to check location and build the right identifier is more error prone. The type should do more.

Or the named parameter ordering bug could have been avoided if the parameters were encapsulated in a proper type, aware of its purpose, rather than just a dumb collection being passed around without any safeguards.

We just have to be selective about it. This refactor ended up being much harder than I expected and this is likely to be the case with anything touching analysis due to all the interdependencies. I think we might have been better off slicing it into even smaller pieces. Any changes that need to shuffle big chunks of code around would probably be easier to review if done separately with as few other modification thrown in as possible.

[rodiazet]

I think refactoring PRs should be handled in different way. This one contains a lot of changes which potentially do not change anything, but because there is so many different aspects it's hard to follow them. I think next time I will try to push smaller PRs with minimal changes which in summary will end up with bigger refactoring. Actually each of the initial commits from the PR should be a separated PR.

[cameel]

Can you squash "Address PR review comments." into other commits? All the review changes fixes should generally be squashed into the original commits they apply to.

[cameel]

Why do you want to check magic type but i.e. not struct type also?

MagicType was the only thing checked after VariableDeclaration. Structs and other stuff were originally before so nothing changed there.

If I get it right I think it's already checked in the following PR

Yes, that assert would check that and more. You could move it here. Or add the one @clonker suggested.