Add #[rustc_edition_redirect]

[Amanieu]

View all comments

This implements the compiler portion of the library API evolution project goal by adding the #[rustc_edition_redirect] attribute. This attribute allows an item path in the standard library to be redirected to a different item when used from a crate with an older edition.

This PR only implements the compiler portion and doesn't make any use of this in the standard library. However I do have a POC branch which replaces the edition-specific panic! dispatching with this.

Example

// std
pub struct Older;
pub struct Old;
pub struct Current;
#[rustc_edition_redirect = "..=2018"]
pub use Older as Current;
#[rustc_edition_redirect = "2021..=2021"]
pub use Old as Current;

// 2024 edition crate
use std::Current; // resolves to Current

// 2021 edition crate
use std::Current; // resolves to Old

// 2018 edition crate
use std::Current; // resolves to Older

Semantics

• #[rustc_edition_redirect = "EDITION_RANGE"] is only allowed on a single-item use.
• The annotated import is resolved and checked like an ordinary import, but does not introduce its alias while compiling the defining crate. Instead, it is attached as an edition-dependent alternative to the ordinary item or re-export with the same name and namespace.
• Every redirect requires a default item with the same name and visibility.
• Multiple redirects may be attached to the same default item, but their edition ranges may not overlap.
• When an external name is looked up, the redirect whose range covers the edition of the lookup span is selected. If none applies, the default item is used.
• The edition comes from the span performing the lookup, so macro-generated identifiers retain the edition of their originating macro.
• Redirect selection applies to explicit paths, ordinary and glob imports, #[macro_use], and prelude lookup.
• Redirects are consumed by the first cross-crate lookup. If another crate re-exports the result, that re-export is fixed according to the edition of the re-exporting crate; redirect metadata is not propagated further.
• Since redirect targets are ordinary re-exports, normal visibility, stability, and import checks apply.

Implementation

1. Attribute parsing records only the edition boundary; the redirect target is the target of the annotated use.
2. Redirect imports participate in normal import resolution, but their aliases are not planted into the defining module.
3. After import resolution reaches its fixed point, redirects are grouped with their default declarations and checked for a missing default, overlapping boundaries, and visibility mismatches.
4. Resolved redirects are stored as part of ModChild in crate metadata.
5. External name-resolution paths use redirect-aware accessors which select a declaration using the edition of the lookup span.

Open questions

• This is currently gated under rustc_attrs without a tracking issue. Does this need a separate tracking issue?
• Do we need a way to automatically propagate redirects through re-exports? Currently this must be done manually when re-exporting through core/alloc/std.
• Are ordinary re-export privacy and stability rules sufficient for legacy redirect targets that should not otherwise appear in the current-edition API?
• This redirect is currently not shown in rustdoc at all. Ideally we would want a note that links to what this resolves to in older editions.

r? petrochenkov

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

[mejrs]

I can review the attribute portion of this.

• Multiple attributes are allowed with different before keys. The oldest one that applies is selected.

Given that it's an internal attribute it's not so important, but another option is a range syntax to force unambiguity (error on overlapping ranges):

#[rustc_edition_redirect(during = "..=2018", target(oldest_module))]
#[rustc_edition_redirect(during = "2021..=2024", target(middle_module))]
pub mod redirected_module { }

View changes since this review

[petrochenkov]

@bors try @rust-timer queue

[petrochenkov]

Given that this is a major expansion to name resolution, it's not surprising that I don't like it :)
My goal here will be making it less major while trying to still satisfy the library goals, perhaps at cost of some reduced ergonomics.

Some initial ideas:

• #[rustc_edition_redirect] is effectively a second reexport system parallel to the regular reexports, it even has to duplicate some reexport checks like "too private to reexport".
  I would prefer the "redirects" to reuse actual reexports, like

  #[rustc_edition_redirect = <edition_info>]
  use RedirectTarget as Name;

  This would be resolved and checked exactly like a regular reexport, except it wouldn't plant Name into the current module to avoid conflicts.
  This will also allow to avoid resolving paths inside built-in attributes, something that was deliberately avoided so far.

• Second, #[rustc_edition_redirect = ...] attributes on a single item should be able to give a precise list of editions they target, in isolation.
  So we don't need to find the full set of items with redirects and the same name and check how their edition ranges overlap to get the edition list.
  Right now name resolution works the same in the local crate and in other crates, this feature as implemented breaks that invariant, but it would be good to keep it, or produce errors in cases where the resolution would works differently.
  #[rustc_edition_redirect = ...] use RedirectTarget as Name; could actually plant the Name into the current module if the rustc_edition_redirect matches the current local edition.

• Third, is it possible to get rid of the "some crates propagate redirects, and some do not" feature, and always either propagate or not? Perhaps at cost of some mild inconveniences in the standard library?
  In any case, right now the choice shouldn't be made by looking at whether feature(edition_redirect) is enabled or not, a missing feature is not supposed to change behavior, only to report a non-fatal error and then continue as usual. Another crate-level attribute gated by rustc_attrs would work instead (#[rustc_preserve_edition_redirects] or something).

I'll need to think about this more in the background for some time.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1b93257 (1b9325785afb0e4413270e6bc497cf74b4bddc70)
Base parent: 922325b (922325bb13bfea5b41454318563f2a65e83c2336)

[rust-timer]

Finished benchmarking commit (1b93257): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.7%]	49
Regressions ❌ (secondary)	0.4%	[0.1%, 1.0%]	50
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	3
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	0.4%	[-0.4%, 0.7%]	52

Max RSS (memory usage)

Results (primary 0.8%, secondary 1.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.5%, 1.3%]	18
Regressions ❌ (secondary)	2.0%	[0.4%, 10.6%]	39
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-4.5%, -0.6%]	4
All ❌✅ (primary)	0.8%	[0.5%, 1.3%]	18

Cycles

Results (primary 0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.5%, 2.4%]	5
Regressions ❌ (secondary)	1.7%	[0.4%, 6.1%]	23
Improvements ✅ (primary)	-1.4%	[-2.3%, -0.7%]	3
Improvements ✅ (secondary)	-4.7%	[-9.2%, -2.0%]	9
All ❌✅ (primary)	0.1%	[-2.3%, 2.4%]	8

Binary size

Results (primary 0.3%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 0.9%]	64
Regressions ❌ (secondary)	0.1%	[0.0%, 0.5%]	30
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.0%, 0.9%]	64

Bootstrap: 490.333s -> 491.013s (0.14%)
Artifact size: 392.58 MiB -> 390.64 MiB (-0.50%)

[Amanieu]

@petrochenkov Can you give more concrete examples of what you are proposing? You seem to be saying that we should just have multiple items with the same name in the same namespace with a different edition filter on them. I think that would be more complex and intrusive than the current solution.

#[rustc_edition_redirect = "2024"]
use RedirectTarget2024 as Name;
#[rustc_edition_redirect = "2021"]
use RedirectTarget2021 as Name;
#[rustc_edition_redirect = "2018"]
use RedirectTarget2018 as Name;

The current system is much simpler: there is only one name per namespace and you can attach any number of edition redirects on that name:

#[rustc_edition_redirect(before = "2024", target(RedirectTarget2024)]
#[rustc_edition_redirect(before = "2021", target(RedirectTarget2021)]
#[rustc_edition_redirect(before = "2018", target(RedirectTarget2018)]
struct Name;

That way all the redirection metadata for one name is available on the single ModChild for that name.

Third, is it possible to get rid of the "some crates propagate redirects, and some do not" feature, and always either propagate or not? Perhaps at cost of some mild inconveniences in the standard library?

In theory yes, we could manually add edition redirect attributes on every re-export of an item that has redirects. But that would just end up being completely equivalent to what the current implementation is doing, while being more error-prone since we may accidentally forget redirects in some places.

• In any case, right now the choice shouldn't be made by looking at whether feature(edition_redirect) is enabled or not, a missing feature is not supposed to change behavior, only to report a non-fatal error and then continue as usual.

There's quite a few places in the compiler where we change global behavior depending on whether a feature is enabled. For example stage_api forces all public items to have stability attributes. I feel that what I am doing is not too different, and in any case this feature is only intended for use in the standard library and tests.

[Amanieu]

cc @rust-lang/edition for awareness.

[petrochenkov]

audit every single code path that ends up calling non_glob_decl. This exposed some issues that are addressed in the latest commit

Yeah, I only did bare minimum for the test suite to pass, but anything not correctly updated will result in a panic rather than a silent bug, so it's not a big problem.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Amanieu]

I collapsed the history, it was getting difficult to rebase. The 2 newest commits are the changes since @petrochenkov's last review. All comments have been addressed.

[petrochenkov]

Could potentially use the standard RangeInclusive here and in places like MetadataEditionRedirect.

View changes since the review

[petrochenkov]

LGTM now, although the PR description needs to be updated.

This also probably needs some formal approval from the lang team.