Dyno: allow mutating const fields in functions called directly from initializers.

[DanilaFe]

Fixes Dyno failure while resolving types/records/const-checking/constructors.

The test was failing because in production, constness checking has a special exception for when const fields of this are changed in methods. Specifically, we allow methods to change const fields of this, but only if they are called directly from the initializer, and not in any other way.

This PR implements this functionality in Dyno. Since Dyno is query-based, it is unable to examine the call stack to see if the current call is being resolved in the initializer. Instead, we take the same approach we took for propagating compilerError/compilerWarning in #26613. Instead of emitting errors directly when const fields are changed, we store the fields into the ResolvedFunction when appropriate, and check them later. "Later" is when the final called candidate is selected, during return intent overload selection, which, conveniently, is in the same function as const checking.

Between detecting and emitting errors, we can decide to silence them. This happens when the current function is init or init=, AND the called function is called on this (either explicitly or implicitly).

Reviewed by @benharsh -- thanks!

Testing

• dyno tests
• paratest
• paratest --dyno-resolve-only

[benharsh]

When does this resolve in relation to InitResolver's logic? InitResolver has the responsibility of identifying uses of this, so it might be convenient to ask the initResolver object about this particular AST. That could happen through InitResolver collecting uses of this as it goes along, to check against later. Alternatively, we could expand InitResolver's logic to be usable by callers.

Doing something like this might help with the fragility of this code.

[DanilaFe]

I've looked into this following your comment. I see logic in the InitResolver with a very similar purpose (detecting field access). Today, however, I prefer my logic, because it relies on toId and thus can tell whether an identifier or dot refers to a field for certain, without re-searching the composite type's fields for the identifier as InitResolver does. I'll add a note to maybe-const to consider integrating the logic, but for the time being I'm inclined to leave it at that: I don't want to change InitResolver (it's out of scope for the PR), and I don't want to change my logic to InitResolver's.

[bradcray]

Just curious since it wasn't obvious to me from the OP: Does this distinguish between an initialization of a const field within an init() and a re-assignment of it within that initializer (after the explicit or implicit init this;)?

(I wasn't certain whether production did the right thing here, but it seems to).

[DanilaFe]

The case you list predates this PR, and is handled in other ways. The specific error added here is for const fields changed in a different function --- specifically not init --- which is only ever called from init. This is a special exception to the const checking error messages. Notably, this pattern cannot happen prior to init this, because we can't call functions at that time. Thus, the answer is, no, we don't emit errors when a field is first set (either implicitly via a default initialization invoked as part of init this, or explicitly), then changed by a called function, because this is the necessary precondition for this feature to work. This matches production's behavior.

[bradcray]

Thanks for clarifying the called function part, which I saw, but obviously didn't absorb.

Now that I've opened up the test, though, to see an illustration of what you're describing, I'm even more confused. If we don't permit modifying a const after an init this; in-line, why would we allow modifying it through a function call? (I understand this is what production does, but is it mistaken in doing so? Seems like either anything within the dynamic scope of the init() should be able to modify it or nothing should. What am I forgetting?)

The mention of "phase 2" (old-school terminology) in the production compiler's error message makes me wonder whether it was our intention to permit modifications anytime within init() but didn't remove a pre-existing check?

I seem to be able to convince myself either is acceptable from moment to moment. And in a quick check of the spec I'm not seeing anything to set me straight (but I'm mostly trying to sign off at this point).

[DanilaFe]

It's not mistaken in doing so; there is an explicit specification of this behavior. I asked in slack about this, and was told that this behavior is somehow required for array initialization or some other deeply internal type. Putting together that comment and the explicit specification in the testing system, I gathered that we should implement this.

When I consulted the production implementation during implementing this, there too it was specially handled; I had the impression that the logic was deliberate.