Introduce linter for diagnostic messages#89455
Introduce linter for diagnostic messages#89455hkmatsumoto wants to merge 1 commit intorust-lang:masterfrom
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.