Ensure we associate an error with all missing identifiers in the parser.#44389
Conversation
333fred
left a comment
There was a problem hiding this comment.
This cannot be merged until after the syntax changes for function pointers go in, which will be after Preview 3.
Why? This doesn't affect function pointers at all. It just fixes a current deficiency in parsing new-types. |
|
I suspect Fred probably wants to minimize extra work (resolving conflicts) that might impede merging a scheduled feature. I haven't looked at the change on either side, so I don't know what are the chances of conflicts. |
When I blocked this, I just looked at the PR title, which implies that it would break function pointers which uses an errorless missing identifier. If that's not what this PR is doing, then perhaps a different title would be clearer.
|
|
||
| ParseAndValidate(text, TestOptions.Regular, | ||
| // (7,21): error CS1526: A new expression requires an argument list or (), [], or {} after type | ||
| // (7,21): error CS1031: Type expected |
There was a problem hiding this comment.
This seems wrong, especially in the face of target-typed new. A type is not necessarily expected here.
There was a problem hiding this comment.
In parsers it's very common to just state what the expectation is for the common case when things are missing. It's not like the otehr error message is correct either. it's acting as if we have a type (we don't, we have a missing identifier), and then saying what is missing coming after that missing identifier.
There was a problem hiding this comment.
Sure. But I would prefer that we introduce a message that says something like "Type or parens expected".
There was a problem hiding this comment.
i'm not opposed to that. it's just generally not something we commonly do. With missing stuff, we commonly just specify the 95% case we expect. It helps keeps teh errors relevant and not overloaded with tons of information.
I'd honestly prefer that if we do this, we do it through a "consistifying pass" that did this for all messages instead of doing it in an adhoc fashion.
There was a problem hiding this comment.
I'm not opposed to a consistifying pass. But I also think we should fix them where they're obvious
There was a problem hiding this comment.
It's not obvious to me that anything is wrong with this will message. I.e. I don't think it's the goal of a parse error to enumerate all potential legal follow-up tokens/nodes.
There was a problem hiding this comment.
@CyrusNajmabadi, my opinion on this hasn't changed. This is a worse error message, and I do not want to take the change with it.
|
Done review pass (commit 1) |
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (iteration 5). Please use squash/merge since compiler PR.
|
Done, thanks :) |
No description provided.