[Unique-Usman]

Parse #[doc(alias)] attributes for local functions and store them in the resolver. Use these aliases during name resolution diagnostics to suggest the original function when an alias is used in its place.

This enables cases like calling bar() to suggest foo() when foo is annotated with #[doc(alias = "bar")].

Includes UI tests for function and method cases(which has already being supported).

[rustbot]

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 10 candidates

[Unique-Usman]

r? estebank

[petrochenkov]

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.

[Unique-Usman]

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 ?

[Unique-Usman]

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.

[petrochenkov]

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.

[estebank]

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.

[Unique-Usman]

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

[rust-bors]

☔ The latest upstream changes (presumably #156305) made this pull request unmergeable. Please resolve the merge conflicts.

[apiraino]

Visited during compiler triage.
cc @petrochenkov feel free to comment when you have time. This looks a small*-ish* change but let's decide if the win is worth.

[estebank]

This small tweak should keep the additional memory consumption of this change to a minimum, by only keeping entries in the hash map for items that do have aliases (minor thing).

Suggested change

	if !item.attrs.is_empty() {
	// for better error reporting in doc alias for function
	let aliases = self.parse_doc_aliases(&item.attrs);
	self.r.doc_aliases.insert(local_def_id, aliases);
	}
	if !item.attrs.is_empty()
	// for better error reporting in doc alias for function
	&& let aliases = self.parse_doc_aliases(&item.attrs)
	&& !aliases.is_empty()
	{
	self.r.doc_aliases.insert(local_def_id, aliases);
	}

View changes since the review

[petrochenkov]

Visited during compiler triage. cc @petrochenkov feel free to comment when you have time. This looks a small*-ish* change but let's decide if the win is worth.

I'm still against merging this, and would just close the PR.

[estebank]

@petrochenkov whenever you have a few minutes, please take a look at the discussion above, if you could clarify your position on this PR. Looking at the changes, I am still for landing this. Of the points I raised in my last comment, the one that I am quite concerned about is the lack of consistency in behavior between local and foreign crates, which can lead to people not ever learning about the feature, or expect it to work and being confused why it doesn't trigger (and if we collectively decide not to make it consistent, then we should have a lint catching this on private items).