sorting diagnostics by source location leads to very confusing output

[zygoloid]

Description of the bug:

The toolchain sorts diagnostics into source location order before emitting them. This results in very confusing diagnostics in cases where a diagnostic at an earlier source location is caused by a diagnostic at a later source location.

What did you do, or what's a simple way to reproduce the bug?

Simple testcase, with a forgotten comma between function arguments:

fn F(n: i32, m: i32);

fn G() {
  F(1 2);       
}

What did you expect to happen?

The first diagnostic I see should report the syntax error: a missing comma. After that, depending on our error recovery strategy, we could perhaps diagnose that the function received only one argument and expected two.

What actually happened?

The first diagnostic produced does not correspond to the error. In fact, the first message indicating what I did wrong is the third diagnostic.

/tmp/zshwmOMMJ:5:4: No matching callable was found.
  F(1 2);
   ^
/tmp/zshwmOMMJ:2:1: Function cannot be used: Received 1 argument(s), but require 2 argument(s).
fn F(n: i32, m: i32);
^
/tmp/zshwmOMMJ:5:7: Expected `,` or `)`.
  F(1 2);
      ^

Any other information, logs, or outputs that you want to share?

I think we would get more reasonable output here if instead of sorting by the source location of the diagnostic, we sorted instead by how much of the file we've looked at, perhaps using the location of the first token we've not yet consumed as the sort key when producing a diagnostic from the parser, and the location of the first parse node we've not yet consumed as the sort key when producing a diagnostic from semantics. That way, errors produced while producing the input sequence of tokens or parse tree nodes would appear before errors produced while consuming them.

[jonmeow]

To offer a different solution, I think the "No matching callable was found" error should have been suppressed. That is, given the contents were erroneous, maybe it wasn't appropriate to give an error that functions couldn't be used. The general principle would be not printing errors for expressions that have already printed errors.

[zygoloid]

To offer a different solution, I think the "No matching callable was found" error should have been suppressed. That is, given the contents were erroneous, maybe it wasn't appropriate to give an error that functions couldn't be used. The general principle would be not printing errors for expressions that have already printed errors.

I think that amounts to saying that we won't do any fine-grained error recovery, even when we're confident about the cause of and solution to the prior error. That could have a fairly substantial cost in terms of needing additional iterations to find and fix all the errors in code, and means we can't build a mode like Clang's -fixit. But maybe that's the right thing for the user experience.

However, it's also not a complete solution for the problem. For example, if the parser produces a warning that a construct was probably not what the developer intended, it's likely important that they see that before they see any semantic errors stemming from that construct, but the sorting by source location will get in the way of that.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please comment or remove the inactive label. The long term label can also be added for issues which are expected to take time.

This issue is labeled inactive because the last activity was over 90 days ago.