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

suggest | when , founds in invalid match value #57008

Conversation

Krout0n
Copy link
Contributor

@Krout0n Krout0n commented Dec 20, 2018

Closes #54807
I get stuck on (what | how) I should implement...

@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 @petrochenkov (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 Dec 20, 2018
@Krout0n Krout0n changed the title suggest | when , founds in invalid match arms suggest | when , founds in invalid match value Dec 20, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Dec 20, 2018

You need to update some tests and commit the changes. You can do this via

./x.py test --stage 1 src/test/ui --bless

and adjust the errors at the file:line locations mentioned in the errors until the command passes successfully

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 20, 2018

The current behavior is intentional and has reasons, as it was mentioned in #54807.
In some very popular languages like Python tuples can be written without parentheses (A, B) <=> A, B, so it's reasonable to expect that the user could actually mean (A, B) when writing A, B.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 21, 2018

As I mentioned in #54807 (comment), I believe it would be cool to suggest both (A, B) and A | B. Do you think doing so will complicate the parser more than it's worth it for such a small suggestion?

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 21, 2018

Suggesting both would be ok, the change will need to be done in fn parse_top_level_pat then.
It will give some false positives like recommending let A | B = expr;, but that doesn't matter much, it will become legal soon, at least in the parser.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2018
@Krout0n
Copy link
Contributor Author

Krout0n commented Dec 26, 2018

Hi, thanks for reviewing! I need solid blocks of time to understand what you mean and implement. So just a moment pls :)

@XAMPPRocky
Copy link
Member

Triage; @Knium Hello, have you been able to get back to this PR?

@Krout0n
Copy link
Contributor Author

Krout0n commented Jan 13, 2019

@Aaronepower Hi, thank you for reminding! Sorry, I'm busier than I thought... If this issue has hurry deadline or there is anyone who want to solve, I approve that someone takes my place. If no one else, I'm still eager to challenge this.

@TimNN
Copy link
Contributor

TimNN commented Jan 22, 2019

Ping from triage @Knium:

Sorry, I'm busier than I thought... If this issue has hurry deadline or there is anyone who want to solve, I approve that someone takes my place.

Don't worry, there is no deadline :). We just like to check in on PRs ~regularly so that we don't have dozens of PRs open that are not being worked on. If you are interested, you can find more information here: https://forge.rust-lang.org/triage-procedure.html

@bors

This comment has been minimized.

@Krout0n Krout0n force-pushed the misleading-try-adding-parentheses-in-match-with-comma branch from 20d7976 to 2a1ee1f Compare January 29, 2019 02:41
@rust-highfive

This comment has been minimized.

@Krout0n Krout0n force-pushed the misleading-try-adding-parentheses-in-match-with-comma branch from 2a1ee1f to 1123a65 Compare January 29, 2019 04:15
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit, then it lgtm

src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
@Krout0n Krout0n force-pushed the misleading-try-adding-parentheses-in-match-with-comma branch from 1123a65 to f19935d Compare January 30, 2019 04:49
@Krout0n Krout0n force-pushed the misleading-try-adding-parentheses-in-match-with-comma branch from f19935d to 62867b4 Compare January 30, 2019 04:51
@oli-obk
Copy link
Contributor

oli-obk commented Jan 30, 2019

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Jan 30, 2019

📌 Commit 62867b4 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 30, 2019
…eses-in-match-with-comma, r=oli-obk

suggest `|` when `,` founds in invalid match value

Issue rust-lang#54807
I get stuck on (what | how) I should implement...
@petrochenkov petrochenkov assigned oli-obk and unassigned petrochenkov Jan 30, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 30, 2019
…eses-in-match-with-comma, r=oli-obk

suggest `|` when `,` founds in invalid match value

Issue rust-lang#54807
I get stuck on (what | how) I should implement...
Centril added a commit to Centril/rust that referenced this pull request Jan 30, 2019
…eses-in-match-with-comma, r=oli-obk

suggest `|` when `,` founds in invalid match value

Issue rust-lang#54807
I get stuck on (what | how) I should implement...
Centril added a commit to Centril/rust that referenced this pull request Jan 31, 2019
…eses-in-match-with-comma, r=oli-obk

suggest `|` when `,` founds in invalid match value

Issue rust-lang#54807
I get stuck on (what | how) I should implement...
bors added a commit that referenced this pull request Jan 31, 2019
Rollup of 12 pull requests

Successful merges:

 - #57008 (suggest `|` when `,` founds in invalid match value)
 - #57106 (Mark str::trim.* functions as #[must_use].)
 - #57920 (use `SOURCE_DATE_EPOCH` for man page time if set)
 - #57934 (Introduce into_raw_non_null on Rc and Arc)
 - #57971 (SGX target: improve panic & exit handling)
 - #57980 (Add the edition guide to the bookshelf)
 - #57984 (Improve bug message in check_ty)
 - #57999 (Add MOVBE x86 CPU feature)
 - #58000 (Fixes and cleanups)
 - #58005 (update docs for fix_start/end_matches)
 - #58007 (Don't panic when accessing enum variant ctor using `Self` in match)
 - #58008 (Pass correct arguments to places_conflict)

Failed merges:

r? @ghost
@bors bors merged commit 62867b4 into rust-lang:master Jan 31, 2019
@Krout0n Krout0n deleted the misleading-try-adding-parentheses-in-match-with-comma branch January 31, 2019 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants