-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Include scope names in diagnostics #49898
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
I opened a pre-RFC for this. https://internals.rust-lang.org/t/pre-rfc-include-scope-names-in-diagnostics/7304 . Will rewrite the implementation to suite the consensus from discussion. |
☔ The latest upstream changes (presumably #49969) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage @nikomatsakis! This PR needs your review! |
Some thoughts:
I do agree that this information is valuable, but I think I would prefer an alternate presentation. I'd also prefer for us to gather the information in a different way. I think however I'll move my concerns over to the internals thread. I'm going to mark this PR as S-blocked for the time being -- however, I'm sort of inclined to close it, just because my queue is long and we're not ready to move here yet. But I'll hold off on that for now. |
14857fa
to
92dd733
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
92dd733
to
30c21ad
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@nikomatsakis - reminded you pinged about this. When we designed the current error messages, we did go through an iteration of giving more information about the location. When we did our original survey of developers, many said the first they they look for is the file and line number. After seeing this pattern, we simplified the idea down to have the file and line number on a line by itself. The drawback of this proposal is that it loses some of that, and it makes it more difficult to, at-a-glance, catch the file and line number for those types of developers. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
So far, rustc has only printed about filenames and line numbers for warnings and errors. I think it is rather missed, compared to `gcc` and other compiler, that useful context information such as function names and structs is not included. The changes in this pull request introduce a new line emission in diagnostics to implement this.
6efc19e
to
5de482c
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Because I have so many dang PRs pending, I'm going to close this pending any final decision (we can always re-open!). I think my post on the internals thread (which I confess I've been too slammed to keep up with) still represents my opinion. |
@nikomatsakis Thanks! I am going to prepare an RFC based on all the comments. |
Pre-RFC: rust-lang/rfcs#2399
So far, rustc has only printed about filenames and line numbers for warnings and errors. I think it is rather missed, compared to
gcc
and other compiler, that useful context information such as functionnames and structs is not included.
The changes in this pull request introduce new emissions in diagnostics to implement this.
Short example:
Check
in fn test
above.The information for the line is gathered by a mechanism to resolve a
Span
to the corresponding named scope, by a Trait dynamic call fromlibrustc_error
back tolibsyntax
, in addition to what is done withCodeMap
. The call does a quick DFS into the AST for theSpan
and collects the relevantIdent
path, so it should handle nested scopes properly.To do:
AST prior to macro expansion?
A larger example:
The output is: