-
Notifications
You must be signed in to change notification settings - Fork 11
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
Emit lints using a diagnostic builder #121
Conversation
fde8fa1
to
77972c0
Compare
I've rebased on master, after the nightly bump 🙃 There was a small conflict, but nothing changed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new API, left a few small comments, but I'm a bit concerned about the foo
field lint going away in the tests, even though the code for it is still there. My best guess is that try_to_hir_id_from_emission_node
is returning None
, but I'm not sure
warning: a field named `foo`, consider using a more meaningful name | ||
--> $DIR/foo_items.rs:7:9 | ||
| | ||
7 | foo: i32, | ||
| ^^^^^^^^ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this lint went away, maybe try_to_hir_id_from_emission_node
is failing for FieldId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good catch 💪
Thank you for the review. ❤️ I had a busy week and now I'm catching up with all the fires I left burning. I'll probably get to this on the weekend. 🙃 |
Co-authored-by: Niki4tap <[email protected]> I should almost create an alias for this specific message
The PR should be ready now. I'll probably do some cleanup and maintenance now, to get back into the flow, after a short break :) |
c2c40f9
to
3647b8c
Compare
Nice, I left some things too, will try to come back soon bors r+ |
136: Test: Use `emit_lint` instead of `println!` in tests r=xFrednet a=xFrednet Follow up of #121 Not much more to say. I'll r+ this directly, as these are automatically generated and would be a pain to review. r? `@ghost` Co-authored-by: xFrednet <[email protected]>
136: Test: Use `emit_lint` instead of `println!` in tests r=xFrednet a=xFrednet Follow up of #121 Not much more to say. I'll r+ this directly, as these are automatically generated and would be a pain to review. r? `@ghost` Co-authored-by: xFrednet <[email protected]>
This PR adds a new
DiagnosticBuilder
to the API to create beautiful diagnostics with help and note messages. This will also allow us to include the expression span, in the print tests. Generally, I'm pretty happy with the concept :)Adding new context callbacks to the API currently requires a lot of boilerplate code. I doubt that we can avoid that right now, but I'll try to look at options to generate a part of it. That would at least help with the implementation and review part.
r? @Niki4tap Sorry for requesting this review direction after the last one. You are welcome to take your time or pass the review if you don't have time. It's also totally fine, if you just review the API and doc changes. The backend is mostly verified by the tests :)
Closes #47
Closes #92 (This PR adds the
id()
andspan()
method toStmtKind
)