[SR-978] Warn on do {} with only one empty catch {} block

[jckarter]

Previous ID	SR-978
Radar	None
Original Reporter	@jckarter
Type	Improvement

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, StarterBug
Assignee	MultiColourPixel (JIRA)
Priority	Medium

md5: d5f41243d2aaf067f1ecd57198c44970

Issue Description:

Eric Knapp caught some sample code in the wild unwisely using an empty catch block to swallow errors:

https://twitter.com/ejknapp/status/710589235464523776

We should warn on this to encourage people to use try! to trap on unexpected errors instead.

[belkadan]

How do you silence the warning?

[jckarter]

Morally, if you don't expect any errors to happen, you should use try! to assert that. Practically, if you really want to throw away errors, there's _ = try?.

[belkadan]

Yes, but what about "I want to catch this particular error, but ignore any others?" ("I want to ignore this particular error" is easy enough to special-case.)

[jckarter]

I see what you're saying. IMO, if you do that, we should let it pass:

do {
  try something()
} catch Foo.Error {
  handleFooError()
} catch {
  /* do nothing */
}

IMO, it's only the case where you do nothing more than do { ... } catch {} that deserves a warning.

[swift-ci]

Comment by Josef Willsher (JIRA)

I’m happy to implement this. How should we handle cases where the do block contains more than 1 expression—we can't issue a fixit here, but should I still emit a warning?

[jckarter]

If the do block contains multiple statements, you should still be able to change every try to try! or try? inside it.

[swift-ci]

Comment by Josef Willsher (JIRA)

try? has different semantics to try though — you won't be able to just interchange them in an expression like foo(a: try getA()).

Are you sure its not too much compiler gymnastics for a fixit to change a whole scope's try expressions to the trapping try!? I think it is reasonable to do for a single expression, but if the do block contains a significant amount of code, surely the better alternative is to get the user to rethink it themselves?

I already have it emitting a warning for a single, empty, unguarded catch block, and I’m happy to implement the fixit either way.

[jckarter]

Sure, it might be reasonable to just warn without fixits if the `do` block is too complex.

[belkadan]

Resetting assignee for all Starter Bugs not modified since 2018.