Skip to content

Conversation

obeis
Copy link
Contributor

@obeis obeis commented Feb 18, 2023

Closes #107518

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I left some nits.

cc @pnkfelix who suggested this message -- To be quite honest, I'm not sure if the parser really benefits from such a bespoke recovery. I agree that the error message you originally hit was confusing, but this seems like it'll probably never get hit by someone else in production often -- at least with the way it's being implemented currently. Do you have thoughts? I don't want to block this parser change if you disagree.

@obeis obeis force-pushed the array-literal-note branch from 48fdf43 to 93a3409 Compare February 20, 2023 13:26
@cjgillot cjgillot assigned compiler-errors and unassigned cjgillot Feb 25, 2023
@bors
Copy link
Collaborator

bors commented Feb 26, 2023

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

@pnkfelix
Copy link
Contributor

r? @pnkfelix

@rustbot rustbot assigned pnkfelix and unassigned compiler-errors Mar 23, 2023
@pnkfelix
Copy link
Contributor

So, I first off want to apologize for how long its taken me to get around to this.

@compiler-errors wrote:

I'm not sure if the parser really benefits from such a bespoke recovery.

Looking at this PR, I'm inclined to agree with Michael's intuition above. This is a lot of dedicated code for a very narrow case.

It is not the approach I expected to see here. I was expecting someting more along the lines of: First establish some kind of state saying "we currently think we are inside an index-expr (i.e. a pair of [ ... ]), and then issuing a tailored message when we encounter the erroneous ;.

based on the rest of Michael's comment, its possible that I should accept that this issue just isn't going to be fixed at all.

I'm not quite ready to give up to that level, but I am willing to say that we don't want a fix that looks like this PR.

@obeis , if you're interested in working together to figure out what the right solution here looks like, please do ping me on Zulip (preferably in a fresh public topic under the #t-compiler stream) and we can try to hash this out.

But in the meantime, I'm going to close this PR because I don't think it will land in this state.

(Also, @obeis , thank you for your patience and for your contribution.)

@pnkfelix pnkfelix closed this May 18, 2023
@obeis obeis deleted the array-literal-note branch January 14, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

parser: [elem; count] is strong signal that an array literal was intended
7 participants