[Wildcard Variables] `empty_catches` support

[pq]

Today if you want to catch and ignore an exception you can silence empty_catches by adding a comment to the catch clause,

try {
  ...
} catch(e) {
  // ignored, really.
}

With wildcards, I wonder if this shouldn't be sufficient to silence the lint?

try {
  ...
} catch(_) { }

EDIT: this is already how it's implemented but there's an open question about whether we want to start reporting on the non-wildcard (catch(e)) case.

[srawlins]

According to the details section, that already is good enough?

[lrhn]

The goal of the lint is to avoid accidentally not handling an error, forgetting to put something in the catch clause.
I think a catch (_) can be considered as a hint that it's deliberate. It's not a guarantee, but

} catch (e) {
  // Remember to handle the error!
}

is also a false negative.

A catch-all is always a code smell. There are places where it's needed, around untrusted self-contained code being run by a framework, but generally one shouldn't ignore errors. This lint is not enforcing that, it's just making sure you intended it, and catch (_) seems like a reasonable sign of intent.

[pq]

Thanks, @lrhn.

I think a catch (_) can be considered as a hint that it's deliberate.

I agree and has the advantage of a non-breaking change.

EDIT: I should have looked closer. This is ALREADY how it's implemented. (Nothing more non-breaking than a non-change!)

As for whether we should start reporting on } catch (e) { (even w/ a comment in the body), this will be more impacting (though easily remedied w/ dart fix). Since it's a core lint, it'd be great to get some more input...

/fyi @goderbauer @mit-mit @dart-lang/language-team @dart-lang/analyzer-team

(Comments and up/down votes welcome.)

[pq]

Proposal: do lint empty_catches on catch(e) even with a comment in the body

This would be breaking and would introduce new diagnostics. The rationale would be to further nudge people towards using wildcards.

[pq]

Alternatively, we could nudge users to wildcards by flagging the e in

try {
} catch(e) {
  // Unused.
}

as unused by producing an UNUSED_CATCH_CLAUSE diagnostic (#55723).

EDIT: as @bwilkerson points out, UNUSED_CATCH_CLAUSE isn't the right diagnostic here (though UNUSED_LOCAL_VARIABLE could possibly be --- #55719).

[bwilkerson]

The unused_catch_clause diagnostic is only reported if there's an on clause. Removing the catch clause when there isn't an on clause would result in invalid code. Even if the user had

try {
  // ...
} on Object catch (_) {
}

the catch clause would still be unnecessary and should still be reported by unused_catch_clause.

[pq]

The unused_catch_clause diagnostic is only reported if there's an on clause.

Oh, right! I got the diagnostics confused and was thinking about the idea of beginning to report an UNUSED_LOCAL_VARIABLE diagnostic in all but the wildcard case.

That is:

try {
} catch(e) { // UNUSED_LOCAL_VARIABLE
}

but not:

try {
} catch(_) {
}

See: #55719.

[bwilkerson]

Yes, if there's no on clause then I agree, we should report unused_local_variable and encourage users to use a wildcard.