Fixes #1477. [Records] Type inference tests updated

[sgrekhov]

No description provided.

[sgrekhov]

@eernstg please review

[eernstg]

Looks good, except that in a couple of cases I can't see that the given code actually tests what we need to test.

[eernstg]

I'm trying to understand how this is a test that confirms any property of type inference.

The type of 42 is int (we do run type inference on that expression, but it doesn't do anything).

Then the getter $0 of r1 has static return type num, because the static type of r1 in (num,), and this means that r1.$0.isOdd should be a compile-time error.

In order to see that type inference does take place would be to use record literals containing expressions that need type inference. For example:

class C<X> {
  Type get typeArgument => X;
}

class D<X extends Y, Y> extends C<Y> {}

void main() {
  (C<int>,) r1 = (D(),);
  Expect.equals(int, r1.$0.typeArgument);
}

This would test that type inference actually finds Y == int and then X == int such that D() infers as D<int, int>(), just like it would have inferred D() outside a record literal.

We should have cases for the other coercions, as well (but they may be below in this PR, I have only seen it until this point for now).

[sgrekhov]

The type of 42 is int (we do run type inference on that expression, but it doesn't do anything).

Then the getter $0 of r1 has static return type num, because the static type of r1 in (num,), and this means that r1.$0.isOdd should be a compile-time error.

But doesn't it contradict the Type inference part of the spec?

As noted above, contrary to the practice for runtime checked covariant nominal types, we do not prefer the context type over the more precise upwards type. The following example illustrates this:

  // No static error.
  // Inferred type of the record is (int, double)
  (num, num) r = (3, 3.5)..$0.isEven;

And this test tests this statement from the spec. Did I miss anything?

[eernstg]

There is no contradiction in this situation: The type inference step in Dart is a program transformation that provides actual type arguments to invocations of generic functions where no actual type arguments are provided in the source code, and it adds type annotations to formal parameters of certain function literals where those type annotations have been omitted, and it infers a return type for certain function literals (and there are some other cases, e.g., it may infer the actual type argument(s) of a collection literal). A similar step is the program transformation that in some situations adds actual type arguments to an expression whose type is a generic function type ('generic function instantiation').

In other words, type inference (and other coercions) can change an expression, including the initializing expression of a variable declaration.

There is a completely separate mechanism whereby the declared type of a variable is inferred (when the source code does not specify any such declared type, e.g., var x = e;). This can also be considered to be a program transformation (changing var x = e; to T x = e; for some T).

Inference of the declared type of a variable never occurs in the case where a declared type is present in the source code.

For line 34, type inference on (42,) yields (42,) (a no-op), and no type inference of the type of r1 occurs. So it's all a no-op, and the type of r1 is (num,). Hence, r1.$0.isOdd is a compile-time error.

In the example I gave, type inference on (D(),) yields (D<int, int>(),), and the test on r1.$0.typeArgument serves to confirm that the run-time semantics corresponds to the transformation that the type inference is expected to have performed.

So in this case there is non-trivial type inference on the initializing expression, and we are investigating the outcome (somewhat indirectly, because we can't directly watch type inference at work).

[sgrekhov]

Thank you for detailed explanation! I updated tests accordingly.

[eernstg]

Same issue here: It's a compile-time error, and it doesn't actually test type inference. Same in line 41-42.

[eernstg]

This is tricky. First, r has the declared type (num, double, int Function(int), void Function(num)) because we've declared it explicitly (no type inference needed).

Next, we do infer (d, 3, id, c) with this type as the context type, and that does introduce the cast d as num, it changes id to id<int>, and it changes c to c.call.

All of that is great, but line 45-46 doesn't serve any purpose that I can see.

The fact that line 44 isn't a compile-time error implies that the static type of (d, 3, id, c) in that context is assignable to the declared type, and I think we can consider that to serve as evidence that the static type of (d, 3, id, c) is OK after type inference. (By the way, I think it could be tested using (d, 3, id, c)..expectStaticType<...>().)

But we'd still like to confirm dynamically that the coercions did actually take place.

For the insertion of the cast from dynamic, we could have a variant dynamic d2 = 1.5; and an assignment (int, double, Function, Function) r2 = (d2, 3, id, c);, which would be expected to throw at this cast.

For the fact that (in line 44) 3 is an integer literal with static type double, we'd have to distinguish between web and native platforms, and on the native platforms we could test that (r2.$1 as dynamic).isEven throws.

Similarly for the generic function instantiation on id, and the .call insertion on c.

[sgrekhov]

Runtime checks added

[eernstg]

Same issues as previous test: Line 41-42 doesn't do anything useful as far as I can see, but we should (1) test the static type of ([], {}, {}) in line 40 using ..expectStaticType as previously, and (2) we should check that the list/set/map does get a run-time type which is a subtype of List<int>, Map<String, int>, respectively Set<String>.

[sgrekhov]

Updated

[eernstg]

..expectStaticType again. The dynamic types can be tested in the body of bar.

[eernstg]

I don't think horizontal inference is applicable to record literals (at least, I don't remember hearing anything about it). But it sounds like an interesting additional feature, so it's great that we have it on the table! I'm creating a new language repo issue about this question.

Next, I can see that this test is actually about a situation involving a generic function and an invocation thereof, and the new thing is that some types involved in the associated horizontal inference for that function invocation are record types. Very good, we need that!

As far as I can see, we have the stages {b, c, d} and {a}, and T and U are as stated. So that looks good!

See dart-lang/language#2578 as well, we may need some additional tests (possibly that it doesn't work, if the language and implementation teams think it should not be supported, or that it should wait).

[eernstg]

Looks good, just one question remaining.

[eernstg]

Ah, yes, we can do that, cool!

[eernstg]

That's interesting! I think it's a correct expectation that the first positional variable initializer d gets transformed into d as num, and hence the record type has num as its first positional type.

I think this is interesting because it tests the detailed ordering of the effects: If we had instead transformed the initializing expression into (d, 3, id, c) as (num, <and more>) then the expectStaticType would see dynamic as the declared type of the first positional variable of the record literal. So they might have to change some things in the analyzer/CFE in order to make things happen in the specified order.

[eernstg]

Wouldn't that expectation be double rather than int?

[sgrekhov]

Of course double. Thank you for noticing

[eernstg]

LGTM