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

matches macro conflicts with matches crate #66518

Closed
Mark-Simulacrum opened this issue Nov 18, 2019 · 8 comments
Closed

matches macro conflicts with matches crate #66518

Mark-Simulacrum opened this issue Nov 18, 2019 · 8 comments
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Path resolution P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

cc #65721

https://crater-reports.s3.amazonaws.com/beta-1.40-1/beta-2019-11-06/reg/bronzedb-protocol-0.1.0/log.txt

Any(?) crate using the matches crate from crates.io is likely to have breakage as the macro there will be ignored in favor of the std macro at least in some cases.

@Mark-Simulacrum Mark-Simulacrum added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Nov 18, 2019
@Mark-Simulacrum
Copy link
Member Author

cc @rust-lang/libs

@SimonSapin
Copy link
Contributor

Any(?) crate using the matches crate from crates.io

It’s not any crate. Servo’s dependency graph includes a number of users of matches, but we didn’t run into this particular issue.

We did run into glob imports becoming ambiguous servo/html5ever#402, but this is different. This code is already importing use matches::matches; specifically:

https://github.com/Hexilee/BronzeDB/blob/8ac9214633872e6869645b027595396d18781b2d/bronzedb-protocol/src/request.rs#L129

The only thing of note I see is that matches! is used inside the expansion of a local macro_rules! macro assert_delete, which in turned is used in the arguments (and presumably these tokens are forwarded as-is in the expansion) of an external procedural macro speculate:

https://github.com/utkarshkukreti/speculate.rs/blob/4e1a6f4c6dfc325dd9609ba890fc5f0bb4c6e2c8/src/lib.rs#L141-L142

That part of the macro’s input is parsed as syn::Block, then the syn::Stmt’s in the block are extracted and used in quote::quote_spanned!. I don’t know if any of this affects name resolution / hygiene.

It looks like the “connection” between the import and the use is “lost”. But what’s surprising is that this (seemingly) happens in 1.40.0 whereas the same code in 1.39.0 does resolve to the import, it doesn’t fail to resolve.

Does 1.40 also include changes to macro hygiene?

@jonas-schievink jonas-schievink added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Path resolution T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 21, 2019
@jonas-schievink
Copy link
Contributor

cc @petrochenkov

@pnkfelix
Copy link
Member

pnkfelix commented Nov 21, 2019

triage: P-high; need to at least determine scope of what changed here. And preferably do a bisection. Leaving nominated for now.

@pnkfelix pnkfelix added P-high High priority E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Nov 21, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 21, 2019

what changed here

matches was added to the standard library prelude.
The compiler / language didn't change.

We did run into glob imports becoming ambiguous servo/html5ever#402, but this is different.

The error says

error[E0659]: `matches` is ambiguous (glob import vs any other name from outer scope during import/macro resolution)

so it looks like the same issue?
The error even shows the glob's span, but it points to some macro arguments, so the glob is probably produced by a macro.

@SimonSapin
Copy link
Contributor

Oh sorry, I was looking at the first errors which do not mention ambiguity or glob and missed that later errors do.

[INFO] [stderr] error[E0658]: use of unstable library feature 'matches_macro'
[INFO] [stderr]    --> src/request.rs:152:21
[INFO] [stderr]     |
[INFO] [stderr] 152 |             assert!(matches!(&new_request, Request::Delete(ref _key)));
[INFO] [stderr]     |                     ^^^^^^^
[INFO] [stderr] ...
[INFO] [stderr] 162 |                 assert_delete!(b"name");
[INFO] [stderr]     |                 ------------------------ in this macro invocation
[INFO] [stderr]     |
[INFO] [stderr]     = note: for more information, see https://github.com/rust-lang/rust/issues/65721

… and there is the glob import:

https://github.com/utkarshkukreti/speculate.rs/blob/4e1a6f4c6dfc325dd9609ba890fc5f0bb4c6e2c8/src/generator.rs#L40-L47

        quote_spanned!(name.span() =>
            mod #name {
                #[allow(unused_imports)]
                use super::*;

So yes it’s the same issue. Thanks @petrochenkov.

I’ve already written my thoughts on this at #65721 (comment).

@pnkfelix
Copy link
Member

okay, based on the dialogue here, I'm treating this as not a T-compiler thing.

(@SimonSapin did propose some compiler/diagnostic changes to try to mitigate the effect of additions like this, in #65721, but that doesn't change the fact that this is still just a library issue, not a compiler or language one.)

Removing T-compiler label.

@pnkfelix pnkfelix removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 22, 2019
qryxip added a commit to qryxip/atcoder-rust-base that referenced this issue Dec 15, 2019
@Mark-Simulacrum
Copy link
Member Author

Closing as we've "eaten" this at this point as it's landed into stable.

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: Path resolution P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants