r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

r? estebank

I don't think we should do this.
The alias annotations are targeted at other people using the crate, not the crate's author.
However, the diagnostics here are targeted at the crate's author, the same person who added the alias annotations.
That's what the removed comment says as well.

So it's a low value diagnostic change, but it requires keeping a new global mutable resolver state and doing extra work on a good path -> doesn't pull its weight.

I don't think we should do this. The alias annotations are targeted at other people using the crate, not the crate's author. However, the diagnostics here are targeted at the crate's author, the same person who added the alias annotations. That's what the removed comment says as well.

Valid point. Not entirely true though, because, doc alias is actually used initially before this pr in the same crate for method and associated functions for diagnostics. Also, the comment was actually also a little bit misleading, after investigating it, the lookup_doc_alias_name was using hir::find_attr! to get the attribute that has been parsed and at that point, we are not yet in hir for same crate so the look_up_doc_alias will panic at that point for function in the same crate while it will work for function in external crate using crate metadata data. Could the reason for not having doc alias for function in the same crate for diagnostics be that it was not sort of possible with the external crate approach ? I asked this because, doc alias for diagnostics works for both extern and same crate works for method and associated function which doc alias attribute was easily gotten as their resolution happens in typecheck hir after attribute panic. So, why have it for method and not function if the doc alias for diagnostics is targeted for people using external and not that because it would cause panic for function ?

So it's a low value diagnostic change, but it requires keeping a new global mutable resolver state and doing extra work on a good path -> doesn't pull its weight.

This is true, but, looking at my reason above, do you think this stills stands ?

This also works for methods and associated function in a trait for the same crates for diagnosis. So, I believe not working for function is more of the hacking that needs to be done to supportting it.

doc alias is actually used initially before this pr in the same crate for method and associated functions for diagnostics.

Yeah, because there (in compiler\rustc_hir_typeck\src\method\probe.rs I suppose) it's zero-cost, all the attributes (local or not) are already parsed and available, using them for diagnostics doesn't have drawbacks. (Technically we can disable the diagnostics for the local cases there, but is no strong motivation for the either choice, so it's just simpler not to do that.)
Here in rustc_resolve the local attributes are not yet available, that's why you have to introduce and fill a separate table, which is a drawback, so it's simpler to disable the diagnostics for the local case.
I don't think consistency between diagnostics in rustc_hir_typeck and in rustc_resolve is too important.

using hir::find_attr! to get the attribute that has been parsed and at that point, we are not yet in hir for same crate

That's an (ideological) issue with the new attribute parsing infra design, parsed attributes are not HIR, and are often used before HIR is built (like in rustc_resolve), but the new infra uses HIR for naming and puts itself into HIR modules.

The alias annotations are targeted at other people using the crate, not the crate's author.

Do note that in some cases, the crate's author is a collection of people of different levels of expertise with the codebase. Think large commercial projects with high turn-over. Those would likely want to be able to leverage existing mechanisms for onboarding new engineers. Realistically, it's true that a lot of these projects would already be relying on workspaces and splitting their projects in multiple crates, but that is not necessarily a given.

However, the diagnostics here are targeted at the crate's author, the same person who added the alias annotations.
That's what the removed comment says as well.

I highly suspect that the order in which the thought process went was "we don't get this for free" -> "we don't need it anyways", which is perfectly defensible! But the comment should likely have included both points :)

So it's a low value diagnostic change, but it requires keeping a new global mutable resolver state and doing extra work on a good path -> doesn't pull its weight.

Agreed on the additional cost to Resolver, but on the "normal" case, items are not annotated with attributes (right now I don't recall if derive attributes are present in this case? If so, my position might change), which wouldn't trigger the opportunistic search for alias. We should do a perf check to confirm if this does affect performance in any real way.

doc alias is actually used initially before this pr in the same crate for method and associated functions for diagnostics.

Yeah, because there (in compiler\rustc_hir_typeck\src\method\probe.rs I suppose) it's zero-cost, all the attributes (local or not) are already parsed and available, using them for diagnostics doesn't have drawbacks. (Technically we can disable the diagnostics for the local cases there, but is no strong motivation for the either choice, so it's just simpler not to do that.)
Here in rustc_resolve the local attributes are not yet available, that's why you have to introduce and fill a separate table, which is a drawback, so it's simpler to disable the diagnostics for the local case.
I don't think consistency between diagnostics in rustc_hir_typeck and in rustc_resolve is too important.

The lack of consistency is a minor papercut that might confuse people by not matching their expectations. I agree it is not too important to address, but I'd like to minimize divergences as much as possible. Otherwise, we'd want a lint on doc(alias) on crate-private items, letting the user know that it does nothing.

@petrochenkov - you might want to take a look at the last comment.