Uplift `clippy::cmp_nan` lint by Urgau · Pull Request #111818 · rust-lang/rust ·...

•

edited

This PR aims at uplifting the clippy::cmp_nan lint into rustc.

`invalid_nan_comparisons`

~~(deny-by-default)~~ (warn-by-default)

The invalid_nan_comparisons lint checks comparison with f32::NAN or f64::NAN as one of the operand.

Example

let a = 2.3f32;
if a == f32::NAN {}

Explanation

NaN does not compare meaningfully to anything – not even itself – so those comparisons are always false.

Mostly followed the instructions for uplifting a clippy lint described here: #99696 (review)

@rustbot label: +I-lang-nominated
r? compiler

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

labels

Collaborator

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

added the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label

This comment has been minimized.

Contributor

Is there some idea why some correctness lints uplifted and others no? I'm expecting that uplifted lints should hit some code often (to be inside rustc), but thinking that anyone playing with NaN's quite rare (looking at github search).

Contributor

Author

Urgau

commented

•

edited

Is there some idea why some correctness lints uplifted and others no? I'm expecting that uplifted lints should hit some code often (to be inside rustc), but thinking that anyone playing with NaN's quite rare (looking at github search).

Doing a simple search in github reveals quite a few instances of people doing direct comparisons against f[32,64}::NAN; it's not huge, but it's not zero either, and I think a lot of that is down to the lint, which is what motivated me to uplift that lint.

Concerning why some lints are being uplifted and not others, I believe this is simply due to two things, man power (it takes quite a few hours, in particular for lints that don't have good diagnostics) and whenever they detect actual bugs with zero false-positive.

Contributor

Using clippy is user's desire, and don't affect anyone except him; but uplifting lints means that now all users will pay that (almost zero, i assume in current case; there was perf run with disabled lints somewhere) price.

Contributor

Author

Urgau

commented

•

edited

Using clippy is user's desire, and don't affect anyone except him; but uplifting lints means that now all users will pay that (almost zero, i assume in current case; there was perf run with disabled lints somewhere) price.

You're right there is a cost (I expect this lint to have near zero impact), which is why I only try to uplift meaningful (UB/near UB/"obviously not what was intended and is wrong") clippy lints.

Contributor

r? compiler

Contributor

The latest upstream changes (presumably #111848) made this pull request unmergeable. Please resolve the merge conflicts.

This comment was marked as outdated.

rfcbot

added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it.

labels

This comment was marked as outdated.

rfcbot

removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it.

labels

nikomatsakis

removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

labels

Contributor

@rustbot labels +T-lang

added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label

Contributor

@rfcbot merge

@rfcbot concern warn-by-default

In the lang team meeting we agreed we want this lint, but that we should make it warn-by-default to keep it consistent with other lints (and reserve deny-by-default for things like insta-UB).

dtolnay reacted with thumbs up emojiasquared31415 reacted with confused emoji

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

~~warn-by-default~~ resolved by Uplift clippy::cmp_nan lint #111818 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

removed T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. A-testsuite Area: The testsuite used to check the correctness of rustc

labels

Jun 9, 2023

This comment has been minimized.

added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label

Jun 9, 2023

Contributor

Trying commit 88ed4ce with merge b663820...

Contributor

Try build successful - checks-actions
Build commit: b663820 (b663820c694a10662705f377280fd6f68105f4c7)

This comment has been minimized.

Collaborator

Finished benchmarking commit (b663820): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Cycles

Results

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 649.669s -> 649.783s (0.02%)