dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
630 stars 170 forks source link

proposal: `cast_into_extension_type` #4760

Open srawlins opened 1 year ago

srawlins commented 1 year ago

cast_into_extension_type

Or some name with "introduces extension type."

Description

From this comment:

Ideally, they would flag any construct that performs a dynamic type test on an expression e and yields an expression whose type is an extension type which is not the static type of e nor a supertype thereof. That is, anything which is a cast or a promotion "into" an extension type.

We need to consider types where extension types occur as subterms as well. For instance, if E is an extension type then List<E> is relevant as well, and so is E Function(Map<E, String>), etc.

So let's spell out this concept in more detail:

We say that a type T contains an extension type E in the case where E is an extension type and T is a term that contains E as a subterm. We say that a type T contains a covariant extension type in the case where T contains an extension type E that occurs covariantly, and similarly for contains a contravariant extension type and contains an invariant extension type.

We say that the transition from a source type S to a destination type T introduces an extension type E if:

  • T contains a covariant or invariant extension type E, and S is not a subtype of T nor a supertype of T. For example, a transition from int to E, where E is an extension type with no subtype relationship to int.
  • T <: S, T contains a covariant or invariant extension type E, and the proof of T <: S contains a subproof of the form E <: U where U != E. For example, a transition from Iterable<dynamic> to List<E>; or a transition from Object? Function(int) to E Function(int).
  • T <: S, T contains a covariant or invariant extension type E, and the proof of T <: S contains a subproof where the structure of the types eliminates the type E. For instance, a transition from Object to List<E>; or a transition from Function to void Function({String Function(E) callback}).

Note that an upcast never introduces an extension type, even in cases like the transition from void Function(Object?) to void Function(E) where E is an extension type. The point is that no object will be treated as an instance of E which wouldn't have been treated as such before the transition. For example, in the simple case ((Object? o) => e) as void Function(E), the body e still treats the argument as having the type Object?. Each call site must new provide an argument with static type E, but it's a separate issue how we managed to create an expression with that type.

With that, we can list cases that are candidates to be linted:

  • e as T, where the transition from the static type of e to T introduces an extension type.
  • e is T, where the transition from the static type of e to T introduces an extension type.
  • on T in a try/catch statement, where T contains an extension type.
  • Matching against an object pattern whose type T contains a covariant or invariant extension type (in any context where a pattern can occur), in a situation where the transition from the matched value type to T introduces an extension type.
  • Implicit casts from dynamic to a type that contains an extension type.

Finally, the lint message should be omitted in the case where the extension type which was introduced has a direct or indirect superinterface which is a non-extension type. For example, extension type E(T t) implements S {...}, where S is a non-extension type.

You could say that implements S is a way for E to announce to the world that every object whose dynamic type is the representation type of E or a subtype thereof is an appropriate representation object for E. In contrast, an extension type that does not implement any other non-extension types than the default ones (top types for some extension types, Object for others) provide a soft kind of encapsulation in that we're not supposed to cast or promote or match our way into those extension types, and that's exactly what the lint is supposed to detect.

Kind

Does this enforce style advice? Guard against errors? Other?

I think guarding against errors.

Bad Examples

Good Examples

Discussion

Discussion checklist

[Edit by eernstg, Oct 1 2023: Changed the example involving Iterable<int> to use Iterable<dynamic>: The example wouldn't trigger the lint because we could never have E <: int unless E were an extension type with implements int, and in that case the lint wouldn't apply anyway. With dynamic, the example actually works.]

lrhn commented 1 year ago

For all of these "bad" examples, I think it's a bad idea to get between the user and their types. If someone does a type check or type cast, they're asking for it. Literally. And a downcast from dynamic is the most essential way to ask the analyzer to get out of the way. Don't try to get in the middle of that.

About the bad examples:

That seems useful to me. I'd be more worried if E was not a subtype of int.

If I have an extension type FancyString(String _) implements String { ...more...methods...}, I'd want to take an Iterable<String> and cast it to List<FancyString> - assuming I know it's a list, but that's no different from casting Iterable<String> to List<String>, or Iterable<num> to List<int>.

A downcast from List<String> to List<FancyString> is one which cannot fail, and which provides me with benefits. Don't try to stop me from doing that.

The "fancy string" use-case, adding more members on an existing type, is an expected use-case for extension types. Casting from the representation type into the extension type is expected in that case. Sure, you can do FancyString(string), but if you get a List<String> from someone, and want to make your own life easier, you just do as List<FancyString> (a no-op at runtime) and carry on.

That's more as than is, though, but you can get promotion by using is.

I'd want to do:

dispatch(Object o) {
  if (o is FancyString) {
     doStuff(o.fancyMethod());
  } ...
}

instead of doing is String, which we won't complain about, and the ... do something more to get into the FancyString

That's more tricky. The use case above is about enhancing values with more members, this doesn't really feel like the same thing. Could be reasonable to warn about. But could also just get in the way of someone's reasonable use.

If we allow as List<String>, we really should allow as List<FancyString>.

So maybe this is about the kind of extension type. Not all extension types are the same.

Let's say that an extension type which implements its representation type is called "transparent".

Then it's OK to cast into a transparent extension type, because it's really no different than casting into the representation type, you can just add some more methods along the way. It's not trying for encapsulation (no matter how badly extension types does that otherwise).

Maybe it's OK to warn about non-transparent extension types, which don't directly advertise their representation type. But still, if the user did write as, they're telling the language that they know what they're doing. How can we claim to know better?

That's a ...counting on fingers... covariant occurrence of E.

If that's the type you want, and you're going to call the result of that cast with a String Function(E) as argument, then it's required to be that cast. Using the representation type instead doesn't work:

  String callback(FancyString input) => input.fancySummarizeString();
  if (function is void Function({String Function(String) callback})) { // Using `(String)` instead of `(FancyString)`.
     // Compile-time error, `String Function(FancyString)` is not assignable to `String Function(String)`.
    function(callback);
  }

I need to cast to that particular type, containing the extension type, to make the static typing of my code work out. I know, I could do:

  String callback(FancyString input) => input.fancySummarizeString();
  if (function is void Function({String Function(String) callback})) { // Using `(String)` instead of `(FancyString)`.
    function(callback as String Function(String)); // Works, and always safe.
  }

but why should I?

(That's a try/on clause (you can't use them in mixin on clauses).

I'd go with the same rule as above. If the extension implements Error, I don't think it's a problem to do on FancyError catch (e) { e.fancyLogThisError(); }. If not, I still don't see the problem.

A pattern match with an extension type. Again:

case Message(:FancyString text): text.fancyLogThisText();

seems reasonable.

Definitely shouldn't warn about that. Downcast from dynamic is working as intended

All in all, I don't see a problem with casts to, or promoting type-checks into, extension types. The author is asking for it.

I'm more worried about casting out of an extension type, but that's also much harder to check for. (I think.) And still, if the author writes a cast or type check, maybe they do know what they're doing.

I just don't see which problems this lint would protect against. I don't see which errors it guards against.

If an extension type wants people to always go through its public constructors, that's a separate issue. Possibly something they need an annotation to opt in to, @opaque extension type Foo._(T _) { ... }, and then the analyzer or a lint can warn if you cast into or out of that extension type, including any covariant occurrences of the type in what you cast to, even for a downcast from dynamic. (Up-casting should be fine.)

A completely general ban on casting into or out of extension types seems far too strict. There are too many use-cases, including those which are just "alias with more member", where the warning would be misplaced.

eernstg commented 12 months ago

@lrhn, I think we agree on many things here!

One point where we seem to disagree is this one:

If someone does a type check or type cast, they're asking for it. Literally. .. Don't try to get in the middle of that.

I'd agree on that for every reified type (that is, roughly: every non-extension type):

The type check (as) or the type test (is) will faithfully determine whether the requested typing is justified by the given object at run time. It doesn't matter how this information was lost in the source program, but somehow we're dealing with an instance o of a class C and the statically known type is some supertype, e.g., B or Object?, and a type test/check will allow us to recreate the lost type information (e.g., o is C), or some of it (e.g., o as B where the static type was Object?).

The type test and type check is justified with a reified type because it will provide access to information which was present at the creation of the object, and which was chosen specifically (intentionally) by the developer who wrote the expression that gave rise to the instance creation.

In contrast, an extension type can be imposed on an object without any justification in the history of the object:

extension type Inch(int it) {
  Inch operator +(Inch other) => Inch(it + other.it);
}

extension type Cm(int it) {
  Cm operator +(Cm other) => Cm(it + other.it);
}

extension on int {
  Inch get inch => Inch(this);
  Cm get cm => Cm(this);
}

void main() {
  var x = 1.inch;
  var y = 1.cm;
  var z = x + (y as Inch);
  print('The sum is $z inches!');
}

In this example we're using a type check y as Inch to access y as having the type Inch, and it will succeed at run time. However, this behavior belies the provenance of y: There's no justification in the prior execution that brought about the value of y which justifies the typing as Inch, it actually used to be a Cm which is very much not the same thing.

The crucial point is that extension types are not reified, and this means that there is absolutely no way to detect whether or not they are justified by the given object itself, they must be part of the static type which is used to access that object. You could say that "OK, this just means that extension types are broken", but I'd prefer to say that this trade-off can be made with open eyes, and it can be legitimate: Do you want to pay (in terms of time and space) for a reified representation of this type, or do you want to save the resources and then rely on information which is only present in the static types?

If we do make the choice to save the resources and use a compile-time only type then the static analysis should help us by flagging the situations where that static typing discipline is known to be ignored (such as o is E and o as E and case E() where E is an extension type).

Another way to say this is that a cast/test/match to an extension type cannot be relied upon to restore true information which has been lost. In contrast, such a cast will proceed to confirm typings that are not justified at all by the history of the given object.

I'd strongly recommend that we do support maintaining this distinction, and we do inform developers who are using type tests, type casts, or pattern matches into an extension type that they're inventing this type, not confirming it.

That said, I'd also recommend that we drop this kind of protection for extension types which have been declared to be related to non-extension types: With extension type E(int it) implements int {...}, we should not lint 1 as E, because E has already announced to the world that it's just a fancy int (and we take that to mean that any int can be an E). (This has been included in the proposal from the very beginning, so there's nothing new about that.)

About the bad examples:

  • casting from Iterable to List where E <: int, using is or as

Ah, that was a bad example, I corrected it. It is indeed the situation where E is not related to int which is considered bad.

A downcast from List<String> to List<FancyString> is one which cannot fail, and which provides me with benefits. Don't try to stop me from doing that.

No problem, surely we'll have FancyString <: String, and there will not be any warnings about that.

  • casting from Object to List<E>, using is or as If we allow as List<String>, we really should allow as List<FancyString>.

Same situation, no warning here assuming that FancyString <: String, no problem.

  • Implicit casts from dynamic to a type that contains an extension type. Definitely shouldn't warn about that. Downcast from dynamic is working as intended.

I disagree: The downcast from dynamic works exactly like all other downcasts, except that it's implicit and hence even harder to avoid in cases where it has an undesirable semantics. A downcast like (1.cm as dynamic) as Inch is not a bit safer than 1.cm as Inch.

The problem is still the same: A downcast from any type, including dynamic, to an extension type does not recover true information based on the given object. Rather, it invents information out of thin air, and we have no way to ascertain that this information is justified by the history of the given object.

It is equally reasonable to flag this situation when the cast goes from dynamic as it is when it goes from any other type.

I don't see which errors it guards against.

x + (y as Inch) may not throw at run time, but it might cause a spacecraft to crash into Mars. I'm not saying that we can't pay for the extra time and space needed in order to use a reified version of this typing distinction (that is, a real wrapper object) if we're actually considering a space mission. But I also don't see why we should refuse to give a heads up in the case where someone has (legitimately, I'd assume) chosen to avoid the real wrapper object, and rely on static types alone to make this kind of distinction.

lrhn commented 12 months ago

I don't believe we can generalize any of the reasonings here to all extension types, which is why we should be very careful about stating that all casts are inherently dangerous. Some might be useful, intended or even necessary.

I can see the problem with Inch and Cm. Those are types where being the type itself is an important information, which is lost when you cast away, even in an up-cast to int.

Not all extension types are like that.

Will have to make a number of exceptions, which may be "only casts to extension types where there is a sub/super-type relation between the two types."

We shouldn't complain about switching on extension types of the matched value type is a supertype of the case types. It's a downcast to an extension type, but it's a downcast from an extension type too, and the two are related, so it's probably deliberate.

That allows something like:

extension type Opt<T>._(({T value})? _) {}
extension type const Some<T>._(({T value}) _) 
    implements Opt<T>{
  const Some(T value) : this._((value: value));
  T get value => _.value;
}
extension type const None._(Null _)
   implements Opt<Never> {
  const None() : this._(null);
}
const none = None();

Opt<R> invokeOpt<R, T>(Opt<T> maybe, R Function(T) compute) => switch (maybe) {
  Some(:var value) => Some(compute (value)),
  none => none
};

This code has switches that cast to an extension type if successful, but that's ok.

A seemingly random side-cast it's more likely to be a problem. But I'm not convinced that it's always going to be a mistake to cast from a representation type to the extension type, especially not in nested types. Doing listOfInts as List<Inch> may be the most correct and idiomatic way to get a list of inches from a list of integers representing inches.

eernstg commented 12 months ago

we should be very careful about stating that all casts are inherently dangerous.

I don't think we have any proposals like that. The proposed lint would flag locations where an extension type is introduced by a type cast, type test, or pattern match, and that extension type does not have a non-trivial non-extension type superinterface; plus the higher-order cases where a similar re-typing occurs in subterms of the type.

For example, with extension type FancyString(String it) extends String {...}, there would never be a warning at all.

"only casts to extension types where there is a sub/super-type relation between the two types."

We could try to do such things, but I'd still prefer that we don't. First note that the proposed lint shouldn't give any warnings for any upcast, ever. Next, consider the following example:

extension type E1(C it) {}
extension type E2(C it) implements E1 {}

void main() {
  var e1 = E1(C());
  var e2 = e1 as E2; // LINT
}

In this situation, the lack of justification for considering the given C instance of type E1 as having the type E2 is just as serious as the lack of justification for C() as E2: There is no evidence whatsoever to suggest that E2 is an appropriate typing for that instance of C, and the maintainers of E1 will not in general have the opportunity to vet E2 in order to say that this re-typing is appropriate.

However, if we use implements ANonExtensionType as a signal that the extension type is "open" then it applies to all subtypes:

extension type E3(C it) implements C {}
extension type E4(C it) implements E3 {}

void main() {
  var e3 = E3(C());
  var e4 = e3 as E4; // No lint.
}

The point is that the author of E3 said "this type is open", and the author of E4 knows that this property is transitive, so everyone who is maintaining these declarations will know that the type is considered to be open, so they had better write the implementation such that it is OK for an arbitrary C to be the representation object of an expression of type E4.

That allows something like [Opt example]

That's a very interesting example!

Basically, the dynamic type test makes sense because there is a parallel subtyping relationship for the extension type and for the corresponding representation type.

That might be a special case where it would be perfectly OK to perform a dynamic type test/check and rely on the result.

However, I can't immediately see how we could make an exception for that kind of typing relationship such that it wouldn't be linted, unless we introduce a whole new rule based on non-trivial subtyping "in parallel" for the extension type and the representation type.

I wonder how common it would be to have that kind of parallel subtyping relationships.....

srujzs commented 12 months ago

This is a really useful feature for JS interop, so it'd be great if we can get this or an annotation-driven version of this.

We have "JS types" in dart:js_interop that we provide to users to be able to interact with JS object and type their external members. This is intended to provide a static interface with which to interop in a platform-independent way. This last part is important because it affects the way we represent these types. Currently, we use some custom annotation logic to emulate extension types, but in the near future, when extension types are no longer under a flag, I expect the definition for a JS string to look something like:

extension type JSString._(_JSStringImpl value)  /* no implementing JSStringImpl! */ {
  external int get length;
  // Other JS string members.
}

The _JSStringImpl is the key part of this. On dart2js and ddc, this is a typedef to a String. This is because all Strings are JS strings. On dart2wasm, this is typedef of a JSValue, a box around an externref. This is an issue, because users may do things like '' as JSString, and it will work on dart2js and ddc, but fail in dart2wasm. This is not unique to string values - every single JS type has a different representation type depending on the platform. The right way to convert from Dart types to JS types (and back) is using the extension conversion methods we provide e.g. toJS and toDart.

This isn't unique to the case where we enter an extension type either, exiting out of an extension type is also problematic e.g.

JSString str = ...;
str as String; // ok in JS compilers, throws in dart2wasm

I'm more worried about casting out of an extension type, but that's also much harder to check for.

I am curious about the worry here. I have expected this to be less of an issue for the general extension types feature, but if we're planning on catching that too, that's great for JS interop.

Beyond as, is a big pitfall for us. Users could write jsObject is JSString (or even is String) to check for types. Historically, this is intuitive for them as JS values were reified to Dart types. Both checks will give different answers depending on the platform. This is worse than as because there is no error thrown. Users might end up going through two different code flows depending on the platform. The right way to do type checks is either typeofEquals or instanceOfString, which are external extension members.

I also think some kind of "transitivity" in this lint would be nice.

extension type MyJSString._(JSString value) implements JSString {}

void main() {
  '' as MyJSString; // hopefully a lint!
}

I think it follows there should be a lint for a similar reason that Erik mentioned: there is no subtyping relationship between MyJSString and _JSStringImpl as JSString doesn't implement _JSStringImpl. If we have to do an annotation, I'd like transitivity with the annotation e.g. JSString is marked so therefore any subtype should be as well. This is a contrived example as users generally are not going to have their own interface for a JS string. This is much more common with JSObject, however, as users will write their own interfaces for all kinds of JS objects. See https://github.com/dart-lang/web for an example.

Philosophically, JS interop is moving to a static space and we're telling users that interop only has static guarantees. as and is are not static operations, so I am advising users to keep that in mind, but I don't think it's intuitive and there will be issues along the way without a lint. I also don't think this is a case where users know what they're doing when they do a cast/check. There are several layers of encapsulation here and in the JS interop case, users are likely rather relying on tools that they're used to (checks and casts) than deliberately doing a runtime check for some reason.

bwilkerson commented 12 months ago

This is a really useful feature for JS interop, so it'd be great if we can get this or an annotation-driven version of this.

One significant consideration is that a lint does nothing unless the author of the code that needs to be checked has enabled the lint. That's great for some kinds of checks. It prevents an author from using a coding style they don't want to use, or to help them find places where they're misusing a dart:core API. We could certainly add a lint that would protect against misuse of the dart:js_interop APIs as well.

But if the lint covers all uses of extension types and users don't want this check for all extension types (an unknown at this point, IMO), then we run the risk that users won't enable the lint because it's too restrictive.

On the other hand, if we add an annotation that can be applied to extension types like JSString (indicating that the type casts and type checks involving the extension type are not allowed), then

I'm personally leaning toward the annotation approach, but I'm not a major stakeholder in this discussion.

lrhn commented 12 months ago

I'm more worried about casting out of an extension type, but that's also much harder to check for.

I am curious about the worry here. I have expected this to be less of an issue for the general extension types feature, but if we're planning on catching that too, that's great for JS interop.

It's mainly intended in context: If the problem with casting into an extension type might add "wrong" information (the usual example being casting an int number of centimeters to Inch), then the problem really started where someone cast a Centimeter to int. That was where real, trustworthy (presumably) information was lost. Any attempt to add it back is questionable after that.

Beyond as, is a big pitfall for us. Users could write jsObject is JSString > (or even is String) to check for types. Historically, this is intuitive for them as JS values were reified to Dart types. This will always fail in dart2wasm as the reified type is never a String. This is worse than as because there is no error thrown until presumably later when the user tries to use it as a JSString.

What you are describing here is a "shared" extension type API with different representation types on different platforms.

To Dart, as a language, those types are completely unrelated, it just happens that code written with one can also be run with the other. That's not new to extension types, that applies to any platform specific code or configurably imported code. It's just more visible to extension types, because we can't create an actual shared interface class that they can all implement, and because extension types provide no encapsulation of the representation type. Users can always access the representation object, and check its actual runtime type.

I also think some kind of "transitivity" in this lint would be nice.

extension type MyJSString._(JSString value) implements JSString {}
void main() {
  '' as MyJSString; // hopefully a lint!
}

If the lint warns about casts between unrelated types, then it would apply here too, String is unrelated to MyJSSTring.

However, it's still possible to write something like:

  extension type BreakOpenJSString._(String value) implements JSString,
String {}

which compiles on platforms where the representation type of JSString is String

There is a hack. Define JSString with an extra indirection:

   extension type _Opaque<T>(T it) {}
   extension type JSString._(_Opaque<_JSStringImpl it>) {
     external int get length => (this as _JSStringImpl).length;
     // ...
   }

As currently specified, that would prevent someone from just implementing JSString like above, because they can't have a representation type that is a subtype of _Opaque<_JSStringImpl>, even if _JSStringImpl is String. Won't prevent

  extension type MyJSString(JSString _) implements JSString {}

but that doesn't directly leak what the representation type of JSString is, and should be fine.

But there is no way to prevent jsstring is String checks, because they are true.

eernstg commented 12 months ago

@srujzs wrote:

I expect the definition for a JS string to look something like: ....

We would then have the following superinterface graph with dart2js and ddc where _JSStringImpl means String:

graph BT;
  MyJSString["MyJSString(JSString)"] --> Object
  _JSStringImpl["_JSStringImpl/String"] --> Object
  MyJSString --> JSString["JSString(_JSStringImpl/String)"]
  JSString --> Object
  Object --> ObjectQ["Object?"]

On dart2wasm where _JSStringImpl means JSValue, we'd have the following (assuming that JSValue is non-nullable, and JSValue is a subtype of Object):

graph BT;
  MyJSString["MyJSString(JSString)"] --> Object
  _JSStringImpl["_JSStringImpl/JSValue"] --> Object
  MyJSString --> JSString["JSString(_JSStringImpl/JSValue)"]
  JSString --> Object
  Object --> ObjectQ["Object?"]

This means that the superinterface graph has the same structure (if I made the right assumptions about the typing structure around JSValue), but a JSValue would presumably need to be treated very differently than a String. Perhaps JSString would be obtained from a conditional import such that the JSString on String can basically just export the members of its representation object, but the JSValue would need to have some extension type members to perform the indirection to the value of the externref and do the lowlevel stuff which is probably needed in order to work with such refs.

If JSValue (being a wrapper for "all kinds of foreign objects", IIUC) is actually (potentially) nullable then we'd get this:

graph BT;
  MyJSString["MyJSString(JSString)"] --> ObjectQ["Object?"]
  _JSStringImpl["_JSStringImpl/JSValue"] --> ObjectQ
  MyJSString --> JSString["JSString(_JSStringImpl/JSValue)"]
  JSString --> ObjectQ

In any case, it seems like the real trick here is that JSString can have a very different meaning on different platforms, but it is possible to write code using JSString in a way which is portable, even though it is also possible to detect which platform we're on very quickly (for example, is String).

That's a delicate dance! ;-) I hope it all works out as intended!

srujzs commented 12 months ago

Thanks for the comments! I'm going to try and address all of them in one comment.

Brian:

But if the lint covers all uses of extension types and users don't want this check for all extension types (an unknown at this point, IMO), then we run the risk that users won't enable the lint because it's too restrictive.

Yes, that's valid. I naturally don't have the full picture of where and when extension types can and will be used, but if we narrow down the lint to just the cases where there isn't a subtyping relationship, that might make this more useful than not for users. One option is that I create JS interop-specific lints for extension types if this lint is a no-go, but that'll probably require the same amount of effort anyways.

every package author can apply the annotation to their extension types to get the same behavior (so it isn't special cased only for dart:js_interop APIs)

I'm not opposed to the annotation-based model, but not having transitivity like I mentioned above is not ideal, but I'll still take it. Maybe it is a high ask to control how everyone else uses their extension types just because they implement our own.

Lasse:

To Dart, as a language, those types are completely unrelated, it just happens that code written with one can also be run with the other. That's not new to extension types, that applies to any platform specific code or configurably imported code.

That's fair to say the platform-specificity of the representation type shouldn't influence a lint on the feature. I suppose you could make the argument that you should disallow casts/checks on typedef'd types as well if it did. :) Maybe the right candidate for JS types is a box that get optimized away, ensuring encapsulation while still maintaining relatively zero cost.

It's worth mentioning as an aside that there still is an issue here due to platform-dependent representation types that would not be addressed by this lint (and that's okay): casting between JS types. A value of static type JSAny (the top type of JS types) can be casted to JSString and this will trivially pass on dart2wasm, but there's an actual check on the JS backends. This isn't something we can address, because we want users to be able to downcast to a more specific type if they checked its value.

There is a hack.

Clever, I like it. I hope users don't try to circumvent the lack of JSString <: String by using platform-specific representation types, but good to know we can stop it nonetheless.

Erik:

Cool graphs! Your assumption that JSValue is a subtype of Object is correct - it's just a class today. There's a separate discussion about what to do with JS null and undefined and if they should be converted to Dart null (which they are today).

but the JSValue would need to have some extension type members to perform the indirection to the value of the externref and do the lowlevel stuff which is probably needed in order to work with such refs.

Right, any external members on JS types operate differently depending on the platform. On the JS backends, this is what you're used to seeing with the js_util lowerings. On dart2wasm, there's an unboxing step in the lowering, and then a trampoline function to forward the extern ref to the JS runtime, which then receives it and calls out to the right interop function. The key here is that we need an extra function in each runtime to make it work per declaration.

Your comment about exporting members of the representation type reminds me of one option we've thought about to make jsString is String work consistently. We can have the representation type on dart2wasm implement the representation type that the JS backends use e.g. JSValue <: String, but this comes across two issues:

  1. We need a JSValue-like box per JS type (or maybe a subset), which is probably okay.
  2. More importantly, we need to do a type check for every external value coming from JS so that it forwards to the right box. This type check is expensive because now you need to go through a trampoline. Maybe there's an opportunity for optimization here.

Anyways, I'm getting off-track here. :)

bwilkerson commented 12 months ago

I'm not opposed to the annotation-based model, but not having transitivity like I mentioned above is not ideal, but I'll still take it. Maybe it is a high ask to control how everyone else uses their extension types just because they implement our own.

Using an annotation doesn't mean that it can't be transitive. There are existing annotations (like @immutable) that are transitive.

srujzs commented 12 months ago

Using an annotation doesn't mean that it can't be transitive. There are existing annotations (like @immutable) that are transitive.

Ah, I think I misread your comment about users applying it to their own extension types as "they can opt into transitivity if they want by using the annotation on their extension types that implement JS types" rather than "users can use this annotation instead of only dart:js_interop".

srujzs commented 11 months ago

I want to get a general gauge of how we're feeling about an implementation of this lint (whether it's annotation-based or not, both are totally okay for interop). Do we think this is something that we're not confident in pushing forward on? Do we think we'd like more user feedback once they start using extension types? Is there anything I can help with in making these determinations?

If we think this is likely a no-go, I'll probably want to invest some effort into implementing some basic lints for the JS interop effort to catch the common case (and avoid the complications of higher-order types and such).

lrhn commented 11 months ago

How I feel about it depends very much on how the lint is phrased.

The original phrasing was, effectively, to warn if any expression was cast in a way that changed a covariantly occurring type from a non-extension type to an unrelated (not super- or sub-type) extension type.

That is, a side cast of a value into an extension type. No warning if up- or down-casting, no warning if casting in contravariant position (doesn't change a value, only the requirements of values going into the thing, likely to something stronger).

It could probably be occasionally useful, but no more useful than warning about any side-cast. We have plenty of errors where someone wanted to do a down-cast from Super to Sub and made a mistake, and got no warning because as Sub never gives a warning. That's not special to extension types. The thing that's special about extension types is that the cast may succeed.

But what really worries me is that every example I see of a "bad cast" is something that I can see good reasons for doing in some situations.

  • casting from Iterable<dynamic> to List<E>, using is or as
  • casting from Object? Function(int) to E Function(int), using is or as
  • casting from Object to List, using is or as
  • casting from Function to void Function({String Function(E) callback}) in is or as

Those are all down-casts. Downcasts should be fine.

  • using an extension type in an on clause

Weird. So weird it'll likely never happen, so not something I'd worry about at all.

  • matching, same as above

Definitely useful, especially if the type being tested for is a subtype of the matched value type. Because then the match is a downcast.

If not, then it may be a little more worrisome.

  • Implicit casts from dynamic to a type that contains an extension type.

That's not in my top-five worries when it comes to implicit downcasts.

The underlying worry here is that:

So if we do anything, I'd suggest an annotation, maybe reuse @sealed, which causes a warning if someone casts into or out of the marked extension type. Effectively suggesting that any construction goes through constructors, and any destructuring goes through getters. Would probably also warn if some other extension type implemented the sealed extension type, without having the sealed extension type as a supertype of its own representation type. (Don't use the "my representation type is a supertype of its representation type" rule for allowing implements.)

eernstg commented 11 months ago

I think the crucial underlying motivation is that with the pre-extension types in Dart, e is T and e as T can be used to test or check an expectation about the nature of an object which is firmly justified by that object itself. We're testing/confirming something that can be known.

With extension types, e is T and e as T can be used to postulate (without any proof or justification in the object itself beyond the required representation type) that the type T (which would be, or contain, an extension type) is an appropriate perspective on that object. We're postulating something that may or may not be true, but there might not be any evidence at all about it at run time.

It makes sense to give developers a heads-up in the case where they think that the situation at hand is the former, but it is actually the latter.

Of course, in some cases it is perfectly OK to use a specific extension type E as the static type of absolutely any object whose run-time type is a subtype of the representation type of E. The typical example is FancyString, where we're simply adding some members to String which are perfectly usable on any string.

However, it is a gratuitous loss of expressive power to assume that there are no useful software designs where an extension type E is intended to maintain some kind of information about an object o which isn't encoded in its run-time type. In this case it could be inappropriate to access o with static type E. The standard example is that we may have an extension type Prime(int _), and it isn't safe to jump to the conclusion that every value of type int is a proper Prime.


This is my take on why we'd want to support a detailed and general tracking of casts into an extension type. However, we could still do something very simple and gain a large portion of the sanity checks that this is all about.


So, in order to keep the implementation effort low, we could proceed gradually:

We could start with a very simple lint that flags each cast e as T where the static type S of e is unrelated to T (that is, we don't have S <: T and we don't have T <: S), and T is an extension type.

The next step could replace "T is an extension type" by "there is an extension type E such that E occurs in a non-contravariant position in T."

The next step again could warn in the case where there is a downcast from a top type to an extension type (in the type test/cast expression itself, or in some subterm of the tested types). And so on, guided by the perceived usefulness.


So if we do anything, I'd suggest an annotation, maybe reuse @sealed ...

We could of course use whatever device we want to trigger the assumption that a given extension type represents some information that isn't encoded in the run-time type of the object (which is of course a superset of everything that any static non-extension type of the same object can tell us).

I've proposed using the relationships to non-extension types (extension type E(...) implements ... T ... where T is a non-extension type) as a guide: If the developer has declared that it is appropriate to consider the value of an extension type like E as an expression of type T, we take that to mean that a downcast from T to E is also appropriate (in addition to the fact that an upcast from T to E is definitely appropriate).

An annotation like @sealed would be more flexible, but it would also be more work to get it right.

HosseinYousefi commented 5 days ago

Wanted to let you know about a different use case for this lint.

From a twitter discussion with @DanTup:

Context: the API of LSPs use a lot of union types and we need a way to represent stuff like String | int

What I suggested: let's use an extension type like this:

extension type Union<L, R>._(Object? _value) {
  Union.left(L value) : _value = value;
  Union.right(R value) : _value = value;
}

Now we can do stuff like

final stringOrInt = Union<String, int>.left('hello');
if (stringOrInt case final String string) {
  print(string.length); // prints 5
}

Concern raised: What if user passed a wrong thing to a function that gets a Union by casting?

I think in this case we would want to have a lint to disallow casts into the extension type. Something like what @lrhn said:

If an extension type wants people to always go through its public constructors, that's a separate issue. Possibly something they need an annotation to opt in to, @opaque extension type Foo._(T _) { ... }, and then the analyzer or a lint can warn if you cast into or out of that extension type, including any covariant occurrences of the type in what you cast to, even for a downcast from dynamic. (Up-casting should be fine.)

However we are fine with casting out of the extension type as seen above.

eernstg commented 5 days ago

What if user passed a wrong thing to a function that gets a Union by casting?

Right, that is a relevant concern. I added some support for checking validity in extension_type_unions: Expressions whose static type is a union should always be established by invoking a constructor, not by as, by promotion (is), or by pattern matching. It would be nice to have lint support for avoiding those invariance-breaking dynamic type tests/checks.

DanTup commented 5 days ago

A lint for this would be great, but I'll point out it still wouldn't give the same guarantees as the existing Either2 class I've been using because you still have to trust that your caller had the lint enabled (and didn't ignore it or do something funky that the lint didn't catch). It's certainly easier to trust that if everything is within your own project, but it still feels like a bit of a hole (similar to the hole that languages with unsound null safety have).

eernstg commented 5 days ago

it still wouldn't give the same guarantees as the existing Either2 class

Of course, you need to use a real wrapper object in order to enforce the invocation of constructors in a way that can't be circumvented. In return, you'll pay for allocation when that wrapper is created, and for indirection each time it is used. I think it makes sense to have the choice, and it makes a lot of sense to know what the trade-off is. ;-)