Add field@ and variant@ doc-link disambiguators

[coolreader18]

I'm not sure if this is big enough to need an fcp or not, but this is something I found missing when trying to refer to a field in macro-generated docs, not knowing if a method might be defined as well. Obviously, there are definitely other uses.

In the case where it's not disambiguated, methods (and I suppose other associated items in the value namespace) still take priority, which @jyn514 said was an oversight but I think is probably the desired behavior 99% of the time anyway - shadowing a field with an accessor method is a very common pattern. If fields and methods with the same name started conflicting, it would be a breaking change. Though, to quote them:

jyn: maybe you can break this only if both [the method and the field] are public
jyn: rustc has some future-incompat warning level
jyn: that gets through -A warnings and --cap-lints from cargo

That'd be out of scope of this PR, though.

Fixes #80283

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
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

[jyn514]

cc #80283

[jyn514]

i'm confused, i thought you were going to remove this? is the problem that DefKind::ns is defined in rustc so you can't modify it?

[coolreader18]

I was, but it turns out it didn't make sense to move the disambiguator == Field so far up and out - that would mean putting it in resolve_disambiguator(), while it makes much more sense (and is easier to keep existing field-last resolution behavior) if it just gets considered to be in the value ns and fails to resolve to anything before getting picked up in the special-case in resolve_associated_item().

[jyn514]

jyn: maybe you can break this only if both [the method and the field] are public

this would also need to consider the story around --document-private-items; in general that flag is kinda broken for intra-doc links because rustc_resolve won't let you see private items outside the module you're resolving from under any circumstance, but maybe there's some compromise that will work 80% of the time.

[notriddle]

In general, this PR's feature design seems fine. If it passes a crater run, I wouldn't mind making it insta-stable.

In the case where it's not disambiguated, methods (and I suppose other associated items in the value namespace) still take priority

Thanks to this choice, I don't think this should break anything, and I don't think it makes the --document-private-items situation any worse than it already is.

[jyn514]

oh tbc this pr doesn't break anything, i would be shocked if crater turned up anything, @ isn't valid outside disambiguators. i was just talking about if you want to error when methods and fields need to be disambiguated

[coolreader18]

Ah, oops, blessed the ui tests before finishing the implementation lol

[coolreader18]

ah wait nevermind. i just broke it forgot to bless other ui tests

[notriddle]

i was just talking about if you want to error when methods and fields need to be disambiguated

Rustdoc already has field-last resolution behavior. This PR doesn't make that one any worse than it already is, either.

If we want to make the disambiguator required later, this PR doesn't seem to prevent that.

[coolreader18]

Bump on this review?

[notriddle]

Good catch. I wanted to know if anyone else objected, and it doesn't seem they do.

@bors r=notriddle,jyn514

[bors]

📌 Commit e91e015 has been approved by notriddle,jyn514

It is now in the queue for this repository.

[bors]

⌛ Testing commit e91e015 with merge f79ef02...

[bors]

☀️ Test successful - checks-actions
Approved by: notriddle,jyn514
Pushing f79ef02 to master...

[rust-timer]

Finished benchmarking commit (f79ef02): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.5%, secondary -3.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-3.1%	[-3.4%, -2.8%]	2
All ❌✅ (primary)	-1.5%	[-1.5%, -1.5%]	1

Cycles

Results (secondary 3.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.8%	[3.8%, 3.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.872s -> 771.378s (0.07%)
Artifact size: 341.44 MiB -> 341.41 MiB (-0.01%)