Repository navigation
[flutter_tool] Where possible, catch only subtypes of Exception - #51440
Conversation
zanderso
added
tool
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
Overall change LGTM
Is there a lint (or regular expression) we could use to prevent us from regressing?
This comment has been minimized.
This comment has been minimized.
|
@zanderso Could we enable the lint and ignore the specific exceptions we need? |
I'll investigate that. |
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
a lot of nits, getting rid of on Exception catch (_) is not that important, but if we use finally rather than catch -> rethrow, enabling the lint would be a lot easier.
There was a problem hiding this comment.
nit. we don't need to catch unused Exception
There was a problem hiding this comment.
nit we don't need to catch unused Exception
There was a problem hiding this comment.
sp cathcing -> catching
There was a problem hiding this comment.
can this just be a finally rather than catch-rethrow
There was a problem hiding this comment.
finally over catch-rethrow?
There was a problem hiding this comment.
finally over catch-rethrow?
There was a problem hiding this comment.
The finally clause would run even if no exception is thrown, which would change the semantics.
There was a problem hiding this comment.
nit no need to catch unused Exception
There was a problem hiding this comment.
nit no need to catch unused exception
There was a problem hiding this comment.
nit no need to catch unused exception
|
@christopherfujino Thanks for the thorough review. I enabled the lint as well. |
Cool, I think this is a good change! I suspect we'll see a lot of things surface after this. |
4 participants
Description
The pattern:
is problematic because the
catchclause will catch not only exceptions resulting from usage and system errors (FileSystemException,SocketException, etc.), but also those resulting from programming errors (TypeError,AssertionError,NoSuchMethodError, etc.). In particular, the programming errors should be surfaced by tests and/or crash the tool with a report to crash logging.This PR removes the above pattern. Unfortunately, I don't think we can enable the analyzer lint to ban it since there are a few places where it is needed, which I've noted.
Related Issues
#8742
Tests
I added the following tests:
A few erroneously passing tests fixed.
Breaking Change
Did any tests fail when you ran them? Please read [Handling breaking changes].