search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.57k forks source link

Re-evaluate and rationalize how errors are de-duplicated #54343

Open pq opened 10 months ago

pq commented 10 months ago

(Follow-on from a discussion in stand-up.)

For more context see https://github.com/dart-lang/sdk/issues/54338 which proposes removing some duplication checks from constant evaluation. The short-term result would be a degraded UX which may not be a good bargain. (The cost is possibly messier code -- though there isn't complete agreement there.) Net-net: in the long run it would be interesting to evaluate other approaches to error de-duplication.

/fyi @kallentu @bwilkerson @scheglov

bwilkerson commented 10 months ago

Do you have one or more concrete proposals, or is this intended more as a space for brainstorming?

pq commented 10 months ago

Nothing concrete. Just a place-holder for brainstorming.

pq commented 10 months ago

in https://github.com/dart-lang/sdk/issues/54338, @scheglov suggests:

[...] 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 commented 10 months ago

There were a couple different possibilities for where to record the errors that @pq and I discussed 2 weeks ago. Here are some notes and thoughts:

First idea - being what Konstantin mentioned with looking for errors reported in the range of the expression in the error reporter. Essentially, looking for overlaps in offset and errors that have been reported before. We would need to re-use the error reporter from the error verification pass and the constant evaluation pass, which currently is not being done.

Second - being attaching error information to the nodes themselves, although you'd have to also know whether each node's subnodes have errors too. Then for example, in constant evaluation, you can just check if that node has an error and avoid extra const evaluation work.

For either or any solution, you would probably need a good heuristic to decide what errors are more important and which one to remove and which one to keep. I'm not completely sure on what this looks like yet, but there's some basic priority for this like choosing syntactic error over semantic error over runtime error (which may occur for const eval). Not sure if that's always the case though and it should be something else we discuss.

bwilkerson commented 10 months ago

There were a couple different possibilities ...

Would either of these possibilities fully prevent the occurrence of duplicate and/or follow-on errors? If not, which would be most likely to do so?

... you'd have to also know whether each node's subnodes have errors too.

Would we? Or would it be enough to mark the top-most expression containing an error and for the evaluator to skip evaluating that expression, producing instead a "valid value of some default type"? If we miss a runtime exception in a subexpression as a result of a semantic diagnostic associated with the parent, I don't think that would be a problem. But maybe I'm not understanding the consequences of that choice.

... choosing syntactic error over semantic error over runtime error ...

I believe that that's always the correct order.