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

$crate -> $ident:ident -> identity_proc_macro!($ident) panics in the proc macro #101211

Closed
rodrimati1992 opened this issue Aug 30, 2022 · 8 comments · Fixed by #101677
Closed
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-proc-macros Area: Procedural macros C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. regression-from-stable-to-stable Performance or correctness regression from one stable version to another.

Comments

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Aug 30, 2022

Code

I tried cargo test-ing this proc macro crate:

/// ```rust
/// pub extern crate proc_macro_crate;
/// 
/// #[macro_export]
/// macro_rules! foo {
///     ($($path:ident)::*) => (
///         $crate::proc_macro_crate::identity!{
///             $($path)::*
///         }
///     )
/// }
/// 
/// #[macro_export]
/// macro_rules! baz {
///     () => (
///         $crate::foo!($crate::BAR)
///     )
/// }
/// 
/// pub const BAR: u32 = 19;
/// 
/// fn main(){ 
///     println!("{}", crate::baz!());
/// }
/// 
/// ```
#[proc_macro]
pub fn identity(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
    input.into_iter().collect()
}

I expected to see this happen: The test completes successfully

Instead, this happened:
The test fails compilation with this error:

error: proc macro panicked
  --> src/lib.rs:7:9
   |
7  | /         $crate::proc_macro_crate::identity!{
8  | |             $($path)::*
9  | |         }
   | |_________^
...
23 |       println!("{}", crate::baz!());
   |                      ------------- in this macro invocation
   |
   = help: message: `"$crate"` is not a valid identifier
   = note: this error originates in the macro `$crate::foo` (in Nightly builds, run with -Z macro-backtrace for more info)

Version it worked on

It most recently worked on:

rustc 1.60.0 (7737e0b5c 2022-04-04)
binary: rustc
commit-hash: 7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c
commit-date: 2022-04-04
host: x86_64-unknown-linux-gnu
release: 1.60.0
LLVM version: 14.0.0

Version with regression

rustc --version --verbose:

rustc 1.61.0 (fe5b13d68 2022-05-18)
binary: rustc
commit-hash: fe5b13d681f25ee6474be29d748c65adcd91f69e
commit-date: 2022-05-18
host: x86_64-unknown-linux-gnu
release: 1.61.0
LLVM version: 14.0.0

This also fails in 1.62 and 1.63.

Meta

This appears to be fixed in the beta channel, but I could not find a mention of this bug or a related one.

It's entirely possible that the bug will trigger again in future versions if this code isn't used in tests.

@rodrimati1992 rodrimati1992 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 30, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 30, 2022
@jyn514 jyn514 added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Aug 30, 2022
@steffahn
Copy link
Member

steffahn commented Aug 30, 2022

Bisection shows it regressed in #92472 and got fixed in #98189. Looks like this issue wasn’t deliberately fixed, since #98189 doesn’t call out any issue being fixed. Also #92472 did involve minor breakage, as discussed in its thread, but I haven’t looked into it enough to tell whether those cases of breakage were similar to this issue or different.

@steffahn
Copy link
Member

@rustbot label A-macros, A-proc-macros

@rustbot rustbot added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-proc-macros Area: Procedural macros labels Aug 30, 2022
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 31, 2022
@steffahn
Copy link
Member

steffahn commented Aug 31, 2022

cc @petrochenkov (author of #92472) and @mystor (author of #98189)

(read #101211 (comment) above to see how these PRs are relevant)

@mystor
Copy link
Contributor

mystor commented Aug 31, 2022

Looking at the code changes from #92472 it makes sense that this regressed, as the call to Ident::new (https://github.com/rust-lang/rust/pull/92472/files#diff-002b2429c67f4267af1485b8b9e2f95cbfe586971ed97f832da4d4d45d0e1e41R179) would have done a check and rejected creating $crate. The code (which was originally introduced in #73084: https://github.com/rust-lang/rust/pull/73084/files#diff-75938b59f6567a18bc15d178cf794cbb3c7e2c846b2afcdfd56110824c70ed07R179), should've probably avoided the Ident::new constructor from the get-go, but it wasn't an issue because $crate wasn't a possible identifier in that case previously.

In #98189, I changed the logic there to avoid going through that constructor when re-building the tokens internally, so we don't do the broken validation on that side anymore, and the problem wouldn't happen anymore.

I don't think there's much more to be done here, as it's fixed already on beta, and isn't likely to regress with the new design.

@steffahn
Copy link
Member

I don't think there's much more to be done here, as it's fixed already on beta, and isn't likely to regress with the new design.

Thanks a lot for your insights!

@steffahn
Copy link
Member

@rustbot label E-easy. (We only need a test to close this issue.)

@rustbot rustbot added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Aug 31, 2022
@mystor
Copy link
Contributor

mystor commented Aug 31, 2022

For someone wanting to add a test for this, you should be able to add the use code in src/test/ui/proc-macro, and the proc-macro in src/test/ui/proc-macro/auxiliary.

You can see https://github.com/rust-lang/rust/blob/master/src/test/ui/proc-macro/dollar-crate-issue-62325.rs for an idea of what the test might look like.

rodrimati1992 added a commit to rodrimati1992/core_extensions that referenced this issue Sep 2, 2022
The bug is described in rust-lang/rust#101211
it affects Rust 1.61, 1.62, 1.63
rodrimati1992 added a commit to rodrimati1992/core_extensions that referenced this issue Sep 2, 2022
The test check that the macros are correct, and ensure that they
don't trigger rust-lang/rust#101211

Changed quasiconst docs to say that defaulted const parameters are supported.
@oMisaki
Copy link
Contributor

oMisaki commented Sep 10, 2022

@rustbot claim

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 12, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2022
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#100185 (Fix `ReErased` leaking into typeck due to `typeof(...)` recovery)
 - rust-lang#100291 (constify some `CStr` methods)
 - rust-lang#101677 (Add test for rust-lang#101211)
 - rust-lang#101723 (Impove diagnostic for `.await`ing non-futures)
 - rust-lang#101724 (Allow unauthenticated users to add the `const-hack` label)
 - rust-lang#101731 (rustdoc: improve rustdoc HTML suggestions handling of nested generics)
 - rust-lang#101732 (Feature gate the `rustdoc::missing_doc_code_examples` lint)
 - rust-lang#101735 (rustdoc: fix treatment of backslash-escaped HTML)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors closed this as completed in 5727331 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-proc-macros Area: Procedural macros C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. regression-from-stable-to-stable Performance or correctness regression from one stable version to another.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants