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

[let_chains] Forbid let inside parentheses #95008

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Mar 16, 2022

Parenthesizes are mostly a no-op in let chains, in other words, they are mostly ignored.

let opt = Some(Some(1i32));

if (let Some(a) = opt && (let Some(b) = a)) && b == 1 {
    println!("`b` is declared inside but used outside");
}

As seen above, such behavior can lead to confusion.

A proper fix or nested encapsulation would probably require research, time and a modified MIR graph so in this PR I simply denied any let inside parentheses. Non-let stuff are still allowed.

fn main() {
    let fun = || true;
    
    if let true = (true && fun()) && (true) {
        println!("Allowed");
    }
}

It is worth noting that let ... is not an expression and the RFC did not mention this specific situation.

cc @matthewjasper

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 16, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2022
@bors
Copy link
Contributor

bors commented Mar 28, 2022

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

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 31, 2022
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Hi @rust-lang/lang -- I'd like to unblock let-chains. It seems to me that permitting parentheses to contain let introduces some lack of clarity and isn't really particularly important. I'd like to FCP just removing it from what is accepted, as proposed by this PR.

If I understand correctly, the result of this is that a let Pattern = Expr can only appear at the "top-level" of an if or as part of a && that is itself (transitively) in the "top-level". Things like the following are therefore illegal:

if (
    let Some(x) = option
) {
...
}

but this would work

if let Some(x) = option && something && let Some(z) = option { ... }

as would this

if let Some(x) = option && let Some(z) = option && (something || something_else) { ... }

@rfcbot
Copy link

rfcbot commented Mar 31, 2022

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

Concerns:

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!

See this document for info about what commands tagged team members can give me.

@rfcbot 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 Mar 31, 2022
@scottmcm
Copy link
Member

This sounds great; let's do it. We can always allow it in future, so personally I think we could even just merge the PR without waiting for an FCP.

@rfcbot reviewed

@joshtriplett
Copy link
Member

joshtriplett commented Mar 31, 2022

@rfcbot reviewed

@rfcbot concern currently

Can we please not use the wording "currently" in this context? That wording seems to imply us doing so in the future.

(I'm entirely in favor of this change otherwise.)

@c410-f3r
Copy link
Contributor Author

The "currently" word was removed as demanded

@joshtriplett
Copy link
Member

@rfcbot resolved currently

Thank you!

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Mar 31, 2022
@rfcbot
Copy link

rfcbot commented Mar 31, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 31, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 10, 2022
@rfcbot
Copy link

rfcbot commented Apr 10, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@c410-f3r
Copy link
Contributor Author

Someone has to r+

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📌 Commit 6ee3c47 has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#95008 ([`let_chains`] Forbid `let` inside parentheses)
 - rust-lang#95801 (Replace RwLock by a futex based one on Linux)
 - rust-lang#95864 (Fix miscompilation of inline assembly with outputs in cases where we emit an invoke instead of call instruction.)
 - rust-lang#95894 (Fix formatting error in pin.rs docs)
 - rust-lang#95895 (Clarify str::from_utf8_unchecked's invariants)
 - rust-lang#95901 (Remove duplicate aliases for `check codegen_{cranelift,gcc}` and fix `build codegen_gcc`)
 - rust-lang#95927 (CI: do not compile libcore twice when performing LLVM PGO)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2ad701e into rust-lang:master Apr 12, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 12, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants