-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
[DO NOT MERGE] crater proposed changes in unused must use lint #148577
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
Conversation
…use lint (or more accurately `Result<T, Uninhabited>`/`ControlFlow<Uninhabited, T>`). This generalizes a previous change where we only did this for `T = ()`.
god I actually hate nested functions now lol. just use modules >:(
7260bf5 to
310b963
Compare
This comment has been minimized.
This comment has been minimized.
[DO NOT MERGE] crater proposed changes in unused must use lint
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
for context: lints emitted for libraries
lints emitted for the compiler
|
|
@craterbot check +rustflags=-Dunmustuse_in_always_ok |
|
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ask in t-infra on Zulip |
|
Hmm, let's try @craterbot run mode=check-only start=642c19bfc3a5c1de985bf5d0cc8207ac9d22708a end=4f260f0f20b3133d20cfb50353c0221943af5796+rustflags=-Dunmustuse_in_always_ok |
|
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ask in t-infra on Zulip |
|
@craterbot run mode=check-only start=master#642c19bfc3a5c1de985bf5d0cc8207ac9d22708a end=try#4f260f0f20b3133d20cfb50353c0221943af5796+rustflags=-Dunmustuse_in_always_ok |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
I'll move this over to #148214 at some point, but wanted to share my take on the crater results (and share some examples). So, what's very clear is that this pattern comes in a lot of places. Definitely more than I would have expected coming into this. With this being said, I've gone through and opened a few crater results to see if I could spot any examples of @joshtriplett's concern, which was basically like "People may be relying on that So, going into this, I'm looking for types where the That being said, given that this comes up so much, I think this decision is likely not as "light" as the lang team originally expected. Though, it's completely backwards- and forwards- compatible to make this change and undo later. Here a couple of examples of code I saw, though far from exhaustive.
|
|
@jackh726 what do you think the next step could be? |
|
I'll probably move this comment over and nominate to discuss in a lang meeting. Unless you can think of some more restricted change to crater, not sure if there's more to do here - just for lang to make a decision with the new data. |
|
@jackh726 do you still want to see the inverse set, i.e. |
|
It's a good question - I'm not sure it actually will end up giving all that much decision-making power. |
Based on #148214, easiest to review commit-by-commit.
This adds two lints:
unmustuse_in_always_okchecks for expressions of typeResult<T, I>orControlFlow<I, T>whereIis uninhabitedTis not a generic paramTis not unitTdoes not have#[must_use]annotationmustuse_in_always_okchecks for expressions of typeResult<T, I>orControlFlow<I, T>whereIis uninhabitedTis not a generic paramTdoes have#[must_use]annotation (or is otherwise decided that it must be used)The idea is to run crater with
-D...such that we can detect where these cases occur.r? @jackh726