-
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
Introduce linter for diagnostic messages #89455
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
r? @estebank |
This comment has been minimized.
This comment has been minimized.
Please loop me in on the bootstrap details here when this is ready for review. |
This comment has been minimized.
This comment has been minimized.
There are cases where it's not clear to say ok or not okay.
Any thoughts on these? |
I'd lean towards the last of each example. |
The code changes in this repo look good, but am slightly concerned about the out of repo linter. That being said, I think that this linter will be useful. |
I don't understand why it's optional to check diagnostic messages -- shouldn't we always do that in compiletest, not gated on a CLI option? Is there a quick link to the lint rules in diaglint? It looks like we're using a default set right now, but I'm not quickly finding docs on what that set actually is. Can you show an example of a potential lint message? |
…estebank Practice diagnostic message convention Detected by rust-lang#89455. r? `@estebank`
…estebank Practice diagnostic message convention Detected by rust-lang#89455. r? ``@estebank``
…estebank Practice diagnostic message convention Detected by rust-lang#89455. r? ```@estebank```
…shtriplett Follow the diagnostic output style guide Detected by rust-lang#89455.
…shtriplett Follow the diagnostic output style guide Detected by rust-lang#89455.
Fair enough, we can make it default and can manually ignore some lint if needed.
Currently I haven't written any docs, but the lints are defined here. These lints are inspired from here |
644758b
to
d77c7d0
Compare
This comment has been minimized.
This comment has been minimized.
I'll leave it up to @estebank to decide, but I think this probably should receive some T-compiler attention (or at least wg-diagnostics). I'm not personally a fan of out-of-tree lints against our lint definitions -- generally, I would expect us to do that in-tree. Bringing in extra dependencies and another diagnostics engine also feels a little too much, particularly since it's in the critical path for all "first" compiletest invocations rather than some separate not-run-by-users suite. (To be clear, I don't think we should move it out of compiletest; that seems like the right place. It's just that the cost feels higher than needed).
I asked this earlier, but I don't think I've seen how this looks in practice -- maybe I missed it? I would want to make sure if we are linting there is a clear message included on (a) how to disable the lint and (b) what to do to fix the lint, if desired. |
☔ The latest upstream changes (presumably #89541) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
Ah, sorry for not showing the output @Mark-Simulacrum. These are what users will see:
|
☔ The latest upstream changes (presumably #90463) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #89488) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: @hkmatsumoto tests are failing. |
mentioned in today's T-compiler meeting on Zulip, perhaps this needs a bit more discussion during a steering meeting or drafting an MCP |
I guess having a meeting is very good to reorganize the pros and cons. I'm not very sure if I can speak/write English fast enough for smooth conversations, though. |
@hkmatsumoto apologies for my poor communication and vague comment :) (I've just dropped a note to let everyone know this is progressing). Don't worry ! In case your input is needed, the team will gladly help you sort out what is needed. thanks for your patience and contributions! :) |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #92526) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing this as it is waiting on a MCP |
Sometimes, contributors add diagnostics not practicing the diagnostic message convention and overlooked by reviewers. This PR adds a linter for diagnostic messages to preemptively warn such diagnostics and lighten the burden of reviewers.