-
Notifications
You must be signed in to change notification settings - Fork 170
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
proposal: cast_into_extension_type
#4760
Comments
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 About the bad examples:
That seems useful to me. I'd be more worried if If I have an A downcast from 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 That's more I'd want to do: dispatch(Object o) {
if (o is FancyString) {
doStuff(o.fancyMethod());
} ...
} instead of doing
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 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. Maybe it's OK to warn about non-transparent extension types, which don't directly advertise their representation type.
That's a ...counting on fingers... covariant occurrence of If that's the type you want, and you're going to call the result of that cast with a 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. 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 I'd go with the same rule as above. If the extension
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. I'm more worried about casting out of an extension type, but that's also much harder to check for. 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, 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. |
@lrhn, I think we agree on many things here! One point where we seem to disagree is this one:
I'd agree on that for every reified type (that is, roughly: every non-extension type): The type check ( 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 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 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
Ah, that was a bad example, I corrected it. It is indeed the situation where
No problem, surely we'll have
Same situation, no warning here assuming that
I disagree: The downcast from The problem is still the same: A downcast from any type, including It is equally reasonable to flag this situation when the cast goes from
|
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 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:
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. |
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
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 However, if we use 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
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..... |
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 extension type JSString._(_JSStringImpl value) /* no implementing JSStringImpl! */ {
external int get length;
// Other JS string members.
} The 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 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 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 Philosophically, JS interop is moving to a static space and we're telling users that interop only has static guarantees. |
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 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
I'm personally leaning toward the annotation approach, but I'm not a major stakeholder in this discussion. |
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
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.
If the lint warns about casts between unrelated types, then it would apply here too, 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 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 extension type MyJSString(JSString _) implements JSString {} but that doesn't directly leak what the representation type of But there is no way to prevent |
@srujzs wrote:
We would then have the following superinterface graph with dart2js and ddc where graph BT;
MyJSString["MyJSString(JSString)"] --> Object
_JSStringImpl["_JSStringImpl/String"] --> Object
MyJSString --> JSString["JSString(_JSStringImpl/String)"]
JSString --> Object
Object --> ObjectQ["Object?"]
On dart2wasm where 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 If 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 That's a delicate dance! ;-) I hope it all works out as intended! |
Thanks for the comments! I'm going to try and address all of them in one comment. Brian:
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.
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:
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
Clever, I like it. I hope users don't try to circumvent the lack of Erik: Cool graphs! Your assumption that
Right, any Your comment about exporting members of the representation type reminds me of one option we've thought about to make
Anyways, I'm getting off-track here. :) |
Using an annotation doesn't mean that it can't be transitive. There are existing annotations (like |
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 |
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). |
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. 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 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.
Those are all down-casts. Downcasts should be fine.
Weird. So weird it'll likely never happen, so not something I'd worry about at all.
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.
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 |
I think the crucial underlying motivation is that with the pre-extension types in Dart, With extension types, 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 However, it is a gratuitous loss of expressive power to assume that there are no useful software designs where an extension type 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 The next step could replace " 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.
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 ( An annotation like |
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 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 I think in this case we would want to have a lint to disallow casts into the extension type. Something like what @lrhn said:
However we are fine with casting out of the extension type as seen above. |
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 |
A lint for this would be great, but I'll point out it still wouldn't give the same guarantees as the existing |
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. ;-) |
cast_into_extension_type
Or some name with "introduces extension type."
Description
From this comment:
Kind
Does this enforce style advice? Guard against errors? Other?
I think guarding against errors.
Bad Examples
Iterable<dynamic>
toList<E>
, usingis
oras
Object? Function(int)
toE Function(int)
, usingis
oras
Object
toList<E>
, usingis
oras
Function
tovoid Function({String Function(E) callback})
inis
oras
on
clauseGood Examples
Discussion
Discussion checklist
[Edit by eernstg, Oct 1 2023: Changed the example involving
Iterable<int>
to useIterable<dynamic>
: The example wouldn't trigger the lint because we could never haveE <: int
unlessE
were an extension type withimplements int
, and in that case the lint wouldn't apply anyway. Withdynamic
, the example actually works.]The text was updated successfully, but these errors were encountered: