Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove NtIdent hack for regressed crates #74616

Open
Aaron1011 opened this issue Jul 22, 2020 · 5 comments
Open

Remove NtIdent hack for regressed crates #74616

Aaron1011 opened this issue Jul 22, 2020 · 5 comments
Labels
A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros -s A-proc-macros Area: Procedural macros C-future-incompatibility Category: Future-incompatibility lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Jul 22, 2020

What is this issue?

If you're a crate author who's been linked here, this issue tracks removing a backwards-compatibility hack in Rust.

Rust has a longstanding issue #43081, which causes procedural macros to lose location and hygiene information (known as a "Span") under certain circumstances. Recently, pull request #73084 was merged, which makes progress towards resolving #43081.

Unfortunately, older versions of certain procedural macro crates (such as proc-macro-hack v0.5.15 and js-sys v0.3.39) cannot handle the changes in input caused by the Rust bugfix. To allow these crates to continue to compile, a backward-compatibility hack was added to adjust the input passed to proc-macro-hack and js-sys specifically.

Eventually, we would like to remove this backwards-compatibility hack, since the compiler should not have hard-coded exceptions for certain crates. However, removing this hack will break any crates that depend on affected versions of proc-macro-hack or js-sys.

To ensure that your crate continues to work, you'll want to ensure that your Cargo.lock references proc-macro-hack v0.5.16 or above, and js-sys v0.3.40 or above. This can be done by running cargo update -p proc-macro-hack and cargo update -p js-sys. If you maintain a library crate (without a Cargo.lock, no action is needed on your part).

Internal compiler details

In #73084 (comment), I added a hack to change the behavior of NtIdents passed to certain proc-macros. This was done by special-casing certain identifiers, and should be eventually be removed in favor of a proper solution.

If we decide to always wrap single identifiers in None-delimited groups, then we will need to wait until enough of the ecosystem has bumped proc-macro-hack and wasm-bindgen, to avoid breaking a large number of crates.

Crater run: https://crater-reports.s3.amazonaws.com/pr-73084-1/index.html
Triage: https://hackmd.io/O7icbSylRP6uVZyAQ9EDeA

@Aaron1011 Aaron1011 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-proc-macros Area: Procedural macros labels Jul 22, 2020
@JohnTitor JohnTitor added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 22, 2020
@Mark-Simulacrum Mark-Simulacrum added C-future-incompatibility Category: Future-incompatibility lints and removed C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Jul 22, 2020
@petrochenkov
Copy link
Contributor

Tests that need to be removed together with the hack are in src/test/ui/proc-macro/group-compat-hack.

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Aug 22, 2020
@Aaron1011
Copy link
Member Author

The hack is here:

// See issue #74616 for details
pub fn ident_name_compatibility_hack(
&self,
orig_span: Span,
source_map: &SourceMap,
) -> Option<(Ident, bool)> {
if let NtIdent(ident, is_raw) = self {
if let ExpnKind::Macro(_, macro_name) = orig_span.ctxt().outer_expn_data().kind {
let filename = source_map.span_to_filename(orig_span);
if let FileName::Real(RealFileName::Named(path)) = filename {
if (path.ends_with("time-macros-impl/src/lib.rs")
&& macro_name == sym::impl_macros)
|| (path.ends_with("js-sys/src/lib.rs") && macro_name == sym::arrays)
{
let snippet = source_map.span_to_snippet(orig_span);
if snippet.as_deref() == Ok("$name") {
return Some((*ident, *is_raw));
}
}
}
}
}
None
}
}

Aaron1011 added a commit to Aaron1011/acme-redirect that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/chlue that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/crowbar that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/guzuta that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/http-serve that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/jack-mixer that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/k8s-gcr-auth-helper that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/lorikeet that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/lorikeet-dash that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/zuse that referenced this issue Aug 24, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/rust that referenced this issue Aug 24, 2020
Fixes rust-lang#74616

There are still a large number of outstanding regressions, so this PR
can't be merged yet. I'm opening it to generate a newer 'try' build
for crater authors to test with.

Eventually, we'll want to do a crater run for this PR.
Aaron1011 added a commit to Aaron1011/s3walk that referenced this issue Aug 26, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
moritzheiber pushed a commit to moritzheiber/crowbar that referenced this issue Aug 27, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
AnderEnder pushed a commit to AnderEnder/s3walk that referenced this issue Aug 30, 2020
Your crate currently depends on an older version of proc-macro-hack
which is buggy, and will stop compiling in a future release of Rust.
See rust-lang/rust#74616 for more details.
Aaron1011 added a commit to Aaron1011/rust that referenced this issue Sep 4, 2020
Issue rust-lang#74616 tracks a backwards-compatibility hack for certain macros.
This has is implemented by hard-coding the filenames and macro names of
certain code that we want to continue to compile.

However, the initial implementation of the hack was based on the
directory structure when building the crate from its repository (e.g.
`js-sys/src/lib.rs`). When the crate is build as a dependency, it will
include a version number from the clone from the cargo registry (e.g.
`js-sys-0.3.17/src/lib.rs`), which would fail the check.

This commit modifies the backwards-compatibility hack to check that
desired crate name (`js-sys` or `time-macros-impl`) is a prefix of the
proper part of the path.

See rust-lang#76070 (comment)
for more details.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 5, 2020
…t, r=petrochenkov

Account for version number in NtIdent hack

Issue rust-lang#74616 tracks a backwards-compatibility hack for certain macros.
This has is implemented by hard-coding the filenames and macro names of
certain code that we want to continue to compile.

However, the initial implementation of the hack was based on the
directory structure when building the crate from its repository (e.g.
`js-sys/src/lib.rs`). When the crate is build as a dependency, it will
include a version number from the clone from the cargo registry (e.g.
`js-sys-0.3.17/src/lib.rs`), which would fail the check.

This commit modifies the backwards-compatibility hack to check that
desired crate name (`js-sys` or `time-macros-impl`) is a prefix of the
proper part of the path.

See rust-lang#76070 (comment)
for more details.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 5, 2020
…t, r=petrochenkov

Account for version number in NtIdent hack

Issue rust-lang#74616 tracks a backwards-compatibility hack for certain macros.
This has is implemented by hard-coding the filenames and macro names of
certain code that we want to continue to compile.

However, the initial implementation of the hack was based on the
directory structure when building the crate from its repository (e.g.
`js-sys/src/lib.rs`). When the crate is build as a dependency, it will
include a version number from the clone from the cargo registry (e.g.
`js-sys-0.3.17/src/lib.rs`), which would fail the check.

This commit modifies the backwards-compatibility hack to check that
desired crate name (`js-sys` or `time-macros-impl`) is a prefix of the
proper part of the path.

See rust-lang#76070 (comment)
for more details.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 6, 2020
… r=petrochenkov

Account for version number in NtIdent hack

Issue rust-lang#74616 tracks a backwards-compatibility hack for certain macros.
This has is implemented by hard-coding the filenames and macro names of
certain code that we want to continue to compile.

However, the initial implementation of the hack was based on the
directory structure when building the crate from its repository (e.g.
`js-sys/src/lib.rs`). When the crate is build as a dependency, it will
include a version number from the clone from the cargo registry (e.g.
`js-sys-0.3.17/src/lib.rs`), which would fail the check.

This commit modifies the backwards-compatibility hack to check that
desired crate name (`js-sys` or `time-macros-impl`) is a prefix of the
proper part of the path.

See rust-lang#76070 (comment)
for more details.
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Sep 17, 2020
Issue rust-lang#74616 tracks a backwards-compatibility hack for certain macros.
This has is implemented by hard-coding the filenames and macro names of
certain code that we want to continue to compile.

However, the initial implementation of the hack was based on the
directory structure when building the crate from its repository (e.g.
`js-sys/src/lib.rs`). When the crate is build as a dependency, it will
include a version number from the clone from the cargo registry (e.g.
`js-sys-0.3.17/src/lib.rs`), which would fail the check.

This commit modifies the backwards-compatibility hack to check that
desired crate name (`js-sys` or `time-macros-impl`) is a prefix of the
proper part of the path.

See rust-lang#76070 (comment)
for more details.
@bors bors closed this as completed in ea468f4 Oct 14, 2020
@Aaron1011
Copy link
Member Author

I don't know why I thought that my PR fixed this issue...

@Aaron1011 Aaron1011 reopened this Oct 14, 2020
@bmisiak
Copy link
Contributor

bmisiak commented Oct 20, 2020

To ensure that your crate continues to work, you'll want to ensure that your Cargo.lock references proc-macro-hack v0.5.16 or above, and js-sys v0.3.40 or above. This can be done by running cargo update -p proc-macro-hack and cargo update -p js-sys. If you maintain a library crates (without a Cargo.lock, no action is needed on your part).

Should we communicate this to users when compiling code with older versions? Give them a special deprecation warning perhaps and a hint like this, to speed things up?

@Aaron1011
Copy link
Member Author

PR #75534 will give us the first part of the needed infrastructure to do this. Right now, our only option would be to ignore cap-lints and emit the full warning for upstream dependencies, which would break anyone who denys warnings.

@Aaron1011 Aaron1011 added the A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros -s label Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macro-back-compat Area: Backwards compatibility hacks for proc macros -s A-proc-macros Area: Procedural macros C-future-incompatibility Category: Future-incompatibility lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants