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

"this could sometimes panic" lint #959

Open
oli-obk opened this issue May 27, 2016 · 10 comments
Open

"this could sometimes panic" lint #959

oli-obk opened this issue May 27, 2016 · 10 comments
Labels
A-lint Area: New lints E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-correctness Lint: Belongs in the correctness lint group T-MIR Type: This lint will require working with the MIR

Comments

@oli-obk
Copy link
Contributor

oli-obk commented May 27, 2016

minimal range analysis (lots of snippets like if x == 0 { return; } x -= 1; around, for example) to eliminate false positives (range analysis by itself can provide useful optimizations)
the hardest thing to reason about is that v1.len() + v2.len() can't ever overflow if v1, v2: Vec<T> and size_of::<T>() > 0

idea by @eddyb https://botbot.me/mozilla/rust-internals/2016-05-27/?msg=66824497&page=3

@eddyb
Copy link
Member

eddyb commented May 27, 2016

The context is #33905, for which I made a simple change to warn on all #[inline] or generic functions with overflow checks and then looked at the hundreds of results, which showed some interesting patterns.

@nagisa's dataflow framework should usable for tracking ranges based on comparisons, for cases where checks prevent overflows (e.g. if i < n { n - i }), without much difficulty.

Some of the "won't overflow only because you can't have more allocated bytes in total than isize::MAX" situations are in unsafe code deep in collection implementations, so most code could be free of false positives even without understanding that, but it's not a given.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 6, 2016

Don't https://internals.rust-lang.org/t/mir-compiler-plugins-for-custom-mir-passes/3166/11?u=ker and rust-lang/rust#34066 basically state that we should not be using MIR for linting?

@nagisa
Copy link
Member

nagisa commented Jun 6, 2016

The AST passes never were stabilised explicitly either so what is said
about the MIR passes was supposed to equally apply to the AST passes too.
On Jun 6, 2016 1:48 PM, "Oliver Schneider" notifications@github.com wrote:

Don't
https://internals.rust-lang.org/t/mir-compiler-plugins-for-custom-mir-passes/3166/11?u=ker
and rust-lang/rust#34066 rust-lang/rust#34066
basically state that we should not be using MIR for linting?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Manishearth/rust-clippy/issues/959#issuecomment-223926092,
or mute the thread
https://github.com/notifications/unsubscribe/AApc0h0Q-Xp2nBTCcuzWBfK02kUTOU4jks5qI_r5gaJpZM4IoeaJ
.

@eddyb
Copy link
Member

eddyb commented Jun 6, 2016

@oli-obk Well, that doesn't make MIR less stable than all of the currently unstable compiler internals all lints use. There might be a way to expose the CFG and pieces of the instructions in a less unstable manner, but IMO not having that yet shouldn't stop people for building incredibly powerful tools.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 8, 2017

@Manishearth can we have a T-MIR label?

@Manishearth
Copy link
Member

Done. you can add labels in https://github.com/Manishearth/rust-clippy/labels

@Manishearth Manishearth added the T-MIR Type: This lint will require working with the MIR label Feb 8, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 8, 2017

thanks! I thought that was an owner-only-feature.

@oli-obk oli-obk added L-correctness Lint: Belongs in the correctness lint group E-hard Call for participation: This a hard problem and requires more experience or effort to work on A-lint Area: New lints labels Feb 8, 2017
@mcarton
Copy link
Member

mcarton commented Mar 15, 2017

FYI, the ability to register MIR passes was removed: rust-lang/rust#40239.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 15, 2017

That's not an issue. We can get the MIR from the tcx and the body_id.

@oli-obk oli-obk closed this as completed Mar 15, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 15, 2017

Wrong button

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-hard Call for participation: This a hard problem and requires more experience or effort to work on L-correctness Lint: Belongs in the correctness lint group T-MIR Type: This lint will require working with the MIR
Projects
None yet
Development

No branches or pull requests

5 participants