Consider adding invalid_runtime_check_with_js_interop_types to the recommended rule set

[srujzs]

Describe the rule you'd like to see added and to what rule set
invalid_runtime_check_with_js_interop_types is a new rule added to address runtime type inconsistencies for JS interop types available through dart:js_interop. It will trigger when users use an is or as expression involving a JS interop type that may result in either confusing or inconsistent results.

https://dart.dev/tools/linter-rules/invalid_runtime_check_with_js_interop_types (will be in Dart 3.5+)

We may want to add this to the recommended rule set.

Additional context
This is predominantly intended to address the inconsistencies between dart2wasm and the JS compilers. When switching between compilers, most of the cases this lint rule catches either are likely to end in an error in the case of as expressions or a different boolean (which is worse!) in the case of is expressions.

This rule also only affects expressions where a JS interop type is used, so it should not affect other code. We could also consider adding it to the core set based on the guidelines, but I can imagine cases where it won't result in an error or inconsistency e.g. JSString s = ...; s is Future;. It's undesirable, but not necessarily worrisome.

[devoncarew]

It looks like that commit is in 3.5-dev; we're about to publish 3.4 as stable. So this lint won't be available in a stable release for a release cycle (~3 months).

We also just published a new major version of this package, so we won't want to do a stable release for a while (~6 months). FYI in terms of the timing of this.

We may want to add this to the recommended rule set.

Just for thinking about this, will all users want this lint on? Are the issues it catches always errors? Does it have false positives? For background about how we think of core vs recommended: https://github.com/dart-lang/lints?tab=readme-ov-file#lint-sets.

[srujzs]

The timing is fine with me. We can recommend users enable this lint themselves in the interim.

will all users want this lint on?

I suppose if you don't care about compiling to Wasm, this might be noisier than you might need. Still, even in that case, I'd expect you'd want this on because it avoids confusing checks/undefined runtime specifics.

Are the issues it catches always errors? Does it have false positives?

No, not necessarily. I give one example above but generally speaking, there are plenty of examples where the code is consistent between all compilers but this lint catches because it relies on runtime specifics which we don't guarantee. Even ignoring the contrived cases (users doing an is check between JSString and Future for example), there are some false positives when it comes to things like type parameters where we don't have enough static type information and lint conservatively.

This is partly why I think core might be a bit too strict, but recommended makes sense.

[natebosch]

there are some false positives when it comes to things like type parameters where we don't have enough static type information and lint conservatively.

Can you elaborate on this?

Are we planning any opt-out mechanism outside of // ignore?

My initial reaction is this seems like something that should be in the core set, but I don't really understand the cases where someone would intentionally violate this.

[lrhn]

All in all the rules seem reasonable, but possibly incomplete.

If related means actual subtype or supertype relationship, then I think most reasonable use-cases will be possible.

Comments on the description (taken from the original commit):

DON'T use 'is' checks where the type is a JS interop type.

DON'T use 'is' checks where the type is a generic Dart type that has JS
interop type arguments.

What about function types or record types containing JS interop types?

DON'T use 'is' checks with a JS interop value.

'dart:js_interop' types have runtime types that are different based on whether
you are compiling to JS or to Wasm. Therefore, runtime type checks may result in
different behavior. Runtime checks also do not necessarily check that a JS
interop value is a particular JavaScript type.

BAD:

Is this Window type intended to come from somewhere, or should there be a @JS() marker on it?
Or is it just a random Dart extension type, not an interop type?

extension type Window(JSObject o) {}

void compute(JSAny a, bool b, List<JSObject> lo, List<String> ls, JSObject o) {
  a is String; // LINT, checking that a JS value is a Dart type
  b is JSBoolean; // LINT, checking that a Dart value is a JS type
  a is JSString; // LINT, checking that a JS value is a different JS interop
                 // type
  o is JSNumber; // LINT, checking that a JS value is a different JS interop
                 // type
  lo is List<String>; // LINT, JS interop type argument and Dart type argument
                      // are incompatible
  ls is List<JSString>; // LINT, Dart type argument and JS interop type argument
                        // are incompatible
  lo is List<JSArray>; // LINT, comparing JS interop type argument with
                       // different JS interop type argument
  lo is List<JSNumber>; // LINT, comparing JS interop type argument with
                        // different JS interop type argument
  // Not a lint, but this doesn't actually check whether `o` is actually a
  // Window.
  o is Window;

Should it be a lint?
Casting to an extension type with a JS-type representation type is basically the same as casting to the representation type. (And is checks are basically casts due to promotion.)

}

Prefer using JS interop helpers like 'isA' from 'dart:js_interop' to check the
type of JS interop values.

GOOD:

extension type Window(JSObject o) {}

void compute(JSAny a, List<JSAny> l, JSObject o) {
  a.isA<JSString>; // OK, uses JS interop to check it is a JS string
  l[0].isA<JSString>; // OK, uses JS interop to check it is a JS string
  o.isA<Window>(); // OK, uses JS interop to check `o` is a Window
}

DON'T use 'as' to cast a JS interop value to an unrelated Dart type or an
unrelated Dart value to a JS interop type.

What does "unrelated" mean? (Up-cast or down-cast only?)

DON'T use 'as' to cast a JS interop value to a JS interop type represented
by an incompatible 'dart:js_interop' type.

What does "incompatible" mean? Same as unrelated?

BAD:

extension type Window(JSObject o) {}

void compute(String s, JSBoolean b, Window w, List<String> l,
    List<JSObject> lo) {
  s as JSString; // LINT, casting Dart type to JS interop type
  b as bool; // LINT, casting JS interop type to Dart type
  b as JSNumber; // LINT, JSBoolean and JSNumber are incompatible
  b as Window; // LINT, JSBoolean and JSObject are incompatible
  w as JSBoolean; // LINT, JSObject and JSBoolean are incompatible
  l as List<JSString>; // LINT, casting Dart value with Dart type argument to
                       // Dart type with JS interop type argument
  lo as List<String>; // LINT, casting Dart value with JS interop type argument
                      // to Dart type with Dart type argument
  lo as List<JSBoolean>; // LINT, casting Dart value with JS interop type
                         // argument to Dart type with incompatible JS interop
                         // type argument
}

Prefer using 'dart:js_interop' conversion methods to convert a JS interop value
to a Dart value and vice versa.

GOOD:

extension type Window(JSObject o) {}
extension type Document(JSObject o) {}

void compute(String s, JSBoolean b, Window w, JSArray<JSString> a,
    List<String> ls, JSObject o, List<JSAny> la) {
  s.toJS; // OK, converts the Dart type to a JS type
  b.toDart; // OK, converts the JS type to a Dart type
  a.toDart; // OK, converts the JS type to a Dart type
  w as Document; // OK, but no runtime check that `w` is a JS Document
  ls.map((e) => e.toJS).toList(); // OK, converts the Dart types to JS types
  o as JSArray<JSString>; // OK, JSObject and JSArray are compatible
  la as List<JSString>; // OK, JSAny and JSString are compatible
  (o as Object) as JSObject; // OK, Object is a supertype of JSAny
}

[srujzs]

Can you elaborate on this?

Sure. Here's a quick example:

void f<T extends JSObject>(T t) {
  t as JSArray; // LINT
}

We don't know if T is a subtype/supertype of JSArray or if that cast is a sidecast (for example, if t was a JSFunction), so we lint. It may be that the user knows better and only passes Ts that are subtypes/supertypes. There are more examples where we lint conservatively in the tests.

Are we planning any opt-out mechanism outside of // ignore?

I don't have any plans to add a flag if that's what you're alluding to.

My initial reaction is this seems like something that should be in the core set, but I don't really understand the cases where someone would intentionally violate this.

I think it's going to be rare for users to write code that will come across false positives.

---

If related means actual subtype or supertype relationship, then I think most reasonable use-cases will be possible.

Yep, that's what "related" means here. I don't love the terminology but I was trying to avoid being too verbose. Perhaps there's a better word that can be used here?

All in all the rules seem reasonable, but possibly incomplete.
What about function types or record types containing JS interop types?

These indeed should be lints, but we don't have code that handles Functions in canBeSubtypeOf yet. Perhaps I should mention not to do such checks anyways. It does have code that handles record types, though, and I should also mention not to do such checks (even if they may be rare). I was mostly worried about the details getting too long/mentioning bad cases that don't actually get linted yet.

Is this Window type intended to come from somewhere, or should there be a @JS() marker on it?
Or is it just a random Dart extension type, not an interop type?

It's not a requirement for JS interop extension types to contain that annotation as we determine based on their representation type if they're an interop type (which here, JSObject tells us it is).

Should it be a lint?

It is. :) See the next CL in the relation for where it's added and this message is modified: https://dart-review.googlesource.com/c/sdk/+/364167.

What does "incompatible" mean? Same as unrelated?

Yes, but I was aiming to differentiate a little by using incompatible for the underlying JS types (under however many layers of extension types) being unrelated. It's maybe better to stick to just one word to avoid confusion.

[lrhn]

Is there a reason to disallow side-casts casts specifically on JS-types, but allow up/down-casts? (That's casts to other JS-types, casting a JS-type to a non-(extension-on-)JS-type should probably always be a warning.)

Knowing that all JS-types form a tree would be a reason, it would imply that a side-cast is always going to throw.

If that is not the case, a side-cast isn't more dangerous than a down-cast. Either can be wrong, and either can be right, and the failure mode isn't significantly different.

If the reasoning is that "side-casts are more likely to be wrong" and "wrong casts are more damaging for JS-types", then maybe it does mean that warning about side-casts for JS types is, all-in-all, more warranted than for non-JS-types. I can buy that reasoning too, but we might want to say explicitly that that is the reasoning.

(I could get behind a "no-side-cast" lint in general, but otherwise I'd want some argument for treating JS-types differently from non-JS-types.)

[srujzs]

Is there a reason to disallow side-casts casts specifically on JS-types, but allow up/down-casts?

Sidecasts always succeed when running with dart2wasm (JS types are just different extension types on the same underlying class), but fail using the JS compilers, so we statically know that it will always do the "wrong" thing in one compiler. With downcasts, we don't statically know that, and we don't want to get in the way of a legitimate downcast e.g. JSAny -> JSObject. Upcasts are naturally always okay.

The effect is ultimately the same between downcasting to the wrong type and sidecasting to another JS type: you'll see an error when using the JS compilers and but not when using dart2wasm.

Casting to unrelated Dart types i.e. not Object/dynamic has the reverse effect. They may succeed with the JS compilers, but will always fail with dart2wasm.

otherwise I'd want some argument for treating JS-types differently from non-JS-types

I suspect you're referring to the above discussion about incompatible vs unrelated. I'm okay saying we don't need two different words for the same underlying issue, even though they may present differently.

[kevmoo]

We should put this in the queue for Right when Dart 3.5 releases, correct?

[devoncarew]

From an off-line discussion:

• this will help in correctness and avoiding runtime errors with code targeted wasm
• we do want this lint enabled to help people as they convert their interop code over to dart:js_interop / supporting wasm
• adding this lint is technically a breaking change (adding a lint makes this lint set more restrictive which will cause CIs to fail); we introduce new lints in new major package versions. We published recently so wouldn't publish a new major version for another release cycle (~3 months). However, if we think that the lint won't really trigger on existing code - is not effectively a breaking change for most existing code - we could choose to publish as a minor release (4.1)

resolution: this is ok to add to recommended; we want to gather more data on the external impact of this in order to help us decide whether to ship as a 4.1 version or a new major version.

[kevmoo]

It will almost CERTAINLY cause CI failures for folks. I'd love it out soon...but also in a major version

[kevmoo]

Well...considering that it'll only affect folks using the new JS-interop bits, I'd love to validate it first. It's certainly good to get it out sooner rather than later.

[devoncarew]

It will almost CERTAINLY cause CI failures for folks. I'd love it out soon...but also in a major version

Do you have links or examples where you've seen this?

[kevmoo]

I know that flutter is showing these hints when this lint is enabled.

flutter/flutter#151010

[devoncarew]

Related: dart-lang/sdk#56396

[devoncarew]

An update: lets add to a 5.0.0-wip version of this package, land the lint in package:dart_flutter_team_lints (dart-lang/ecosystem#285) to give us some dogfood time, and plan to ship with the next major release of this package (~3 months, give or take).