Consider removing `strictCasts` awareness from the constant evaluator

[pq]

The type system object will soon not track strictCasts (see: #53873) and in the process of evaluating that impact, we noticed that the const evaluator is using this setting when checking assignability in order to avoid reporting duplicate errors. Chatting with @scheglov, this was arguably not the best place to de-duplicate and looking forward we want to get away from the const evaluator doing these checks. In the short term, removing these checks would result in potentially duplicated errors but will unblock #53873 in the service of the big analysis options refactor (#53876).

@kallentu and I will chat about this later and I'll update w/ next steps.

/fyi @bwilkerson

[scheglov]

In these places we checked that typeSystem.runtimeTypeMatch() is false, so we have to return InvalidConstant.
We can check whether to report it when we are inside ConstantVerifier.
This checking mechanism is not available yet, but I think we can implement it.
And this would be cleaner than passing strictCasts that by itself does not mean anything to constant evaluation.
Also, ask yourself, which strictCasts do you want to use when you evaluate a constant in a library imported from another package, with different analysis_options.yaml file?

[bwilkerson]

We can check whether to report it when we are inside ConstantVerifier.

I'm not convinced that's true without duplicating some of the work already done by the constant evaluator. Consider the case where the exception is thrown in a constructor that is potentially removed from the current scope of the ConstantVerifier.

Also, ask yourself, which strictCasts do you want to use when you evaluate a constant in a library imported from another package, with different analysis_options.yaml file?

Wouldn't that question need to be answered regardless of where the deduplication is done?

[scheglov]

We can check whether to report it when we are inside ConstantVerifier.

I'm not convinced that's true without duplicating some of the work already done by the constant evaluator. Consider the case where the exception is thrown in a constructor that is potentially removed from the current scope of the ConstantVerifier.

What work is done by the constant evaluator?

For the specific example that you provided, I expect that the constant evaluator will attach information to the InvalidConstant that says that there was an exception. These we will always report as diagnostics.

OTOH, InvalidConstants that are result of runtimeTypeMatch() will have different information saying so, are not always have to be reported.

Also, ask yourself, which strictCasts do you want to use when you evaluate a constant in a library imported from another package, with different analysis_options.yaml file?

Wouldn't that question need to be answered regardless of where the deduplication is done?

No. If the constant evaluator always report a runtimeTypeMatch() error and leaves it to ConstantVerifier to decide whether report it, then strictCasts only controls whether to report the runtime error that happened during evaluating a constant expression in the current file. For the current file the value of strictCasts is well defined.

[pq]

I share some of Brian's hesitation but value the continued conversation. Thanks!

After chatting with @kallentu I feel really strongly that we don't want to degrade the current experience as duplicated errors are a substantial source of confusion for users.

To that end, I propose that we move ahead with the approach proposed in https://dart-review.googlesource.com/c/sdk/+/341336 with a TODO linking to this issue inviting re-evaluation. If there's a better way to do this, preserving the current behavior and removing strictCasts awareness that can be picked up as a separate arc of work.

Sound good?

[scheglov]

I agree about not degrading the current experience.
The question is only how to achieve this.

@bwilkerson Do we add passing strictCasts as technical debt? How long lived will be this TODO, and whether we will ever pick up this separate arc of work?

I don't agree about passing strictCasts everywhere in constants evaluation. But @bwilkerson is TL, so if he says that we are doing this, I will review the rest of the CL and rubber stamp the constant evaluation portion of it.

[kallentu]

We can check whether to report it when we are inside ConstantVerifier.

What would this check consist of? Would it not still use isAssignable (and hence, strictCasts)?

[scheglov]

We can check whether to report it when we are inside ConstantVerifier.

What would this check consist of? Would it not still use isAssignable (and hence, strictCasts)?

Please read my next message in this thread, it adds some details about strictCasts.

The answer, if a vague form, is the new issue that @pq opened - a generate mechanism for knowing which diagnostics were already reported, with tracking where. Very specifically, but possibly not optimally, for constants, we could ask the error reporter, if there were type errors (or maybe any errors) reported in the source range of the evaluated constant expression. Performance is probably not important here, we are on the rate path with an error.

[kallentu]

Sorry, I just don't really understand the second part where you wrote
leaves it to ConstantVerifier to decide whether report it, then strictCasts only controls whether to report the runtime error that happened during evaluating a constant expression in the current file. For the current file the value of strictCasts is well defined.. I don't really comprehend what the vision of this looks like (ie. I'm a little confused! If you'd like to help me understand that more)

If we're talking about a new mechanism to handle what diagnostics are reported, the constant evaluator currently doesn't know of the errors that are already reported by the other passes. Its ErrorReporter is shared with the ConstantVerifier, but not ErrorVerifier for example. We would need to make a common error reporter and do deduplication if we are going down that route.

If we choose to do it differently, @pq and I have come up with some other ideas and heuristics. I can write them up in a little doc to discuss. But I think in general the solution to this, I agree, is to have a mechanism for knowing which diagnostics were already reported. Probably with some more discussion though?
I agree that "passing strictCasts everywhere in constants evaluation" isn't great, but I don't know if this is something to block on without some more work to tackle the larger issue. And I feel like it's going to be an arc of work that's larger than to tack onto Phil's PR.

[scheglov]

And I feel like it's going to be an arc of work that's larger than to tack onto Phil's PR.

I don't propose doing this arc of word in the same CL.
If we going to rework de-duplication, it will be in its own CL.