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

Suppress some lints that trigger don't trigger in all cfg's #83

Merged
merged 1 commit into from
May 16, 2022

Conversation

Xiphoseer
Copy link
Contributor

This allows a new nightly lint (rust-lang/rust#96150) on some macros that trigger because of how they interact with cfg attributes. From my point-of-view, it didn't make sense to replicate those cfg in an cfg_attr here. There's another case in my patch-5 branch, which might not be a false-positive like these ones.

@Alexhuszagh
Copy link
Owner

The issue isallow(unused_macro_rules) should produce a warning of unknown_lints if cond is true on anything under nightly-2022-05-12, but should work on nightly-2022-05-12 and above. This means we need some more complex logic, IE, allow(unknown_lints, unused_macro_rules), which I believe I've partially committed as of effaae8.

#![allow(dead_code, unused_macros, unknown_lints, unused_macro_rules)]

Copy link
Owner

@Alexhuszagh Alexhuszagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace the #[allow(unused_macros)] changes to also allow unknown_lints, or #[allow(unknown_lints, unused_macros)].

Also please rebase to make sure input.rs doesn't have any conflicts, since I just submitted a similar patch to that file.

lexical-benchmark/input.rs Outdated Show resolved Hide resolved
lexical-parse-float/src/index.rs Outdated Show resolved Hide resolved
@Alexhuszagh
Copy link
Owner

Alexhuszagh commented May 16, 2022

@Xiphoseer I'm planning a release this Wednesday to fix a bug present in #84 in the float writer, so I was wondering if these changes could be made prior to then to ensure no spurious warnings are present. If not, there are no issues, I would like to give you credit for your work so please take your own time. I appreciate all the work you've done.

@Alexhuszagh Alexhuszagh merged commit 0cf23c8 into Alexhuszagh:main May 16, 2022
@Xiphoseer
Copy link
Contributor Author

Xiphoseer commented May 16, 2022

Right, I totally forgot about unknown_lints and that it would affect stable and other nightlies. I had looked at dtolnay/syn@189c60c and didn't realize that this only doesn't need unknown_lints because it's just tests.

I've run the CI checks locally before submitting and they all went through. It looks to me like no CI check actually runs something like RUSTFLAGS=-Dwarnings cargo +stable check. If I run that on the old commit, it obviously errors out. Might be useful to add that as a non-required job.

Also noticed something weird while testing this: The unknown_lints lint doesn't seem to pick up lints within a cfg_attr at all times, filed rust-lang/rust#97094 for that just now. So with this PR merged, I don't think any of them will error, but the one in

#![allow(dead_code, unused_macros, unknown_lints, unused_macro_rules)]
is now redundant, the one in
#![cfg_attr(feature = "compact", allow(unused_macros, unused_macro_rules))]
should error on stable but doesn't and is also duplicate, and I should have removed the two in
#[allow(unknown_lints, unused_macro_rules)]
and
#[allow(unknown_lints, unused_macro_rules)]
because you removed the entire macro branch earlier.

@Xiphoseer Xiphoseer deleted the patch-3 branch May 16, 2022 23:03
@Alexhuszagh
Copy link
Owner

Right, I totally forgot about unknown_lints and that it would affect stable and other nightlies. I had looked at dtolnay/syn@189c60c and didn't realize that this only doesn't need unknown_lints because it's just tests.

I've run the CI checks locally before submitting and they all went through. It looks to me like no CI check actually runs something like RUSTFLAGS=-Dwarnings cargo +stable check. If I run that on the old commit, it obviously errors out. Might be useful to add that as a non-required job.

Also noticed something weird while testing this: The unknown_lints lint doesn't seem to pick up lints within a cfg_attr at all times, filed rust-lang/rust#97094 for that just now. So with this PR merged, I don't think any of them will error, but the one in

#![allow(dead_code, unused_macros, unknown_lints, unused_macro_rules)]

is now redundant, the one in

#![cfg_attr(feature = "compact", allow(unused_macros, unused_macro_rules))]

should error on stable but doesn't and is also duplicate, and I should have removed the two in

#[allow(unknown_lints, unused_macro_rules)]

and

#[allow(unknown_lints, unused_macro_rules)]

because you removed the entire macro branch earlier.

Incredible work, thank you for all your help with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants