Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sorting diagnostics by source location leads to very confusing output #3054

Open
zygoloid opened this issue Aug 2, 2023 · 3 comments
Open
Labels
long term Issues expected to take over 90 days to resolve. toolchain

Comments

@zygoloid
Copy link
Contributor

zygoloid commented Aug 2, 2023

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
Copy link
Contributor

jonmeow commented Aug 2, 2023

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
Copy link
Contributor Author

zygoloid commented Aug 2, 2023

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.

Copy link

github-actions bot commented Nov 1, 2023

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.

@github-actions github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Nov 1, 2023
@jonmeow jonmeow added long term Issues expected to take over 90 days to resolve. and removed inactive Issues and PRs which have been inactive for at least 90 days. labels Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long term Issues expected to take over 90 days to resolve. toolchain
Projects
None yet
Development

No branches or pull requests

2 participants