[SR-7113] `try` on closures needs a clearer error message

[swift-ci]

Previous ID	SR-7113
Radar	None
Original Reporter	designatednerd (JIRA User)
Type	Bug

Attachment: Download

Environment

Xcode Version 9.2 (9C40b)

OS X Version 10.12.6 (16G1212)

swift --version output:

Apple Swift version 4.0.3 (swiftlang-900.0.74.1 clang-900.0.39.2)

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, DiagnosticsQoI, StarterBug
Assignee	None
Priority	Medium

md5: 0de4d3bda4791cbb582db3d2e00a5ef2

Issue Description:

When trying to add a try to a closure, the warning received is confusing if there is something within the closure which also needs a try. See attached screenshot for an example.

Per @jckarter, this is something which probably needs a clearer message: https://twitter.com/jckarter/status/969774890373996544

My personal suggestion for a message would be "try outside of a closure does not affect the code within the closure", but feel free to bikeshed that as much as you'd like.

[belkadan]

@xedin, do you think this is simple enough for a StarterBug? It would just be an extra note on the regular error, right?

[xedin]

Yes, I think it should be simple enough to add such a message.

[swift-ci]

Comment by Ray Fix (JIRA)

I was going to take a crack at this one. It sounds like you just want to change the value of

{{ WARNING(no_throw_in_try,none,}}
{{ "no calls to throwing functions occur within 'try' expression", ())}}

to

{{ WARNING(no_throw_in_try,none,}}
{{ "no calls to throwing functions occur within 'try' expression; 'try' outside a closure doesn't apply to the statements inside the closure", ())}}

Do we want to do something smarter where it detects presence of a throwing closure and use a longer diagnostic only in that case? It will take me more research to figure out how this can be done.

[belkadan]

Yes, I think we have to do that. (Though I would put the longer part in a separate NOTE myself.) @xedin, do you have any suggestions on the best way to check that?

[xedin]

rayfix (JIRA User) I think the only way to do that is to compute the function type of the closure expression and check if it has 'throws' flag set on it.

[swift-ci]

Comment by Ray Fix (JIRA)

Cool. Thank you for the advice. I will start looking into it.

@xedin If you know of any place in the code where something similar is done that you could point me to that would be enormously helpful.

[belkadan]

Ah, this is an error that's emitted on the 'try', not on the closure. We don't actually care if the closure throws or not, because of things like the original example where the user didn't write try inside the closure.

[xedin]

rayfix (JIRA User) I think you might need look how `CSDiag` handled diagnostics right now, check how we detect and produce the current error for missing "try" in addition to that we need to check if there is a "try" in the AST already and if it's related to a call with closure (where current expression is contained), which isn't expected to "throw".

[swift-ci]

Comment by Ray Fix (JIRA)

@belkadan @xedin Thank you. I think my first [baby] step is to setup the new diagnostic and create some [passing and failing] tests so I can see things in action and where we want to go.

[xedin]

Sounds good to me!

[swift-ci]

Comment by Ray Fix (JIRA)

Hello @xedin & @belkadan

Does this look like I am on the right track?

https://github.com/rayfix/swift/commit/8264f479e595cf60f18b46837fb05240ff44500c

[I couldn't figure out a way of expressing not to expect a note. That is sort of what I want for the negative test cases. Maybe I should have more complicated test signatures? This is sort of minimalist.]

Still I have no idea how to implement this yet but I will start poking at it. :] At least now I know how to add and run tests. 🙂

[xedin]

rayfix (JIRA User) Almost but I think it has to be a warning not just a note and replace `no_throw_in_try` when there is a closure involved which has something that throws in its body.

[swift-ci]

Comment by Ray Fix (JIRA)

Thank you @xedin Good point on the proper trigger condition, I will update the tests to do that.

Don't we want it to be a note that modifies the original no_throw_in_try warning instead of a separate warning. [I thought this is what @belkadan was saying.]

Revisiting the wording, something like:

WARNING(no_throw_in_try,none,
"no calls to throwing functions occur within 'try' expression", ())
NOTE(and_throw_in_closure_does_not_apply,none,
"throwing statements inside the closure body do not apply", ())

On the other hand, I think one advantage of making it a separate warning is that it is easier to test. I.e: It either has the warning or it doesn't. As far as I can tell, there isn't a way of testing the absence of note on some warnings.

[xedin]

I think in the situation like this one, there is no reason to emit original warning since it doesn't really give enough information about what is going on, where new warning will.

[swift-ci]

Comment by Ray Fix (JIRA)

Got it. Thanks!