rustdoc: Go back to loading all external crates unconditionally

[jyn514]

This continues to cause regressions. This code will be unnecessary
once access to the resolver happens fully before creating the tyctxt
(#83761), so load all crates unconditionally for now. To minimize churn, this leaves in the code for loading crates selectively.

"Fixes" #84738. Previously: #83738, #85749, #88215

r? @petrochenkov cc @camelid (this should fix the "index out of bounds" error you had while looking up crate_name).

[camelid]

(this should fix the "index out of bounds" error you had while looking up crate_name).

Just a followup so we don't get confused later on: This PR doesn't fix that ICE; they seem to be different bugs.

[petrochenkov]

@jyn514
The minimized reproduction in #84738 looks like #88679 (comment).
Could you check whether visiting associated items in IntraLinkCrateLoader fixes #84738?

[jyn514]

@petrochenkov given how many regressions this has caused, I don't particularly want to keep playing wack-a-mole with fixing the bugs. I'm fine with fixing the bugs in parallel, but I don't think we should be testing things "in-prod" as it were, we don't need to enable this until rustdoc's fundamental problems with crate loading are resolved (#83761).

[petrochenkov]

given how many regressions this has caused

Which regressions?
I only see #84738 right now, which is caused by the missing visiting of associated items in IntraLinkCrateLoader with 99% certainty.
I'd also add visiting for at least fields and enum variants in the same PR, because the doc link resolver scans them as well. I'm not sure which other nodes need to be visited, I've seen some relevant enum somewhere in rustdoc, but don't remember how it's called.

[jyn514]

Which regressions?

#84738 is the only one I know of, but lots of people have been hitting it, and it was hit the last time we tried to land this patch too.

I'd also add visiting for at least fields and enum variants in the same PR, because the doc link resolver scans them as well. I'm not sure which other nodes need to be visited, I've seen some relevant enum somewhere in rustdoc, but don't remember how it's called.

Right, yes, this is what I mean, there's no guarantee that fixing one issue will fix the others, and it happens rarely enough that usually the bug doesn't get caught until the patch hits stable. And I'm not sure what benefit it brings, no one is using #68427 in practice.

[jyn514]

I just spent about half an hour to come up with the following impls for the visitor:

    fn visit_foreign_item(&mut self, item: &ast::ForeignItem) {
        self.load_links_in_attrs(&item.attrs, item.span);
        visit::walk_foreign_item(self, item)
    }

    // NOTE: if doc-comments are ever allowed on function parameters, this will have to implement `visit_param` too.

    fn visit_assoc_item(&mut self, item: &ast::AssocItem, ctxt: visit::AssocCtxt) {
        self.load_links_in_attrs(&item.attrs, item.span);
        visit::walk_assoc_item(self, item, ctxt)
    }

    fn visit_field_def(&mut self, field: &ast::FieldDef) {
        self.load_links_in_attrs(&field.attrs, field.span);
        visit::walk_field_def(self, field)
    }

    fn visit_variant(&mut self, v: &ast::Variant) {
        self.load_links_in_attrs(&v.attrs, v.span);
        visit::walk_variant(self, v)
    }
}

finding #90610 in the process, and I'm still not sure I got them all. If you really think this is a good idea, I'd suggest adding a visit_attr_list function to Visitor so I don't have to rewrite all of these and be worried I'm missing one. But again, I think this is really high risk for the little benefit we get from it.

[petrochenkov]

I've seen some relevant enum somewhere in rustdoc

It's ItemKind, and Items are constructed from AST by the impl Clean<Item> for ... function.
So the visitor methods above should cover all of the required AST nodes.
These methods are necessary for #83761 anyway, so let's land them now.
(Also could you add test cases for all these new nodes loading crates through doc links?)

The only remaining potential source of missing extern crates that I'm aware of is reexport inlining.
If you can build a test case with a link on an import attempting to access a crate not previously loaded by IntraLinkCrateLoader, then we can land the "load all external crates" hack as well, because I don't want you to implement all that reexport walking now (it should be implemented in #88679, but it may cause perf issues there).

[petrochenkov]

@bors r+

[bors]

📌 Commit 51345a8 has been approved by petrochenkov

[apiraino]

backport suspended, waiting on #90489 (Zulip discussion)

[jyn514]

@apiraino I don't understand your comment - #90489 is this PR. My understanding from the meeting is they were just waiting for this PR to stop changing before approving a backport (which should be the case now; this isn't the sort of change I would expect to pass PR CI but fail bors).

[apiraino]

Beta backport approved as per compiler team on Zulip

(stable backport decision deferred to next week)

@rustbot label +beta-accepted

[bors]

⌛ Testing commit 51345a8 with merge 14a2fd6...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 14a2fd6 to master...

[rust-timer]

Finished benchmarking commit (14a2fd6): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

• Large improvement in instruction counts (up to -3.9% on incr-unchanged builds of deep-vector)
• Large regression in instruction counts (up to 2.8% on full builds of cargo)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[apiraino]

stable backport declined as per compiler team on Zulip (and previous comments)

@rustbot label -stable-nominated