Constant evaluator support for member constants

[matheusaaguiar]

Fix #16055.

[clonker]

I am not sure if there aren't any unwanted side-effects hiding in this. Looks good to me generally, though!

[clonker]

LGTM aside from the typo

[cameel]

Actually, there are some problems here.

[cameel]

This does not cover all relevant cases. I think it will break at least on these:

• m.CONST (expression is a module)
• (C).CONST (expression is not an identifier)
• m.C.CONST (member access on a member access)

Especially the first and the third one are not far fetched at all.

These are probably not all cases either. I can also think about a few more that are now disallowed due to our previous changes in implicit conversions:

• [C][0].CONST
• (true ? C : C).CONST

[cameel]

This should really be possible to implement in a more general way, without handling all those cases one by one. Have you tried getting the constant definition via _memberAccess.annotation().referencedDeclaration?

[matheusaaguiar]

The problem is that constant evaluation is done "on demand" whenever it is needed, so in some (most?) cases information like referencedDeclaration may not be present and will only be calculated later.
For example, in the case of array sizes, the expression will be evaluated during the DeclarationTypeChecker step and referencedDeclaration is resolved later when TypeChecker step runs.
On the other hand, in the case of the custom storage layout of the contract, the evaluation would occur during the same step and information like referencedDeclaration would be available.
I ended up focusing only in the simplest use of contract constants, and then later was considering handling modules in a different PR to not hold this one back.
I am not sure about expressions like (true ? C : C).CONST, I think maybe it kinda falls out of scope, because currently the constant evaluator can't handle them, so it would not be able to yield C as the "result" there...

[cameel]

in some (most?) cases information like referencedDeclaration may not be present and will only be calculated later.

I see. I looked at this and I think we can change this though. In cases that we are interested in, we could actually calculate referencedDeclaration earlier.

In most cases referencedDeclaration is set by ReferencesResolver, which runs even before DeclarationTypeChecker. It just defers resolution of some Identifiers and all of MemberAccess to TypeChecker, because resolving them often required knowledge about the type. This happens for two reasons:

1. The meaning of the member may change depending on the type of the identifier. E.g. depending on the type of x, x.transfer() can mean:
   • An external call on a contract instance.
   • An internal call on a contract type.
   • A call of the transfer() builtin on address payable.
   • A call of a function pointer stored in a struct.
2. using for can affect overload resolution by adding new candidates to a scope.

However, not all cases are this complicated. Some could be resolved earlier. Specifically member access on an identifier that refers to a known scope (so members are already known) and where the member being accessed is not a function (so overloading is impossible). Our case falls into this, because the name of a contract/library/interface type or of a module represents a scope (that cannot be extended with using for) and constants cannot be overloaded.

So the solution I'd propose here is to add MemberAccess handler for such constants to ReferencesResolver. Then you'll be able to safely access referencedDeclaration in ConstantEvaluator even at this early stage. It not being set will simply mean that it's not a constant.

[cameel]

I am not sure about expressions like (true ? C : C).CONST, I think maybe it kinda falls out of scope, because currently the constant evaluator can't handle them, so it would not be able to yield C as the "result" there...

Yes, we do not have to make this case constant evaluated - it does not work for numbers either. My point was that there may be more such expressions and with your approach we'd have to find and special-case them all. It's not really about whether these particular cases are useful, but about the language being consistent. It should work based on generic rules rather than require us to enumerate cases in which things work. The rule is just that constants can be member-accessed and this rule should compose nicely with the rules we already have on what things can contain constants and rules for how expressions work. We should not have to analyze the expression again here.

I'm more concerned about the (C).CONST case. It's also an expression and this one should work. And that will actually be a problem. My ReferencesResolver suggestion above can cover things like m.C.CONST in a generic way, but anything where the expression is more complex would require special handling for such expressions in ReferencesResolver.

One way around it might be to create something similar to ConstantEvaluator but for evaluating types. So that it could tell us that (C) is really just C. It could give up on anything that involves variables. ReferencesResolver could run that on the base expression of MemberAccess and resolve it. Still, I don't think it's worth the effort for now. We can deal with it later, when we expand the constant evaluation.

So for now, in this PR, it is fine for ConstantEvaluator to just give up on (C).CONST due to referencedDeclaration not being set.

[cameel]

We need more tests. At least:

• Access to a non-constant member should produce an appropriate error.
• Member access that is not a top-level expression in array size, e.g. a[1 + C.CONST + 2].
• Nesting: member access to a constant whose value contains member access as well.
• Cycles: Two member access constants using each other as values (should be an error).

Especially the last one looks worrying. I just found #12928 (comment), which may be the reason we had this limitation to begin with. If that's the case, then properly solving this may require deeper changes to analysis phases.

[cameel]

If that's the case, then properly solving this may require deeper changes to analysis phases.

It should be fine actually. The comment mentions that constant cycles are checked only in PostTypeChecker, but we also have a recursion depth check in ConstantEvaluator::evaluate(), which will catch the cycle if a cyclic constant is used as an array length. This already works for constant Identifiers and should work for MemberAccess as well.

It's not great that it's not PostTypeChecker that catches this, but we can live for now and we can try to fix it when we get to implementing compile-time constant evaluation.