Skip to content
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

Merged
merged 2 commits into from
Sep 1, 2021

Conversation

xFrednet
Copy link
Member

🖼️ Rendered 🖼️

This PR documents the following aspects about diagnostic items:

  • Background
  • How To Find Diagnostic Items
  • How To Add Diagnostic Items
  • Naming Conventions
  • How To Use Diagnostic Items
    • Check For A Type
    • Check For A Trait Implementation
    • Associated Types
    • Usage In Clippy
  • Related Issues

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. 🙃

Copy link
Member

@jyn514 jyn514 left a 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.

src/diagnostics/diagnostic-items.md Outdated Show resolved Hide resolved
src/diagnostics/diagnostic-items.md Outdated Show resolved Hide resolved
src/diagnostics/diagnostic-items.md Outdated Show resolved Hide resolved
src/diagnostics/diagnostic-items.md Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member Author

the only other thing I'd want to see is when to use lang_items over diagnostic items

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 <camelidcamel@gmail.com>
@xFrednet
Copy link
Member Author

Thank you for the review @camelid, you found my usual spelling mistakes ^^.

@jyn514
Copy link
Member

jyn514 commented Aug 27, 2021

the only other thing I'd want to see is when to use lang_items over diagnostic items

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 🙃

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.

@xFrednet
Copy link
Member Author

No problem, I didn't take it that way 🙃

we can leave it out until we hear from @nikomatsakis.

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 🙃

@nikomatsakis
Copy link
Contributor

I think the answer is:

  • Use a lang item if you are changing what code is accepted or what the code does when it is compiled
  • Use a diagnostic item if you are only trying to emit better diagnostics

@camsteffen
Copy link

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).

@jyn514
Copy link
Member

jyn514 commented Aug 27, 2021

@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.

@camsteffen
Copy link

As a concrete, hypothetical example, with diagnostic items you might mistakenly use sym::Vec instead of sym::vec_type.

@llogiq
Copy link

llogiq commented Aug 28, 2021

As a non-hypothetical example, I recently (in some unfinished code) wanted to match a str type, and the str_type diagnostic item didn't do this, so I got the lang_item::str instead.

In general, people will try things until something works.

@xFrednet
Copy link
Member Author

My understanding from @nikomatsakis comment is, that diagnostic items should be domain specific to diagnostics. This is implied for me by the location Errors and Lints > Diagnostic items and the focus on lints in the documentation. However, I can add a comment about :)

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.

even if it means that some items are both a lang item and a diagnostic item

FYI: Rust already has some items which have both a lang and diagnostic item attached to them, and everything seems to be working well.

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

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 :)

@jyn514 jyn514 added the waiting-on-review This PR is waiting for a reviewer to verify its content label Aug 31, 2021
@nikomatsakis
Copy link
Contributor

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.

@Manishearth
Copy link
Member

Yeah, this is in line with my expectation as well.

Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@camelid camelid merged commit 139b8fb into rust-lang:master Sep 1, 2021
@xFrednet xFrednet deleted the 1188-diag-item-docs branch September 1, 2021 19:30
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 15, 2021
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 95f1acf9a39d6f402f654e917e2c1dfdb779c5fc..9198465b6ca8bed669df0cbb67c0e6d0b140803c
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)
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 15, 2021
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 95f1acf9a39d6f402f654e917e2c1dfdb779c5fc..9198465b6ca8bed669df0cbb67c0e6d0b140803c
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review This PR is waiting for a reviewer to verify its content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how/when to use diagnostic items
7 participants