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

Allow irrefutable patterns in if-let statements #2081

Closed
Nokel81 opened this issue Jul 26, 2017 · 14 comments
Closed

Allow irrefutable patterns in if-let statements #2081

Nokel81 opened this issue Jul 26, 2017 · 14 comments
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@Nokel81
Copy link
Contributor

Nokel81 commented Jul 26, 2017

This might seem a bit strange but not allowing them has brought up some strange results for me.

The equivalent can be done with a plain match statement which is defiantly not as clean. In the docs it says to just use a plane let statement, but that is not always possible because of macros. If you accept a pattern in a macro then the ability to have the following -

if let $p = $val {
    $b
}

instead of

#[allow(unreachable_patterns)]
match $val {
    $p => {
        $b;
        break;
    },
    _ => ()
}
@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 26, 2017

This might not be very important but the disallowing of irrefutable patterns is nearly saying: totality detected reformat code to remove totality in the following statement:

let x = if true { 5 } else { 0 }

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2017

They could simply be allowed, and the dead_code lint could trigger on the else path (if there is one). Imo that should happen for if true, too, as long as the bool doesn't depend on cfg!.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

How would you tell if bool depends on cfg!. What is cfg!?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2017

The issue is e.g. if cfg!(target_pointer_width = "32") { foo } else { bar }, which executes foo on 32bit systems and bar on all others. If you just take the bare value, it'll be true on 32bit and false on others. So you'd get a warning for bar being useless on 64bit and a warning for foo being useless on 32bit.

What is cfg!?

https://doc.rust-lang.org/book/first-edition/conditional-compilation.html#cfg

How would you tell if bool depends on cfg!.

That's the big question. You'd need to track this during constant evaluation. Which definitely isn't easy, but should be a topic addressed if the warning rules around dead_code or unreachable_* lints is changed.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

Ah, that makes sense. Is there a way to tell automatically if a value's true value for the dead_code lint on an else case is platform specific or does it have to be hardcoded?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2017

Is there a way to tell automatically if a value's true value for the dead_code lint on an else case is platform specific or does it have to be hardcoded?

There isn't right now, but it's also not too hard to add such a feature.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

Shall I start writing up a pull request to this repo?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2017

Sure. But I'd limit the RFC to just the if let part and leave out the if part. They are orthogonal imo. Turning the error into a lint should totally suffice, since your macro can then allow the lint.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

That makes sense. Should I then include that the cfg! macro include that allow? Why not include the if part, that might make a good warning

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2017

You have a much bigger chance of getting a single targeted RFC through, than a collection of features

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

Then why not two different ones?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2017

Nothing speaks against that!

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

Two RFC PR now live: #2086 and #2087

@mbrubeck
Copy link
Contributor

#2086 was merged.

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 23, 2018
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

No branches or pull requests

4 participants