-
Notifications
You must be signed in to change notification settings - Fork 520
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
Documenting diagnostic items with their usage and naming conventions #1192
Conversation
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.
This looks great :) the only other thing I'd want to see is when to use lang_items over diagnostic items, but that doesn't have to block this.
I've seen a comment in Clippy's documentation regarding this, but I'm honestly not sure what the guidelines are. Could you elaborate on this a bit more? @jyn514 🙃 |
Co-authored-by: Camelid <[email protected]>
Thank you for the review @camelid, you found my usual spelling mistakes ^^. |
Oh, that wasn't meant to be a gotcha question - I honestly don't know ( #1119 (comment)). If you're not sure either we can leave it out until we hear from @nikomatsakis. |
No problem, I didn't take it that way 🙃
Sounds like a plan, I would leave the PR open a little longer, about a week, to give the named parties in cc some time as well 🙃 |
I think the answer is:
|
Just a Clippy dev perspective: I think there is a potential answer of - Clippy should just use diagnostic items for consistency and simplicity, even if it means that some items are both a lang item and a diagnostic item. The answer I have gone with thus far is - prefer lang items in Clippy if they happen to be available. I guess the only reason I prefer lang items is that they seem a little less error prone since they are strongly typed (I wonder if diagnostic items could be granted a similar API). |
@camsteffen you don't want the API to be strongly typed - that means you have to add the diagnostic items in both the compiler and the standard library, which hurts compile times. You'd still have to handle the diagnostic item being missing either way so I don't know how much it's buying you. |
As a concrete, hypothetical example, with diagnostic items you might mistakenly use |
As a non-hypothetical example, I recently (in some unfinished code) wanted to match a In general, people will try things until something works. |
My understanding from @nikomatsakis comment is, that diagnostic items should be domain specific to diagnostics. This is implied for me by the location I agree here with @llogiq that contributors will most likely just take what's available. I personally prefer diagnostic items in Clippy for consistency, as @camsteffen said. Additionally, diagnostic items are easier to add, PRs for those are usually merged quite quickly.
FYI: Rust already has some items which have both a lang and diagnostic item attached to them, and everything seems to be working well. |
Well, in any case this PR is a great improvement as-is. @camelid wanted to review the wording in detail but otherwise it seems good to me :) |
I don't have a problem with having both a lang and a diagnostic item. I am also ok with people using lang items if they are available, tbh. I think I would not want a language semantics change using a diagnostic item, however. |
Yeah, this is in line with my expectation as well. |
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.
Thanks!
Update books ## rust-by-example 1 commits in 04f489c889235fe3b6dfe678ae5410d07deda958..9d4132b56c4999cd3ce1aeca5f1b2f2cb0d11c24 2021-08-17 08:01:20 -0300 to 2021-09-14 06:56:00 -0300 - Fix link to "integration testing" page (rust-lang/rust-by-example#1458) ## rustc-dev-guide 17 commits in 95f1acf..9198465 2021-08-31 12:38:30 -0500 to 2021-09-12 11:50:44 -0500 - Clarify difference of a help vs note diagnostic. - remove ctag section - Update suggested.md - Update SUMMARY.md - Move ctag section to "Suggested Workflow" - Delete ctags.md - Clarify paragraph in "Keeping things up to date" - Docs: added section on rustdoc - Docs: made suggested fix - Docs: deleted copy - Docs: added section discussing core ideas - Docs: delete redundant use of correctness - Docs: consolidated parallelism information - Add links to overview.md (rust-lang/rustc-dev-guide#1202) - Spelling change intermidiate to intermediate - Fix a typo (rust-lang/rustc-dev-guide#1200) - Documenting diagnostic items with their usage and naming conventions (rust-lang/rustc-dev-guide#1192) ## embedded-book 1 commits in c3a51e23859554369e6bbb5128dcef0e4f159fb5..4c76da9ddb4650203c129fceffdea95a3466c205 2021-08-26 07:04:58 +0000 to 2021-09-12 12:43:03 +0000 - 2.1(QEMU): update app name for git project init (rust-embedded/book#301)
Update books ## rust-by-example 1 commits in 04f489c889235fe3b6dfe678ae5410d07deda958..9d4132b56c4999cd3ce1aeca5f1b2f2cb0d11c24 2021-08-17 08:01:20 -0300 to 2021-09-14 06:56:00 -0300 - Fix link to "integration testing" page (rust-lang/rust-by-example#1458) ## rustc-dev-guide 17 commits in 95f1acf..9198465 2021-08-31 12:38:30 -0500 to 2021-09-12 11:50:44 -0500 - Clarify difference of a help vs note diagnostic. - remove ctag section - Update suggested.md - Update SUMMARY.md - Move ctag section to "Suggested Workflow" - Delete ctags.md - Clarify paragraph in "Keeping things up to date" - Docs: added section on rustdoc - Docs: made suggested fix - Docs: deleted copy - Docs: added section discussing core ideas - Docs: delete redundant use of correctness - Docs: consolidated parallelism information - Add links to overview.md (rust-lang/rustc-dev-guide#1202) - Spelling change intermidiate to intermediate - Fix a typo (rust-lang/rustc-dev-guide#1200) - Documenting diagnostic items with their usage and naming conventions (rust-lang/rustc-dev-guide#1192) ## embedded-book 1 commits in c3a51e23859554369e6bbb5128dcef0e4f159fb5..4c76da9ddb4650203c129fceffdea95a3466c205 2021-08-26 07:04:58 +0000 to 2021-09-12 12:43:03 +0000 - 2.1(QEMU): update app name for git project init (rust-embedded/book#301)
…ust-lang#1192) * Documenting diagnostic items with their usage and naming conventions * Fixed typos in diagnostic items documentation Co-authored-by: Camelid <[email protected]> Co-authored-by: Camelid <[email protected]>
🖼️ Rendered 🖼️
This PR documents the following aspects about diagnostic items:
I focussed on general information that is true for both, writing lints in rustc and Clippy. The documentation still contains some related links and comments about the usage in Clippy. I hope that this is a nice balance.
cc: @rust-lang/clippy, @rust-lang/wg-diagnostics
Meta
Closes #1188
Also referencing rust-lang/rust-clippy#5393 for good measure. Hello Clippy repo!
That's it, I hope everyone is having a great day. 🙃