[SR-510][Parser] complain for invalid enum raw value

[dduan]

Instead of fail silently, issue a diagosis when a valid expression can not
be parsed for enum raw value.

Resolves SR-510.

[jrose-apple]

This seems reasonable, but (a) is it possible to use the loc of the first token after the equal sign instead, and (b) what happened to the other issue about parsing a closure? That still seems like a better fix for this particular unparseable expression.

[dduan]

Hi @jrose-apple,

(a) is it possible to use the loc of the first token after the equal sign instead

done.

(b) what happened to the other issue about parsing a closure? That still seems like a better fix for this particular unparseable expression.

I agree with your verdict. #934 could fix a number of unforeseen issues (wherever a closure is defined without a context). That said, this approach has a similar benefit: it catches all cases where the parseExpr fails. There could be more besides closure. IMHO this PR addresses SR-510 more directly.

[dduan]

I think we can just reuse the regular diagnostic (nonliteral_enum_case_raw_value). That's what they need to correct to anyway.

Didn't notice this one util now. Done.

Do we know of anything else that will trigger this error?

None to the best of my knowledge.

I would guess the reason we didn't diagnose anything here before is because anything else would get its own diagnostic first anyway.

That makes an argument for this type of catch-all fix IMO. It's always hard to prove something doesn't exist 😛.

(it's still valuable to find all edge cases. If the context-less closure parsing causes other bugs, we would fix it then).

@jrose-apple

[lattner]

This looks good to me. Please email swift-dev to see if Ted will approve pulling this into Swift 2.2, thanks!

[jrose-apple]

I'd argue that this diagnostic should be an assertion, then, not a separate diagnostic. In the common case here, we'll get duplicate error messages, and this one doesn't add any value.

[dduan]

Fair. To avoid the duplicate message, we could do one of the following:

1. Let parseExpr know it's working on a raw value. Give closure parser a context somehow.
2.  use parseExrBasic instead of parseExpr. In theory, this will skip closure parsing.
3. Detect the closure pattern outside of parseExpr. This might duplicate a little bit of code.
   I'll try all these and report back.

• Daniel Duan

On Fri, Jan 15, 2016 at 8:41 AM -0800, "Jordan Rose" notifications@github.com wrote:

I'd argue that this diagnostic should be an assertion, then, not a separate diagnostic. In the common case here, we'll get duplicate error messages, and this one doesn't add any value.

—
Reply to this email directly or view it on GitHub.

[jrose-apple]

I really just want to parse the closure successfully and then error successfully, as discussed in #934.

[jder]

@dduan Are you still working on this? If so, let me know and I will assign the bug to you.

FWIW, I think this can also happen for at least a couple other cases:

#if {}

and

typealias foo = (Int = {})

[dduan]

Hi, @jder. @jrose-apple has been helping me with #987. But from what you posted here, that fix may not be general enough.

[jder]

@dduan OK, I'm going to assign you the bug and stop working on it, then. My WIP is here, if you'd like to grab anything from it: master...jder:parse-closures-dummy-localcontext. It catches some cases but still not all of them.