[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!

[swift-ci]

Comment by Ray Fix (JIRA)

Hello. @xedin

Restarting work on this at try! Swift, San Jose.

One thought is that it would be nice if somehow the warning was suppressed completely in this case.

Consider the following code:

func foo(where predicate: () throws -> Void) rethrows -> Int {
{{ try predicate()}}
{{ return 42}}
{{}}}

func bar() throws {
{{}}}{{ }}

When the user writes the the code without any try statements, the compiler correctly shows an error diagnostic against bar.

let result = foo {
{{ bar() // ERROR: Call can throw, but it is not marked with 'try' and the error is not handled}}
}

It seems like if there was a fixit was generated here that put the try, try?, try! in the right place (before bar), that would be good to head off the problem in the first place. This would lead the user to the next error to put in the upper level try (assuming it is rethrow as shown).

With some help, I was investigating TypeCheckError.cpp (around line 1046)

if (reason.getKind() != PotentialReason::Kind::CallThrows)
return;

TC.diagnose(loc, diag::note_forgot_try).fixItInsert(loc, "try ");
TC.diagnose(loc, diag::note_error_to_optional).fixItInsert(loc, "try? ");
TC.diagnose(loc, diag::note_disable_error_propagation)
.fixItInsert(loc, "try! ");

Perhaps the problem that it is returning without generating the fixits. (???)

1. Do you think enabling fixits in this case would be a good thing to strive for?

If the user somehow started with this version of the call with a try? both a error and warning come are shown. (The original bug report.). In this case, I think it would be best if the warning could be repressed altogether.

{{ let result = try? foo { // WARNING: Call can throw, but it is not marked with 'try' and the error is not handled}}
{{ bar() // ERROR: Call can throw, but it is not marked with 'try' and the error is not handled}}
}

2. What do you think of making the warning go away completely and adding the fixits to the error here?

[xedin]

rayfix (JIRA User) Thanks for looking into this! I think it would be great if we could provide a fix-it in the case you mentioned, I also think that warning should stay although should be re-worded in case the scope of the `try` is a closure or call with a trailing-closure it shouldn't be too hard to figure out what is the underlying type/expr and if it's marked as throwing or not - looks like all related logic is gathered in CheckErrorCoverage class which walks AST to find if the `try` spelled out are really required or not, take a look at `checkTry` method there it already has TryExpr passed in so I think diagnose statement there is an ideal place to accommodate new case.

[belkadan]

Resetting assignee on all Starter Bugs not touched since 2018.