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

Clap derive expansion no longer works on stable #79503

Closed
pksunkara opened this issue Nov 28, 2020 · 12 comments
Closed

Clap derive expansion no longer works on stable #79503

pksunkara opened this issue Nov 28, 2020 · 12 comments
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pksunkara
Copy link
Contributor

pksunkara commented Nov 28, 2020

Code

Clap has the following test at clap_derive/tests/nested.rs:

use clap::Clap;

#[test]
fn use_option() {
    macro_rules! expand_ty {
        ($name:ident: $ty:ty) => {
            #[derive(Clap)]
            struct Outer {
                #[clap(short, long)]
                #[allow(dead_code)]
                $name: $ty,
            }
        };
    }

    expand_ty!(my_field: Option<String>);
}

As you can see from the latest master CI, it fails to compile on stable

Version it worked on

It most recently worked on: Successfully compiles and runs on 1.46.0

Version with regression

rustc --version --verbose:

rustc 1.48.0 (7eac88abb 2020-11-16)
@pksunkara
Copy link
Contributor Author

pksunkara commented Nov 28, 2020

Weirdly, when I use my mac to test this locally, it works.

→ rustup show active-toolchain
stable-x86_64-apple-darwin (default)
→ cargo test -p clap_derive --test nested --features "wrap_help yaml regex"
   Compiling ....
    Finished test [unoptimized + debuginfo] target(s) in 55.55s
     Running target/debug/deps/nested-55411ec307e7b9f1

running 1 test
test use_option ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

NOTE: macos stable fails on CI, so I don't think this is an OS issue.

On EC2, it fails with the above error from the CI:

→ rustup show active-toolchain
stable-x86_64-unknown-linux-gnu (default)
→ cargo test -p clap_derive --test nested --features "wrap_help yaml regex"
error[E0425]: cannot find value `arg_matches` in this scope
Error:   --> clap_derive/tests/nested.rs:32:26
   |
32 |     expand_ty!(my_field: Option<String>);
   |                          ^^^^^^ not found in this scope

@jyn514 jyn514 added E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 28, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 28, 2020
@pksunkara
Copy link
Contributor Author

pksunkara commented Nov 28, 2020

Pinging @petrochenkov because I suspect this is an issue with macro expansion. I tried cargo expand on the EC2 instance to check the output, but the code it had generated had no issues at all and successfully compiled.

Which is also the reason I couldn't generate a minimal reproducible example. I have no idea what's causing this.

@jyn514 jyn514 added the A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) label Nov 28, 2020
@LeSeulArtichaut LeSeulArtichaut added the A-resolve Area: Name resolution label Nov 28, 2020
@jyn514 jyn514 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 Nov 28, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 28, 2020

This works on 1.47 and breaks on 1.48.

@jyn514 jyn514 added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Nov 28, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 28, 2020

searched nightlies: from nightly-2020-07-11 to nightly-2020-11-28
regressed nightly: nightly-2020-09-12
searched commits: from a1947b3 to 9911160
regressed commit: 94b4de0

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc -v --script ./test.sh --preserve --start 2020-07-11 

where test.sh is

#!/bin/sh
cargo test -p clap_derive --test nested

@jyn514
Copy link
Member

jyn514 commented Nov 28, 2020

cc @Aaron1011 - #75800 broke clap.

@Aaron1011
Copy link
Member

This is due to a bug in how clap uses quote_spanned!. When #75800 was merged, we started preserving hygiene information in more cases, exposing the bug,

I've opened clap-rs/clap#2231 to fix the issue

@Aaron1011
Copy link
Member

@pksunkara: Do you know if this test was failing on nightlies prior to the stable release that broke this?

@jyn514
Copy link
Member

jyn514 commented Nov 28, 2020

@Aaron1011 no, it was broken in #75800: #79503 (comment). That searched nightlies, not stable releases.

@Aaron1011
Copy link
Member

I mean, did the Clap CI tests start failing on nightly after that rust PR was merged?

@pksunkara
Copy link
Contributor Author

@pksunkara
Copy link
Contributor Author

To see a passing nightly CI run, check this, https://github.com/clap-rs/clap/runs/1393825395

@pksunkara
Copy link
Contributor Author

pksunkara commented Nov 28, 2020

I am closing this since the regression is not one. It looks like the original PR did a crater run, but the bugged code was in a test file. Thanks for help @Aaron1011

I am not sure why one of the nightlies wasn't failing, but that's a separate issue altogether.

@jyn514 jyn514 removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 28, 2021
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-resolve Area: Name resolution E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants