rustdoc: Cleanup handling of associated items for intra-doc links #83849

jyn514 · 2021-04-04T14:15:19Z

Helps with #83761 (right now the uses of the resolver are all intermingled with uses of the tyctxt). Best reviewed one commit at a time.

r? @bugadani maybe? Feel free to reassign :)

bugadani

I'm not quite sure how the cleanup helps with the particular issue, but it looks good overall - except for the failing test. I have some questions, though.

bugadani · 2021-04-04T17:33:43Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -534,29 +531,39 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                }
            })?;

-        // FIXME: are these both necessary?
-        let ty_res = if let Some(ty_res) = resolve_primitive(&path_root, TypeNS)


I guess the error shows why this resolve_primitive might be necessary?

I'm bisecting this now - I think it's unlikely this is the cause, but it's possible.

Oh! I figured it out - resolve_path gives precedence to modules over primitives, this does the reverse.

| ^^^^^^^^^ no item named `MAX` in module `usize`

The issue is that both mod usize and the primitive usize are in scope. I think this might actually be a bug even before - rustdoc should give an error if the base res is ambiguous, but it doesn't:

$ cat primitive-ambiguous.rs //! [usize::MAX] mod usize { const MAX: usize = 1; } $ rustdoc primitive-ambiguous.rs $

For this specific test case, I think we could fix it by not giving an ambiguity error if the module has doc(primitive) on it?

I don't think I'm qualified to have a say in this :)

cc @Manishearth - do you have an opinion here? For context, the original test case was

//! [usize::MAX] #[doc(primitive = "usize")] mod usize {}

Actually I think I'm going to wait to fix this for a follow-up PR, it's a change in behavior and shouldn't be mixed with the cleanups. Opened #83862.

src/librustdoc/passes/collect_intra_doc_links.rs

mentioning rust-lang#83849 (comment)

bugadani · 2021-04-05T08:22:30Z

I'm not sure the third commit message ("Greatly simplify") applies at this point :) Otherwise LGTM.

jyn514 · 2021-04-05T08:27:03Z

@bors r=bugadani

bors · 2021-04-05T08:27:04Z

📌 Commit 3611a64 has been approved by bugadani

jyn514 · 2021-04-05T08:29:19Z

I'm not quite sure how the cleanup helps with the particular issue

Oh sorry I forgot to answer this: the early collector won't be able to use a TyCtxt at all. By separating this into "functions that use TyCtxt" and "functions that use resolver", it will be a lot easier to just cut and paste them to a new file.

rustdoc: Cleanup handling of associated items for intra-doc links Helps with rust-lang#83761 (right now the uses of the resolver are all intermingled with uses of the tyctxt). Best reviewed one commit at a time. r? `@bugadani` maybe? Feel free to reassign :)

rustdoc: Cleanup handling of associated items for intra-doc links Helps with rust-lang#83761 (right now the uses of the resolver are all intermingled with uses of the tyctxt). Best reviewed one commit at a time. r? ``@bugadani`` maybe? Feel free to reassign :)

rustdoc: Cleanup handling of associated items for intra-doc links Helps with rust-lang#83761 (right now the uses of the resolver are all intermingled with uses of the tyctxt). Best reviewed one commit at a time. r? ```@bugadani``` maybe? Feel free to reassign :)

Dylan-DPC-zz · 2021-04-05T10:59:40Z

failed in rollup

@bors r-

jyn514 · 2021-04-05T12:21:48Z

The warning is correct, ModuleData is public and the kind field is private. It must not have been caught before because kind_side_channel wasn't set. I'll ignore the lint and add a test.

Previously, the types looked like this: - None means this is not an associated item (but may be a variant field) - Some(Err) means this is known to be an error. I think the only way that can happen is if it resolved and but you had your own anchor. - Some(Ok(_, None)) was impossible. Now, this returns a nested Option and does the error handling and fiddling with the side channel in the caller. As a side-effect, it also removes duplicate error handling. This has one small change in behavior, which is that `resolve_primitive_associated_item` now goes through `variant_field` if it fails to resolve something. This is not ideal, but since it will be quickly rejected anyway, I think the performance hit is worth the cleanup. This also fixes a bug where struct fields would forget to set the side channel, adds a test for the bug, and ignores `private_intra_doc_links` in rustc_resolve (since it's always documented with --document-private-items).

jyn514 · 2021-04-05T12:35:51Z

@bors r=bugadani

bors · 2021-04-05T12:35:52Z

📌 Commit 3b7e654 has been approved by bugadani

bors · 2021-04-05T13:47:42Z

⌛ Testing commit 3b7e654 with merge 3313e25dc6b7d8ae459b6340818d62b46c6da2c4...

jyn514 · 2021-04-05T13:50:12Z

@bors retry yield to #83890 (in particular #83878 fixes spurious failures)

rust-log-analyzer · 2021-04-05T13:55:17Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

rustdoc: Cleanup handling of associated items for intra-doc links Helps with rust-lang#83761 (right now the uses of the resolver are all intermingled with uses of the tyctxt). Best reviewed one commit at a time. r? `@bugadani` maybe? Feel free to reassign :)

rustdoc: Cleanup handling of associated items for intra-doc links Helps with rust-lang#83761 (right now the uses of the resolver are all intermingled with uses of the tyctxt). Best reviewed one commit at a time. r? ``@bugadani`` maybe? Feel free to reassign :)

Rollup of 8 pull requests Successful merges: - rust-lang#83370 (Add `x.py setup tools` which enables `download-rustc` by default) - rust-lang#83489 (Properly suggest deref in else block) - rust-lang#83734 (Catch a bad placeholder type error for statics in `extern`s) - rust-lang#83814 (expand: Do not ICE when a legacy AST-based macro attribute produces and empty expression) - rust-lang#83835 (rustdoc: sort search index items for compression) - rust-lang#83849 (rustdoc: Cleanup handling of associated items for intra-doc links) - rust-lang#83881 (:arrow_up: rust-analyzer) - rust-lang#83885 (Document compiler/ with -Aprivate-intra-doc-links) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

jyn514 force-pushed the intra-doc-cleanup branch from c561013 to 2f0f22b Compare April 4, 2021 14:31