Skip to content

[Breaking change request] Make TypeError not extend AssertionError #40317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lrhn opened this issue Jan 24, 2020 · 8 comments
Closed

[Breaking change request] Make TypeError not extend AssertionError #40317

lrhn opened this issue Jan 24, 2020 · 8 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes library-core NNBD Issues related to NNBD Release

Comments

@lrhn
Copy link
Member

lrhn commented Jan 24, 2020

Change:
We will make the platform TypeError class not extend/implement the AssertionError class.
That will also make it not inherit the message getter.

Reason for Change:
It is a legacy hold-over from Dart 1 that type errors (thrown by failing type annotations) are assertion errors (thrown by failed assertions in debug mode), because in Dart 1 type annotations were only checked in debug mode.

When we added the message operand to assert statements, we also added a message field to AssertionError. Then TypeError inherited that field, even though it had no need for it.

We plan to change which operations throw TypeError and CastError (most likely by combining the two and only have one error type), and that would then cause cast errors to also be assertion errors. So, before moving forward with any other error changes, we want to detach TypeError from AssertionError.

An alternative would be to simply deprecate TypeError and just use CastError for everything, but TypeError has the better name for being a general "not the type expected" error because Dart doesn't/won't have implicit casts.

Expected Impact:
You should generally not be catching errors, and if you do, it should be for generic handling like logging. Your code should not depend on which particular Error is thrown.

That doesn't hold for test code, where you very often do want to check that the error that occurs is the one that you expect.
If such code conflates TypeError and AssertionError by only expecting an AssertionError for code which triggers an assert in debug (assertions enabled) mode, but a type error in production mode when a wrong value gets past the disabled assertion, then that test will start failing when a TypeError occurs.
The Dart SDK itself has tests like this.

However, the impact is expected to almost exclusively affect tests, not production code, which gives the affected developers time to fix the tests.

Mitigating the Impact:
If your code starts failing because you expect an AssertionError and get a TypeError, then start expecting a TypeError.

Look at all places where you have try { ... } on AssertionError { ... } (remembering that catching a specific Error is discouraged in general, so there hopefully won't be many cases of this), and
consider whether it needs to handle TypeError instead or as well.

Look at places where your tests do expect(..., throwsAssertionError) and consider whether it should catch TypeError instead or as well.

If you have code which uses the message field of TypeError, then that will start failing, and it should stop doing so now. The message was a fixed text in the VM implementation, so it wasn't particularly useful.

@lrhn lrhn added breaking-change-request This tracks requests for feedback on breaking changes analyzer-api Issues that impact the public API of the analyzer package labels Jan 24, 2020
@srawlins srawlins added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core labels Jan 27, 2020
@lrhn lrhn removed the analyzer-api Issues that impact the public API of the analyzer package label Jan 27, 2020
@lrhn lrhn changed the title BREAKING CHANGE: Make TypeError not extend AssertionError [Breaking change request] Make TypeError not extend AssertionError Feb 12, 2020
@franklinyow
Copy link
Contributor

cc @Hixie @matanlurey @dgrove for review and approval.

@matanlurey
Copy link
Contributor

Seems fine

@vsmenon
Copy link
Member

vsmenon commented Feb 12, 2020

@lrhn - when we unfork, developers will start seeing CastError in a few places where they used to see TypeError, right?

If so, shall we combine both your bullets here:

dart-lang/language#787 (comment)

into this breaking change?

Otherwise, LGTM.

@Hixie
Copy link
Contributor

Hixie commented Feb 13, 2020

There's logic in Flutter (at runtime in release builds) that specifically looks for AssertionError in some cases, but I doubt it'll be affected by this, so LGTM.

@franklinyow franklinyow assigned lrhn and unassigned franklinyow Feb 13, 2020
@franklinyow
Copy link
Contributor

Approved

@lrhn
Copy link
Member Author

lrhn commented Feb 17, 2020

@vsmenon Would it be easier to combine the two changes (TypeError is! AssertionError, CastError is TypeError) in one change than do them as two changes?

My guess is that the error situations are independent of each other, so there will be no work saved by combining them, it will just be a bigger chunk of work.

There might be a few cases where a test is changed from is AssertionError to is TypeError as part of the first change, and that check then starts catching CastError as well with the second change. I don't expect it to happen much, if at all.

@lrhn
Copy link
Member Author

lrhn commented Feb 17, 2020

@vsmenon
Copy link
Member

vsmenon commented Feb 18, 2020

@lrhn @franklinyow - I'm fine either way. LGTM.

@mit-mit mit-mit added the NNBD Issues related to NNBD Release label Feb 19, 2020
sjindel-google added a commit to flutter/flutter that referenced this issue Feb 24, 2020
TypeError no longer implements AssertionError after dart-lang/sdk#40317.
sjindel-google added a commit to flutter/flutter that referenced this issue Feb 24, 2020
dart-bot pushed a commit that referenced this issue Mar 18, 2020
Boolean conversion now throws a TypeError, not CastError. See:
* #40317
* #40763

Presubmit: https://critique.corp.google.com/#review/301195917

Change-Id: I76e8aa4a849eb519e47c80f1b4873032a05ad636
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/139402
Reviewed-by: Nicholas Shahan <[email protected]>
Commit-Queue: Mark Zhou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes library-core NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

7 participants