-
Notifications
You must be signed in to change notification settings - Fork 213
[Records] Should pointwise implicit downcasts be allowed in records? #2488
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
So the question is whether the context type is propagated into the record, and what that means. The context type must be propagated into a record, otherwise For We should downcast We are not consistent between decimal-literal-to-double promotion and implicit downcast: extension <T> on T {
T log() {
print("$this:$runtimeType @ $T");
return this;
}
}
void main() {
double d = 1..log(); // Prints "1.0:double @ double", so static type of `1` is double.
var d = "x" as dynamic;
// int x = d..log(); // Does not work because static type of `d` is `dynamic`.
int x = d..toString().log(); // prints "x:String @ String" before failing the cast, so it's at least outside of the *cascade*.
} I don't remember what we decided about where to insert the downcasts. int x = ((d)..something()); could be ant of int x = ((d as int)..something());
int x = ((d)..something() as int);
int x = ((d)..something()) as int; It's not the first one. double y = args.isEmpty ? 1 : 2.5; // Works.
var d = "a" as dynamic;
int x = (args.isEmpty ? d : 4)..log(); // Throws because static type is `dynamic` for the ..log call. We cast outside the conditional expression. So, for consistency, I guess (int, int) r = (d, 4); should give the RHS the static type That's also what happens when the RHS is not a literal. var d = (random ? 'hello" : 2) as dynamic;
(dynamic, int) p1 = (d, 4);
(int, int) p2 = p1; The (int, int) p2 = p1 as (int, int); (where an optimizing compiler can omit checking the second field, and only check the ones typed as Same thing should happen for: var (int x, int y) = p1; The RHS is implicitly downcast to the LHS's required type before destructuring. If ordering matters, which it shouldn't here, but might for something like; dynamic d = "a" as dynamic;
Foo foo = Foo(); // has x getter with side effects
var (Foo(x: var x), int y) = (foo, d); Here the RHS is assignable to the LHS type of |
I saw the feature spec updates before I saw this issue, and commented here. I agree that there should be an implicit cast (and it's a separate effort, using I'd prefer if we could specify this mechanism as a transformation that adds a cast to an expression of type (So type inference is just one of several transformations, transforming So I'm suggesting that we transform The most compelling reason to do this is probably the potential introduction of bottom-up inference on other expressions, namely instance creations where some type arguments use statically checked variance: class C<out X, inout Y> {
final X x;
Y y;
C(this.x, this.y);
}
void main() {
var d = 1 as dynamic;
C<int, int> c = C(d, d); // Error or implicit casts?
} |
Erik's question is whether we define "assignability" for records so that a record type is assignable to another record type iff they have the same shape and the field types are pointwise assignable, or just by subtyping. I think we should define assignability like that, for a couple of reasons.
I have a strong opinion about assignability of records, which implies that an implicit downcast will be made. var p1 = (1, 2 as dynamic);
(int, int) p2 = p1; // implicit `as (int, int)`. For a literal, it matters mostly when combined with conditional expressions or cascades. (int, int) r = someTest ? (1, 2 as dynamic) : (2 as dynamic, 1);
(int, int) q = (1, 2 as dynamic)..$1.arglebargle(); // allowed statically. What we currently do is to delay the cast as long as possible (for double literals, that's no delay at all, ditto for the instantiated tear-off, because originally we couldn't instantiate after tear-off, for |
Like Erik, I commented on that topic elsewhere. I copy my comment below. Maybe another minor argument in defence of casting from foo(Funcion(int, String) f, (dynamic, String) a) {
// This will not work.
f(...a); // '...a' is a hypothetical syntax for spreading arguments.
// This will work.
f(a.$0, a.$1); // The downcast from 'dynamic' will be inserted here.
} |
Ok, I think this discussion broadened a bit, so let me try to summarize the issues raised. First, as I should have pointed out in my initial comment, this is quite connected to the issues raised here by @stereotype441 . With that in mind, there are three possible directions that have been raised above (only two of which I had intended to introduce). To make it concrete, let's discuss WRT the following example: dynamic d = "hello";
(dynamic, dynamic) dr = (d, d);
(int, int) r = (d, 4); // Example 1
r = dr; // Example 2 There are three possible interpretations we could take:
I had not proposed to go the third route, and am somewhat opposed to it - it's highly inconsistent with what we do with all other types. For example, we do not allow Addressing some specific comments:
I don't think this follows. Note that we do not make
None of your examples require the pointwise assignability (my option 3 above). They are all covered by option 2, I believe. That said:
It is somewhat compelling that this would work uniformly if we did choose to specify option 3. The fact that the runtime type of a record is computed from its values means that casting any value I'm not sure I find this compelling enough to want to add more implicit downcasts the the language though. |
I usually argue in favor of option 2, #2129, exactly because it is a simple and consistent approach. The argument against this approach is usually that it does not work very well for cascades: class Callable {
void call(int i) {}
void foo() {}
}
void main() {
// Standard example: Works today, breaks if taken to mean `(Callable().call)..foo()`
void Function(int) f = Callable()..foo();
// Variant that arises for records and cast-from-dynamic.
var d = 1 as dynamic;
(num, num) r = (d, 3.14)..$0.foo(); // Allowed with outer cast, fails with `(d as num, 3.14)`.
} We surely do want to propagate the context type to cascade receivers (such that However, I don't see any additional inconveniencies caused by pushing casts into record literals, so I'll continue to support option 2 as the most consistent approach. That shouldn't prevent any of the solutions to the cascade problem that we might come up with. On the other hand I'm worried about option 3 (that is, lifting assignability such that not just void f(int i) {}
void main() {
void Function(dynamic) g = f; // Will _not_ desugar as `... g = (dynamic d) => f(d);`
} |
I don't think comparing records to other kinds of types, mainly collections, is the right choice. They are not like other types. Records are defined entirely by their content. A triple is not a special object that exists in its own right, it's just three objects in a trenchcoat. The record "object" has no identity, no behavior other than projecting values out of it, and no type other than the combination of the types of the field values. It's reasonable to consider a record object as a boxing of the collection of individual values, rather than records being inherently unboxable objects. A list is an object with its own behavior, which can include mutability, and its own runtime type. It happens to be containing other objects, but it's not defined entirely by its elements. Because of our reified generics, a For a value of type Another place where it's obvious that a record is just adjacent individual values is in the treatment of The more we lean into that explanation, the easier things are to explain and understand. So I believe we should do number 3: Define assignability to include records of the same shape, where the corresponding fields are assignable. |
I don't really understand the argument being made here. It's true that the list value might be a So what's the principle here? We have a bunch of implicit casts we could do based on the principle "it might work". The principle we've taken as of |
Ok, I'll be honest: I'd be fine with allowing an implicit downcast from The principle here is that records are not like other things, and I want record assignments and operations to behave like they are single-object operations performed in parallel. If there is a significant difference in how any of these six record assignments work: dynamic d = 0 as dynamic;
var p = (1, d);
/*1*/ (int, int) r1 = (1, d);
/*2*/ (int, int) r2 = p;
/*3*/ var (int x1, int y1) = (1, d);
/*4*/ var (int x2, int y2) = p;
/*5*/ int x3 = (1, d).$0, y3 = (1,d).$1;
/*6*/ int x4 = p.$0, y4 = p.$1; then I'll have a hard time explaining why. Numbers 5 and 6 will definitely work. One of these solutions is easy to predict and understand, the one where it doesn't matter in which order you do things. |
Fair enough. But there's no getting around this in general. For int to double conversions, 2 and 4 will never work. Similarly for In general, this tends to make me lean in the other direction: for downcasts from dynamic (at least), this feels like a good argument for not pushing the context type into things (or at least into records). In that case, 3-6 work, and 1, 2 don't, and it's easy to explain: if you're assigning something of type dynamic to something of a different type, it just works - otherwise it's an error if it's not a subtype. At a higher level, I think the consistency argument is reasonable, but I think it's not sufficient: doing the wrong thing consistently is still doing the wrong thing. I'd like to see a positive argument here: why is it generally helpful to users to allow these assignments? In general, my experience with implicit assignments from dynamic is that the make it harder to read the code - you can't trust that what you're seeing is what you're getting. I'm reluctant to expand the set of situations in which we do this. Note too that we do not do this for other composite types (e.g. FutureOr and Function types). So it's not clear to me that this is even a net win in terms of consistency even ignoring the other kinds of conversions. |
@leafpetersen wrote:
Perhaps we should make a distinction between propagating the context type schema into various constructs, and performing code transformations based on differences between the context type schema and the type of the inferred subexpression? class C<X> {
void setup(String s) {...}
}
void main() {
C<int> c = C()..setup('');
} Surely we don't want I tend to prefer the proposals where a mismatch between the context type schema and the inferred expression type triggers a code transformation initially, but with each propagation of a context type schema into a subexpression, the particular shape of the enclosing expression will or will not propagate the ability to perform the code transformation. When specific code transformations are not enabled, the result of a typing discrepancy is usually a compile-time error. For instance, we could disable the ability to perform From that perspective, it's an open choice whether or not we'd propagate the ability to cast from dynamic into a record literal: We have some code transformations that are enabled in similar situations, and other code transformations that aren't enabled. I would tend to enable cast from dynamic in record literals, because of the very direct connection between each field of the record literal and the context type schema for that field. However, we don't have any other code transformations that are similar to the one that transforms a record-typed expression |
If we define record assignability as point-wise assignability, it's true that we get into a problem with It would make We can do that. It means every record assignment can be an implicit destructuring and rebuilding. So, we can make the number 2 case work. And I'd be in favor of doing that. Also, number 4, (int, CallableClass) p = ...;
var (int x, Function f) = p; I'd actually expect the second line to be the same as |
There is a lot going on here that I'm struggling to wrap my head around. I don't agree with:
Records definitely do exist. They are objects—literally subtypes of They are value types in that they don't have any persistent identity and can be allocated on the stack or registers or otherwise optimized away. But they are values. I've never fully internalized how and where Dart inserts implicit coercions, especially when things like cascades get involved. But as far as Lasse's examples go, here's how I intuitively think they should behave and what I intend the proposal to specify: /*1*/ (int, int) r1 = (1, d); This is OK.
/*2*/ (int, int) r2 = p; This should be a compile-time error because /*3*/ var (int x1, int y1) = (1, d); This is OK.
/*4*/ var (int x2, int y2) = p; This is also OK, but through a different mechanism than in 3. Here, since
/*5*/ int x3 = (1, d).$0, y3 = (1,d).$1; This is fine. We insert an implicit cast from /*6*/ int x4 = p.$0, y4 = p.$1; Likewise.
It does. The process is:
The way the behavior is specified here is a little different from other places where a value of static type However, this only works for class Callable {
void call() {}
}
var (Function f,) = (Callable(),); According to the current proposal, this would be allowed at compile time since |
Oh hi. At @leafpetersen's encouragement I just read carefully through this thread to catch myself up on the discussion. I don't have a lot to add beyond what's already been said, but I guess I might as well throw my opinion into the ring. I'm trying to think about this in terms of the two-part question "what would I want to do if I could design the langauge from scratch, without worrying about existing code, and then how can I make incremental steps toward that while breaking as few users as possible?" Personally my answer to the first part is "do int->double conversions and inference of generic type parameters just as Dart does today. Do dynamic downcasts as early as possible based on context type (as proposed in #2129). Get rid of implicit And my answer to the second part is "since records are new, let's make them behave as consistently with #2129 as we can, without breaking anything else". So from that standpoint, I like @leafpetersen's second bullet (from #2488 (comment)). Namely, define type inference for record literals so that if a field has static type dynamic, an implicit downcast is inserted to coerce it to the corresponding type from the context. Which means that in @lrhn's example: dynamic d = 0 as dynamic;
var p = (1, d);
/*1*/ (int, int) r1 = (1, d);
/*2*/ (int, int) r2 = p;
/*3*/ var (int x1, int y1) = (1, d);
/*4*/ var (int x2, int y2) = p;
/*5*/ int x3 = (1, d).$0, y3 = (1,d).$1;
/*6*/ int x4 = p.$0, y4 = p.$1; I'm proposing that 1, 5, and 6 will work (and implicitly downcast This makes sense to me, and I think I can explain it intuitively to users like so: in 1, 5, and 6, there is an expression with type So what about 3 and 4? According to the current patterns spec (as I read it), 3 and 4 will work (and implicitly downcast I was about to post this comment when I got interrupted for another discussion, and when I returned I saw that @munificent had posted his opinion. It looks like the two of us want the same thing, so we've got that going for us, which is nice. |
After further discussion with @munificent and @leafpetersen, I'm OK with what @munificent says above too. We could allow item 2 too, but only for implicit downcasts, it won't work for generic instantiation or The way we currently implement implicit downcasts is to do it at "assignments", which is why we don't do it below a cascade, like in About:
I think that should be accepted using the same logic as the other cases:
Success, because we do the coercion on the RHS. Function f = (Callable().call,).$0; If we instead look at var c = (Callable(),);
var (Function f,) = c; it would still work!
Success. Because the value is only assignable, not a subtype, we need to insert a coercion at the assignment to The corresponding desugaring would be: Function f = c.$0.call; The implicit coercions to pattern variables only work for assignment, not matching. if (c case (Function f,)) ... would start out checking if In a pattern match (rather than pattern declaration or assignment), every eventual assignment is either guarded by a type check, which means anything requiring coercion will fail the check, or there is no type check and then there is also no context type, so you can't get a coercion anyway. |
OK, it sounds like we have consensus 🎉 . I believe Leaf will update #2489 to align with this in regards to record static type inference. I want to update the patterns proposal to make it clearer where these "assignments" (i.e. coercion points) happen. I'll leave this issue open to track that. |
Ok, this is good discussion, and nice to see a consensus developing. I have updated #2489 with a proposed specification for this that I believe reflects the intended static semantics as described above, with the exception of the cases that involve destructuring, which will be specified separately in the patterns proposal. |
This CL implements the adjustment of the static semantics for records as described in dart-lang/language@d19f6d5. The adjustment is based on the discussion at dart-lang/language#2488. Part of #49713 Change-Id: I7a9d456f702ad0fb14aa3bd121ba9d2bbd104414 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/262202 Reviewed-by: Johnni Winther <[email protected]> Commit-Queue: Chloe Stefantsova <[email protected]>
* Clarify how implicit conversions happen in patterns. Fix #2488. * Remove "first" in "first accesses". * Fix non-normative example to not claim assignability is involved. * Respond to review comments. * Revise some more. Do use a downwards context type when inferring constant patterns. But don't insert implicit coercions in refutable patterns.
Consider the following example:
There are two possible inferred types for the record literal.
(dynamic, int)
dynamic
, and the RHS type is not a subtype of the LHS type.(int, int)
d
toint
before constructing the record.Which do we intend?
In general, I'm not keen on adding more implicit downcasts. I'd rather the user make the cast explicit here.
We do push casts inwards for list literals however:
fails at runtime, not statically (as it must, since we prefer the context type for nominal types, unlike the structural types).
cc @munificent @eernstg @lrhn @jakemac53 @natebosch @stereotype441 @kallentu @chloestefantsova @johnniwinther @scheglov @srawlins
The text was updated successfully, but these errors were encountered: