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

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

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.

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!

cc @estebank

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.

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)

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

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

@bors r=Manishearth,oli-obk

📌 Commit d38f2b0 has been approved by Manishearth,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

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

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

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.

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.

@bors: r-

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

@bors r=Manishearth,oli-obk

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