Add diagnostic items for Clippy

[xFrednet]

This adds a bunch of diagnostic items to std/core/alloc functions, structs and traits used in Clippy. The actual refactorings in Clippy to use these items will be done in a different PR in Clippy after the next sync.

This PR doesn't include all paths Clippy uses, I've only gone through the first 85 lines of Clippy's paths.rs (after rust-lang/rust-clippy#7466) to get some feedback early on. I've also decided against adding diagnostic items to methods, as it would be nicer and more scalable to access them in a nicer fashion, like adding a is_diagnostic_assoc_item(did, sym::Iterator, sym::map) function or something similar (Suggested by @camsteffenon Zulip)

There seems to be some different naming conventions when it comes to diagnostic items, some use UpperCamelCase (BinaryHeap) and some snake_case (hashmap_type). This PR uses UpperCamelCase for structs and traits and snake_case with the module name as a prefix for functions. Any feedback on is this welcome.

cc: rust-lang/rust-clippy#5393

r? @Manishearth

[xFrednet]

The pipeline shows why some diagnostic items use #[cfg_attr(not(test), rustc_diagnostic_item = "...")]. I would apply the fixes and your suggestions in the two commits and force push the changes again. Would that be okay for you @Manishearth

[camsteffen]

I would just add diagnostic items for modules cmp, mem and iter. We can use tcx.parent to check the containing module of a function DefId (and possibly add utils to make it easier).

Aside: I am looking into adding a new lookup util so we can write code like lookup!(cmp::min).has_def_id(id).

[xFrednet]

Wouldn't adding diagnostic items to the modules kind of defeat the purpose of using diagnostic items instead of paths? We wouldn't match against the whole path, but it seems weird 🤔
On second thought, it would make sense if we would have a function like is_diagnostic_assoc_item then it would basically be the same as asking for a method on a struct/trait. Do we still want to add them in this PR in the meantime?

Such a lookup macro would be awesome, btw.

[Manishearth]

I would just add diagnostic items for modules cmp, mem and iter. We can use tcx.parent to check the containing module of a function DefId (and possibly add utils to make it easier).

Hmm, but doesn't that bring us closer to what we used to have? Seems faster and cleaner to just have diagnostic items for everyone.

The pipeline shows why some diagnostic items use #[cfg_attr(not(test), rustc_diagnostic_item = "...")]. I would apply the fixes and your suggestions in the two commits and force push the changes again. Would that be okay for you @Manishearth

Yeah, sure!

[Manishearth]

cc @estebank

[Manishearth]

Suggested change

	#[cfg_attr(not(test), rustc_diagnostic_item = "HashmapEntry")]
	#[cfg_attr(not(test), rustc_diagnostic_item = "HashMapEntry")]

[camsteffen]

I recommend that only because some functions are very closely tied to the module they are in. min will always be cmp::min. This is reflected in that you choose to name it cmp_min. This approach means adding a lot less diagnostic items and less going to check what the diagnostic item is for specific items. It's analogous to not adding a diagnostic item for every Iterator method. We could have a util method like is_diag_child_named for now. It would be more frictionless with my lookup! idea.

[Manishearth]

Maybe someone from @rust-lang/compiler can make sure we're not doing anything wrong here

[camsteffen]

It would work with associated types, associated methods, inherent methods, module items, ...

lookup!(HashMap)lookup!(HashMap::Entry)lookup!(HashMap::insert)lookup!(Iterator::find)lookup!(cmp::min)

[xFrednet]

I definitely like the idea. Note that it probably wouldn't work for HashMap::Entry as that type is separate from the HashMap struct itself (This was probably just an example).

Using the parent search method would probably still not fully work for reexports though. std::iter::repeat for instance, has the actual path core::iter::sources::repeat::repeat adding a diagnostic item to the iter module would only drop the first core:: part of it

[Manishearth]

Right, part of the reason behind having diagnostic items is never having to know the internal organization of the crate. It makes sense to do parent lookups for methods/etc but not as much for free functions

[oli-obk]

@bors r=Manishearth,oli-obk

[bors]

📌 Commit d38f2b0 has been approved by Manishearth,oli-obk

[oli-obk]

for future reference on method lookups of traits/impls: https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/Diagnostic.20Item.20Naming.20Convention.3F

[xFrednet]

Right, part of the reason behind having diagnostic items is never having to know the internal organization of the crate. It makes sense to do parent lookups for methods/etc but not as much for free functions

Would it maybe be worth it to implement a function/macro like lookup! that doesn't use diagnostic items at all and instead resolves the items like use statements do?

My understanding is that paths in use statements resolve to the original DefId even if the item has been reexported. This would allow the usage of these for diagnostics without knowing the internal design of these crates. Example: lookup("std::iter::repeat") would return the DefIdfor core::iter::sources::repeat::repeat as that is also the function that is being called when the user uses use std::iter::repeat.

Has there been any previous discussions about a concept like this?

Edit: @oli-obk thanks for posting the link again, should we move the discussion over to Zulip? 🙃

[xFrednet]

I've created a Zulip topic with a short summary here: https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/Use.20of.20diagnostic.20items.20.28discussion.20from.20rust.2387170.29/near/246219798

[Manishearth]

The point of diagnostic items was, in part, to not have to do any such lookups: I don't see what problem it solves to switch from diagnostic items to lookups.

[xFrednet]

My understanding was that the main point of diagnostic items was to avoid hard coded paths to the exact item. That problem would be solved, and it would make a slightly nicer interface IMO. But that's it.

There has also been some feedback on Zulip that using the resolver for this can be problematic and produce ICEs. I'll dig a bit more through the compiler, but it seems like diagnostic items are a currently the best solution.

[GuillaumeGomez]

@bors: r-

[xFrednet]

Sorry, that I didn't fix the error earlier, the error message confused me at first. Now everything should hopefully be correct 🙃

[oli-obk]

@bors r=Manishearth,oli-obk

[bors]

📌 Commit 67002db has been approved by Manishearth,oli-obk