Remove effect of #[no_link] attribute on name resolution

[petrochenkov]

Previously it hid all non-macro names from other crates.
This has no relation to linking and can change name resolution behavior in some cases (e.g. glob conflicts), in addition to just producing the "unresolved name" errors.

I can kind of understand the possible reasoning behind the current behavior - if you can use names from a no_link crates then you can use, for example, functions too, but whether it will actually work or produce link-time errors will depend on random factors like inliner behavior.
(^^^ This is not the actual reason why the current behavior exist, I've looked through git history and it's mostly accidental.)

I think this risk is ok for such an obscure attribute, and we don't need to specifically prevent use of non-macro items from such crates.
(I'm not actually sure why would anyone use #[no_link] on a crate, even if it's macro only, if you aware of any use cases, please share. IIRC, at some point it was used for crates implementing custom derives - the now removed legacy ones, not the current proc macros.)

Extracted from #91795.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

The macros_only flag is only set for proc macro crates (processed above), and for #[no_link] crates.
Maybe it should be renamed, but I don't want to do it in a single PR with observable language changes.

[Mark-Simulacrum]

I agree that this seems like a generally good change. It feels like we should deprecate no_link, perhaps with a future-compat warning and/or removal planned in the next edition. Searching on github it looks like most usage is related to macro crates, but I suspect likely not in a way that is actually necessary -- if we do try to deprecate/remove, it might make sense to try and more intentionally test these.

I have a mild sense of the pre-existing behavior being better in the sense that it prevents linker errors (which are generally bad), but it seems like we should be able to improve diagnostics there if there's an actual use case (through changes in mono item collection, I suppose). But given the general lack of need for something like this it doesn't seem like a good end-goal to pursue.

@petrochenkov to make sure I'm parsing the issue description right: it sounds like the "breaking" change here is enabling the use of non-macro names changing glob imports to potentially now conflict/resolve to different items?

Nominating for T-lang to look at in a meeting, though it feels like FCP might be a bit excessive (given the rather low likelihood of any potential cases where this changes behavior on old code).

[petrochenkov]

@Mark-Simulacrum

it sounds like the "breaking" change here is enabling the use of non-macro names changing glob imports to potentially now conflict/resolve to different items?

Yes, the changing case may look like this:

extern crate link_crate; // also has function `foo` in the root
#[no_link] extern crate no_link_crate; // has function `foo` in the root

use link_crate::*; // imports function `foo`
use nolink_crate::*; // before this PR: doesn't import function `foo`
                     // after this PR: imports function `foo` - conflict if `foo` is used

fn main() { foo(); } // `foo` is used

This is equivalent to adding new items to any library crate, which is generally not considered a semver-breaking change.

[nikomatsakis]

This seems like a good change. If we are concerned about linker errors, I'd prefer we detect and enforce those after the fact rather than having name resolution not have the names in the first place.

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting, and agreed that we want to make this change and simplify name resolution.

Also, if people regularly use this attribute, we should consider a version that attaches to use rather than extern crate.

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[joshtriplett]

@bors r+

[bors]

📌 Commit 54cd824 has been approved by joshtriplett

[bors]

⌛ Testing commit 54cd824 with merge f7934f6...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing f7934f6 to master...

[rust-timer]

Finished benchmarking commit (f7934f6): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression