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

RFC: Warning on tautology in if or while statements #2087

Closed
wants to merge 1 commit into from
Closed

RFC: Warning on tautology in if or while statements #2087

wants to merge 1 commit into from

Conversation

Nokel81
Copy link
Contributor

@Nokel81 Nokel81 commented Jul 27, 2017

This is the initial commit for the RFC that adds compiler warnings for tautologies and contradictions. Similar to how there is currently a warning for while true { }

Edit: Rendered

@scottmcm scottmcm added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jul 27, 2017
@joshtriplett
Copy link
Member

Note that in general we might want functions to be able to allow this for expressions containing them. If you have a function that on architecture X does some work, and on architecture Y always returns a constant value, you shouldn't get a warning on architecture Y.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

This is why there is an [#allow]

@est31
Copy link
Member

est31 commented Jul 28, 2017

Agree with @joshtriplett . The lint has too many false positives. You shouldn't have to regularly #[allow] it. Therefore I think it better belongs to clippy than main rustc.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 28, 2017

Why would it have a lot of false positives, is the current checker that positived-biased?

@main--
Copy link

main-- commented Jul 28, 2017

Why would it have a lot of false positives, is the current checker that positived-biased?

AFAIK there is no current checker.

The check for while true only checks for literally this - const TRUE: bool = TRUE; while TRUE {} already evades it successfully. Its purpose is not checking for tautologies, it's only intended for programmers that don't know about loop and write while true instead.

That's why your RFC as written requires (to my knowledge) an entirely new checker to be implemented (and it's quite underspecified in that regard, probably because you did not realize this).

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 28, 2017

That is quite true that I did not realize that it did not exist. I will add those sorts of requirements to the RFC

@nrc
Copy link
Member

nrc commented Aug 1, 2017

This is probably better in Clippy than the compiler (I think if the while true check were proposed today, it would end up in Clippy).

@eddyb
Copy link
Member

eddyb commented Aug 1, 2017

@nrc I'd be very surprised if clippy doesn't already have this including a check that it came from a macro (so if cfg!(...) wouldn't get a warning) cc @llogiq @Manishearth @mcarton

@Manishearth
Copy link
Member

Clippy has http://rust-lang-nursery.github.io/rust-clippy/master/#nonminimal_bool and http://rust-lang-nursery.github.io/rust-clippy/master/#logic_bug which deal with collapsing conditions.
However, it does not have lints for cases like a < 5 && a > 5. Rustc's const eval stuff may help here, I'm not sure how much of that info is cached (shouldn't be too expensive to recalculate though).

Neither does it actually warn if you do if true.

Both of these lints could be added.

@joshtriplett
Copy link
Member

@est31

Agree with @joshtriplett . The lint has too many false positives. You shouldn't have to regularly #[allow] it. Therefore I think it better belongs to clippy than main rustc.

I wasn't arguing against this; I was arguing for implementing it with care to make sure it doesn't trigger on things that depend on cfg or similar conditional compilation.

@aturon aturon self-assigned this Aug 3, 2017
@Nokel81
Copy link
Contributor Author

Nokel81 commented Aug 10, 2017

Another benefit for something like this would be moving towards logical proofs of conditions

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 4, 2017

@Manishearth Are you saying that those lints could be added to rustc or clippy, it seems to me that you are saying that if they were in rustc the information is already there

@eddyb
Copy link
Member

eddyb commented Sep 4, 2017

@Nokel81 There shouldn't be any difference between rustc and clippy in that regard.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 14, 2017

Closing so that this can be discussed as an addition to clippy

@Nokel81 Nokel81 closed this Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants