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

: token not accepted in attribute? #65524

Closed
Mark-Simulacrum opened this issue Oct 17, 2019 · 6 comments
Closed

: token not accepted in attribute? #65524

Mark-Simulacrum opened this issue Oct 17, 2019 · 6 comments
Assignees
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

https://crater-reports.s3.amazonaws.com/beta-1.39-1/beta-2019-09-28/reg/jellyschema-0.11.10/log.txt

I can't seem to reproduce this in a trivial example so it might be related to pre-cfg expansion or something like that...

cc #64962

@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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 labels Oct 17, 2019
@Mark-Simulacrum Mark-Simulacrum added this to the 1.39 milestone Oct 17, 2019
@petrochenkov petrochenkov self-assigned this Oct 17, 2019
@petrochenkov
Copy link
Contributor

Most likely introduced by #64462.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 17, 2019

Minimized:

macro_rules! m { () => () }

#[allow(clippy:all)]
m!();

fn main() {}

Inert attributes on macro invocations (like allow in this case) disappear during macro expansion.

Previously such attributes were partially checked (compared to inert attributes remaining in the AST), after #64462 they are fully checked (in the same way as inert attributes remaining in the AST).
It may make sense to not check them at all (cc #63221).

@petrochenkov petrochenkov removed 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 labels Oct 17, 2019
@estebank
Copy link
Contributor

It sounds like we would want to warn on these cases.

@Centril
Copy link
Contributor

Centril commented Oct 17, 2019

It may make sense to not check them at all (cc #63221).

Imo, based on a single regression, which also exposed a bug in the crate, it doesn't seem like a justified change.

@estebank
Copy link
Contributor

I hadn't checked how many dependencies it had. It should be fine but we must proactively reach out to fix their crate.

@Centril
Copy link
Contributor

Centril commented Oct 19, 2019

Done in balena-io-modules/jellyschema#118.

@Centril Centril closed this as completed Oct 19, 2019
@Centril Centril removed this from the 1.39 milestone Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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

4 participants