Fix extern prelude failure in rustdoc

[GuillaumeGomez]

Fixes #50561.

r? @QuietMisdreavus

[QuietMisdreavus]

This sounds like it could be a bug in the way this error is handled, not necessarily something that we would need to hack around. (And if we do need to patch it specifically for rustdoc, i would prefer using the actually_rustdoc flag instead of adding a new parameter.)

@Manishearth What do you know about the extern_prelude feature? The intra-links seem to have an awkward interaction with it.

[Manishearth]

Well, actually_rustdoc doesn't handle the difference between resolution that rustdoc has to do for compilation vs resolution for intra doc links.

[QuietMisdreavus]

Hmm, that's a good point, since this PR currently only enables the setting after the rest of the crate has passed resolution.

Still, i'm wondering whether there's a "proper" fix that's not "disable this check during intra-link resolution". What's actually going on here?

[Manishearth]

Actually the proper fix is make it such that we hit that call with record_used=true, since record_used is also "is this a speculative resolution" which is true in this case. Perhaps rename the parameter while you're at it.

Elsewhere I've proposed a more coherent abstraction for paths that also includes speculation but it's currently stalled on a decision from compiler folks think.

[bors]

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

[pietroalbini]

Ping from triage @QuietMisdreavus! This PR needs your review.

[QuietMisdreavus]

I think we still needed to discuss what it would take to get to the solution @Manishearth mentioned. I'm not a fan of adding a new flag just to toss it into an existing if-clause somewhere.

[GuillaumeGomez]

Me neither so an alternative solution would come in handy.

[TimNN]

@QuietMisdreavus / @GuillaumeGomez: How do you plan to move forward with this PR?

[GuillaumeGomez]

We haven't talked about it since then (rustfest). We will come back to it.

[Mark-Simulacrum]

For when this is ready to go, @bors p=1

We should also make sure to backport this if necessary -- I've put the related issue on the tracking milestone (edit: to be clear, for now it's not necessary, but this may slip into beta).

[GuillaumeGomez]

Should we merge this meanwhile writing a better version?

[QuietMisdreavus]

Reading over this again, i'm willing to have the temp flag (even though i'm concerned it may not stay temporary). However, the location of the check should probably be moved. Right now, what this is doing is universally enabling the extern_prelude feature for every user of type-links, whether or not they actually intended for it, by bypassing the location where the error would have been emitted. Do we want this? It seems to me like the existing feature check is kinda late in the flow.

In fact, reading this over again, it looks like we can bypass the need for the new flag by moving the feature-flag check. Would it be possible to reconfigure this so that the block with self.extern_prelude.contains(&ident.name) doesn't get executed at all if the feature flag is absent? Or use that inner check to set some indicator and emit the error afterward? We can use the ignore_extern_prelude_feature check to silence the error then.

I've changed my mind a few times while writing this, but basically i want to make sure we don't eagerly use the extern_prelude feature if the crate doesn't have the feature flag set. Right now it will use it regardless.

[petrochenkov]

Temporary flag to avoid a feature gate error is okay, but I'd like to avoid this:

don't eagerly use the extern_prelude feature if the crate doesn't have the feature flag set.

We did the breaking change once in #49789 making sure it caused no regressions and now people can't write code that would conflict with extern prelude when it's stabilized.
If extern prelude is completely ignored if feature(extern_prelude) is not specified, then we are opening one more time window for people to write such code that will be broken later.

[QuietMisdreavus]

@petrochenkov I'm not sure what you're suggesting. Are you wanting to keep this PR as-is, or make it do something different?

[petrochenkov]

"As is" LGTM.

[QuietMisdreavus]

Works for me! I was worried about exposing the feature more widely than it otherwise would have been, but if you're worried about breaking more code when it's fully stabilized, i won't hold it up. Thanks for commenting!

@bors r+

[bors]

📌 Commit 27db90a has been approved by QuietMisdreavus

[bors]

🔒 Merge conflict

[GuillaumeGomez]

Rebased (and no conflicts of course...).

@bors: r=QuietMisdreavus

[bors]

📌 Commit dadfa13 has been approved by QuietMisdreavus

[bors]

⌛ Testing commit dadfa13 with merge f9944fd...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing f9944fd to master...