Skip to content

Deprecate CastError and use TypeError uniformly instead #787

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
rakudrama opened this issue Jan 19, 2020 · 5 comments
Closed

Deprecate CastError and use TypeError uniformly instead #787

rakudrama opened this issue Jan 19, 2020 · 5 comments
Labels
request Requests to resolve a particular developer problem

Comments

@rakudrama
Copy link
Member

Developers don't typically write code that depends on the distinction between CastError and TypeError. I suspect many developers don't have a clear model of which one is thrown when. Either the code crashed or it didn't. Errors tend to be handled uniformly, for example, they are logged and then the program tries to recover from a general failure.

Having two almost identical Errors creates a burden in code size and library complexity.
dart2js has many small 'helper' functions to implement the checks efficiently.

The following code from sdk/lib/_internal/js_runtime/lib/rti.dart illustrates the duplication:

/// Specialization for 'as String?'.
/// Called from generated code.
String /*?*/ _asStringNullable(object) {
  if (_isString(object)) return _Utils.asString(object);
  if (object == null) return object;
  throw _CastError.forType(object, 'String');
}

/// Specialization for check on 'String?'.
/// Called from generated code.
String /*?*/ _checkStringNullable(object) {
  if (_isString(object)) return _Utils.asString(object);
  if (object == null) return object;
  throw _TypeError.forType(object, 'String');
}

Note that the only difference is in the error object _CastError vs _TypeError.
These methods are called from many places in the program, either indirectly through type objects representing String* and String?, or directly in optimized code.

The whole system of specialization is replicated for two exception types. Simplification via passing in an 'isTypeError' flag has an impact proportional to the application size that is unacceptable to our customers.

I propose that CastError becomes an interface that is implemented by TypeError, and removed at a future major revision.

@rakudrama rakudrama added the request Requests to resolve a particular developer problem label Jan 19, 2020
@lrhn
Copy link
Member

lrhn commented Jan 21, 2020

One issue is that TypeError implements AssertionError (for historical reasons, and people being too wary of breakage to fix it in Dart 2.0) and therefore has an unnecessary message getter. Making CastError implement TypeError would make it too an AssertionError.

If we are doing things, I would prefer that we make TypeError not an AssertionError first. Then I'm fine with merging CastError and TypeError into one and deprecating CastError.

@leafpetersen
Copy link
Member

I'm fine with this.

@leafpetersen
Copy link
Member

We agreed to do as @lrhn proposes, he will shepherd the breaking change.

@lrhn
Copy link
Member

lrhn commented Feb 12, 2020

The plan is:

I decided against making either of CastError and TypeError implement each other.
That is only necessary if someone somewhere is subclassing CastError, which does not appear to be the case.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Mar 11, 2020
This makes CastError implement TypeError, and changes all test
expectations to look for TypeError.  A followup CL will deprecate
CastError.

Bug: dart-lang/language#787
Bug: #34097
Change-Id: I7102c6260901317572d2df08c4be9c4c48197688
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138670
Reviewed-by: Bob Nystrom <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
@leafpetersen
Copy link
Member

This has landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

3 participants