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

Don't emit "is not a logical operator" error outside of associative expressions #75658

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

tgnottingham
Copy link
Contributor

Avoid showing this error where it doesn't make sense by not assuming
"and" and "or" were intended to mean "&&" and "||" until after we decide
to continue parsing input as an associative expression.

Note that the decision of whether or not to continue parsing input as an
associative expression doesn't actually depend on this assumption.

Fixes #75599


First time contributor! Let me know if there are any conventions or policies I should be following that I missed here. Thanks :)

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2020
@tgnottingham
Copy link
Contributor Author

tgnottingham commented Aug 18, 2020

r? @estebank

Hi @estebank, thought you might be the best person to review, since you've touched this part of the code recently.


Also, want to point out that, with this change, code like this...

fn foo(a: Option<u32>, b: Option<u32>) -> bool {
    if let Some(x) = a { true } else { false }
    and
    if let Some(y) = a { true } else { false }
}

...will no longer generate the suggestion to replace "and" with "&&". But it still suggests using parens to turn the construct into an expression, and after you do that, you will get the "and" -> "&&" suggestion.

Oops, I'm mistaken. It doesn't show the parens suggestion. But it does show the exact same errors as before, sans the "and" -> "&&" suggestion. Not sure if this is a deal-breaker.

error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found keyword `if`
 --> src/main.rs:4:5
  |
3 |     and
  |        - expected one of 8 possible tokens
4 |     if let Some(y) = a { true } else { false }
  |     ^^ unexpected token

error[E0308]: mismatched types
 --> src/main.rs:2:26
  |
2 |     if let Some(x) = a { true } else { false }
  |     ---------------------^^^^------------------ help: consider using a semicolon here
  |     |                    |
  |     |                    expected `()`, found `bool`
  |     expected this to be `()`

error[E0308]: mismatched types
 --> src/main.rs:2:40
  |
2 |     if let Some(x) = a { true } else { false }
  |     -----------------------------------^^^^^--- help: consider using a semicolon here
  |     |                                  |
  |     |                                  expected `()`, found `bool`
  |     expected this to be `()`

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 18, 2020
@estebank
Copy link
Contributor

This looks good to me. Can you squash the two commits? r=me after that.

@estebank
Copy link
Contributor

But it does show the exact same errors as before, sans the "and" -> "&&" suggestion. Not sure if this is a deal-breaker.

That is unfortunate, but at the same time the code is far away from correct enough for us to accept the lack of helpful suggestions.

…xpressions

Avoid showing this error where it doesn't make sense by not assuming
"and" and "or" were intended to mean "&&" and "||" until after we decide
to continue parsing input as an associative expression.

Note that the decision of whether or not to continue parsing input as an
associative expression doesn't actually depend on this assumption.

Fixes rust-lang#75599
@tgnottingham
Copy link
Contributor Author

Squashed the commits. Thanks @estebank.

@jyn514
Copy link
Member

jyn514 commented Aug 18, 2020

@bors r=estebank

@bors
Copy link
Contributor

bors commented Aug 18, 2020

📌 Commit ff73a40 has been approved by estebank

@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 Aug 18, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
Don't emit "is not a logical operator" error outside of associative expressions

Avoid showing this error where it doesn't make sense by not assuming
"and" and "or" were intended to mean "&&" and "||" until after we decide
to continue parsing input as an associative expression.

Note that the decision of whether or not to continue parsing input as an
associative expression doesn't actually depend on this assumption.

Fixes rust-lang#75599

---

First time contributor! Let me know if there are any conventions or policies I should be following that I missed here. Thanks :)
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
Don't emit "is not a logical operator" error outside of associative expressions

Avoid showing this error where it doesn't make sense by not assuming
"and" and "or" were intended to mean "&&" and "||" until after we decide
to continue parsing input as an associative expression.

Note that the decision of whether or not to continue parsing input as an
associative expression doesn't actually depend on this assumption.

Fixes rust-lang#75599

---

First time contributor! Let me know if there are any conventions or policies I should be following that I missed here. Thanks :)
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
Don't emit "is not a logical operator" error outside of associative expressions

Avoid showing this error where it doesn't make sense by not assuming
"and" and "or" were intended to mean "&&" and "||" until after we decide
to continue parsing input as an associative expression.

Note that the decision of whether or not to continue parsing input as an
associative expression doesn't actually depend on this assumption.

Fixes rust-lang#75599

---

First time contributor! Let me know if there are any conventions or policies I should be following that I missed here. Thanks :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#75038 (See also X-Link mem::{swap, take, replace})
 - rust-lang#75049 (docs(marker/copy): provide example for `&T` being `Copy`)
 - rust-lang#75499 (Fix documentation error)
 - rust-lang#75554 (Fix clashing_extern_declarations stack overflow for recursive types.)
 - rust-lang#75646 (Move to intra doc links for keyword documentation)
 - rust-lang#75652 (Resolve true and false as booleans)
 - rust-lang#75658 (Don't emit "is not a logical operator" error outside of associative expressions)
 - rust-lang#75665 (Add doc examples coverage)
 - rust-lang#75685 (Switch to intra-doc links in /src/sys/unix/ext/*.rs)

Failed merges:

r? @ghost
@bors bors merged commit e6fe523 into rust-lang:master Aug 19, 2020
@tgnottingham tgnottingham deleted the issue-75599 branch January 20, 2021 18:33
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression involving "is not a logical operator" in match
7 participants