[SR-510] diagnose enum raw value in closure parsing #987
Conversation
|
Hm. First request: please split the revert out as a separate commit. :-) (Same pull request is fine, so we don't regress on master, but it is logically separate.) Beyond that, the error message doesn't really apply here. There is an expression there; it's just not one that belongs there. |
and all changes introduced in the same pull request
|
@jrose-apple done and done. |
|
The error message is now raw value for enum case must be a literal. |
|
This approach seems reasonable, although the hard-coding under the Do you really think it's better to throw away the closure, rather than to create a dummy local context, parse it, and then complain? |
|
I guess we can add a very coarse assertion that some error was emitted if we end up in the "enum case was null" path. We can't guarantee that it happened while trying to parse the case, but if there haven't been any other errors, we won't let this one slip by. |
dduan
force-pushed
the
SR-510-3
branch
2 times, most recently
from
3807d29 to
8ec8bf4
Compare
|
@jrose-apple |
dduan
force-pushed
the
SR-510-3
branch
5 times, most recently
from
bc0713d to
7bd34ca
Compare
|
Update for @jrose-apple: I implemented your suggestion. Adding a local context made the patch much simpler. The closure gets parsed normally and errors related to both the closure itself and raw value gets issued.
All tests are passing. |
| @@ -4413,7 +4412,7 @@ ParserStatus Parser::parseDeclEnumCase(ParseDeclOptions Flags, | |||
| return Status; | |||
| } | |||
| if (RawValueExpr.isNull()) { | |||
| diagnose(NextLoc, diag::nonliteral_enum_case_raw_value); | |||
| assert("No error emmited for invalid enum raw value expression"); | |||
dduan
force-pushed
the
SR-510-3
branch
2 times, most recently
from
8271f27 to
e242ad7
Compare
|
Um. We can work out the assertion failures separately (it still seems valuable). I actually meant creating a local context ahead of time, so that we wouldn't have to hardcode a particular ID in the main expr-parsing loop. I guess this is cheaper, though, since it only comes up on failure. Would it at least make sense to move it into the closure-parsing logic, though? Also, how does that local context ever get cleaned up? Seems like it's just new'ed into space. |
dduan
force-pushed
the
SR-510-3
branch
2 times, most recently
from
9100bdd to
cf045f7
Compare
|
@jrose-apple Ok, I removed the hard-coded diagnosis from |
|
That still leaks the local context, though, doesn't it? |
|
@jrose-apple Oops, fixed. |
|
@jrose-apple anything else I can do to make this one better? |
|
Sorry I hadn't gotten back to this! I think this is pretty good. It's not a completely beautiful solution but it does mean we're parsing the same way as everything else. I have nitpicks on your comments but I'll merge it after that. |
enum raw value is parsed as a normal expression using `parseExpr()`. However, for a closure, the parser expects a local context that doesn't exist for raw values. We create a temporary context to ensure the closure gets parsed as normal. As a consequence, `parseExpr()` returns normally for closure and correct diagnosis for raw value gets issued.
|
Done. Thank you for all the help 😬. |
[SR-510] Diagnose closures used as enum raw values.
This reverts the previous fix "complain for invalid enum raw value" and fixes
SR-510 with a different approach:
enum raw value is parsed as a normal expression. When a closure is encounteredas the expression, a proper context will be absent. If we pass a diagnosis
from raw value to
parseExprClosure, it will get properly issued.enum raw value is parsed as a normal expression using
parseExpr(). However,for a closure, the parser expects a local context that doesn't exist for raw
values.
We create a temporary context to ensure the closure gets parsed as normal.
As a consequence,
parseExpr()returns normally for closure and correctdiagnosis for raw value gets issued.
@jrose-apple