-
Notifications
You must be signed in to change notification settings - Fork 1.7k
proposal: runtime_checks_with_js_types
#59366
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
Comments
FYI @eernstg @sigmundch I don't think any of the content here should be surprising, I'm just filing an issue to keep track. |
Sounds good! I noted a couple of things: In the section about good examples:
I guess that should be At the end there's a "down-check" from Would it be useful to lint this kind of type test and type check? The point would be that checking |
Yes, thanks! :)
I presume you're referring to the bad example where we're checking whether a variable of type
I again presume you're referring to the bad example. I believe so, for the same reason you've mentioned where the underlying runtime type is not consistent and on dart2wasm, the type can represent many different values. The canonical way to check the type of the JS value is to use a JS mechanism, like an |
Sounds like we really want to have a lint called |
Closes https://github.com/dart-lang/linter/issues/4841 Adds a lint to verify and report whether an is check or a cast may result in platform-inconsistent behavior. In order to do so, adds necessary additions to canBeSubtypeOf and extends the mock SDK. Also adds this to the list of default rules. Change-Id: I7f235ee698419d9f253cc96f08322bedca271825 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/361722 Reviewed-by: Sigmund Cherem <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]>
This has landed under the lint rule |
runtime_checks_with_js_types
Description
These are lints that will make it easier to write and use JS interop code such that it can be deployed on dart2wasm without a divergence in semantics.
Details
Due to the difference in runtime types of the extension types ("JS types") we expose in
dart:js_interop
, casts and checks usingis
andas
will almost certainly give different behavior depending on the platform.A simple example is
JSString
. On dart2wasm, the representation type is aJSValue
, while on the JS compilers it isString
.I go into a little more detail on this here: #59310. I chose to file a separate proposal, because even though there is heavy overlap, that proposal might not be sufficient.
Kind
Guard against errors and support platform compatibility.
Bad Examples
true on JS backends, false in dart2wasm. The reverse case is the same result.
Always true on dart2wasm, only true if the value actually is a JS string value on the JS backends. Note that this may differ from the proposal in #59310, as this
is
check doesn't introduce or eliminate an extension type as it's a "downcheck" between two extension types.Depends on the initialization of
x
! #59310 does catch instances where we eliminate the extension type, so that'll combat the frequency of such checks. I think we should lint on this (whereas maybe not with a cast?) as this is likely to do more harm than good.This is always false on both compilers, but we can still lint, telling users they shouldn't do an
is
check where a JS type is compared against a non-JS type.This is a "downcheck" so we should likely lint. However, the reverse e.g.
JSString is Object
should probably not be linted, as that's statically true sinceJSString <: Object
.Good Examples
Add a few examples that demonstrate a “good” adoption of this lint’s principle.
This is the reverse of the check above. This is okay because this is statically true as
JSString <: JSAny
. In the implementation, we should actually verify that though.Comparisons between JS types where the type is the same should be fine. The above check is not trivial either, as there is a useful check being done (
List
is
aCustomList
).Checking for nullability is interesting. I think this should be okay as the nullability should not be platform-dependent.
Almost all the
as
cases are equivalent to theis
cases, except for this one. This shouldn’t be a lint as it’s a legitimate downcast. If users know thatJSAny
is a JS string value, a cast is safe and the only way they can get aJSString
. It may lead to different runtime semantics if users aren't careful, but we don't want to be in the way of a legitimate cast. Analysis with generics is similar e.g.List<JSAny> as List<JSString>
is okay.Discussion
We may choose to extend these lints further to suggest to users that they should use a conversion function when the intent is obvious. For example,
String as JSString
is an obvious candidate where we should tell users to use.toJS
fromdart:js_interop
instead (andtoDart
in the reverse case). This would require a bit more inspection on the types involved.In general,
is
is more dangerous and consistently wrong thanas
, so we may relax linting on casts versus checks.The text was updated successfully, but these errors were encountered: