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

Sidestep bad precedence of closures in `let` bindings #64277

Open
wants to merge 1 commit into
base: master
from

Conversation

@estebank
Copy link
Contributor

commented Sep 8, 2019

Avoid adding unnecessary parentheses around closures in
if let _ = || ().

CC #51635.

r? @petrochenkov

Sidestep bad precedence of closures in `let` bindings
Avoid adding unnecessary parentheses around closures in
`if let _ = || ()`.
@estebank

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Tested locally on project with the issue:

error[E0308]: mismatched types
  --> src/main.rs:12:12
   |
12 |     if let Some(_) = || Some(()) {
   |            ^^^^^^^ expected closure, found enum `std::option::Option`
   |
   = note: expected type `[closure@src/main.rs:12:22: 12:33]`
              found type `std::option::Option<_>`

error[E0308]: mismatched types
  --> src/main.rs:17:5
   |
5  | fn problem() -> String {
   |                 ------ expected `std::string::String` because of return type
...
17 |     1
   |     ^
   |     |
   |     expected struct `std::string::String`, found integer
   |     help: try using a conversion method: `1.to_string()`
   |
   = note: expected type `std::string::String`
              found type `{integer}`

error: aborting due to 2 previous errors
|| parser::needs_par_as_let_scrutinee(scrutinee.precedence().order())
|| parser::needs_par_as_let_scrutinee(scrutinee.precedence().order())
// #51635: handle closures correctly, even though they have a really low
// precedence, they *don't* need parens for assignment 🤷.

This comment has been minimized.

Copy link
@Centril

Centril Sep 8, 2019

Member

Heh; would be good to replace 🤷 with the actual reason if you have a sec to figure it out.

This comment has been minimized.

Copy link
@estebank

estebank Sep 8, 2019

Author Contributor

I'm waiting on insight from @petrochenkov, I believe the precedence order between let and closures is incorrectly, but I didn't want to affect all other relative precedences accidentally so I did this, but the underlying reason would require going over every Precedence and double checking them. Clearly the Parser disagrees with what we have encoded :-|

This comment has been minimized.

Copy link
@estebank

estebank Sep 9, 2019

Author Contributor

The alternatives would of course be to either change the parser to conform to the encoded precedence (potentially breaking working code) or changing the precedence order (potentially breaking working code) :-|

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

I don't know right away whether this change is correct or not and don't have time to investigate.
r? @Centril who implemented let expressions including pretty-printing and has more context.

@rust-highfive rust-highfive assigned Centril and unassigned petrochenkov Sep 9, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

So the precedences here are a bit tricky. This may take some time to think through but I'll try to find some of that soon.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Update for wg-triage: I will set aside time to review this tomorrow or declare reviewer bankruptcy if it takes a lot more. :)

@estebank

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@Centril did you get a chance to take a look at this?

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Here's my investigation thus far:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=1032dc3984e4673a1d4f3405c255342b

There seems to be more wrong with the current code than this PR intends to fix but it doesn't seem like the diff in the PR is the right fix either.

It could be that the parser is incorrect, but if not, then pprust should do something more like:

    /// Print a `let pat = scrutinee` expression.
    crate fn print_let(&mut self, pat: &ast::Pat, scrutinee: &ast::Expr) {
        self.s.word("let ");

        self.print_pat(pat);
        self.s.space();

        self.word_space("=");
        let order = scrutinee.precedence().order();
        self.print_expr_cond_paren(
            scrutinee,
            parser::contains_exterior_struct_lit(scrutinee)
            || parser::needs_par_as_let_scrutinee(order) && order > PREC_JUMP
        )
    }

I think we'll also need to add appropriate tests to the PR. The playground above might be reusable for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.