Skip to content

#2953. Add inference with bounds tests #2957

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

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Conversation

sgrekhov
Copy link
Contributor

Added examples from appropriate SDK issues. These tests fail without inference-using-bounds flag and pass with it. @eernstg @chloestefantsova what else can be added to test the new fucntionlity? Any advices are wellcome!

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think we should have a test that includes mutually dependent type variables, something like this:

class A1<X extends A1<X, Y>, Y extends A2<X, Y>> {}
class A2<X extends A1<X, Y>, Y extends A2<X, Y>> {}
class B1 extends A1<B1, B2> {}
class B2 extends A2<B1, B2> {}
class C1 extends B1 {}
class C2 extends B2 {}
class D implements B1, B2 {}

void f<X extends A1<X, Y>, Y extends A2<X, Y>>(X x, Y y) {}

void main() {
  f<B1, B2>(B1(), B2());
  f<B1, B2>(C1(), C2());

  f(B1(), B2()); // Can only be inferred with this feature.
  f(C1(), C2()); // Ditto.
}

Also two levels of "extract the bound" would be nice:

class A<X extends Iterable<Y>, Y extends Iterable<Z>, Z> {
  A(X x);
}

void main() {
  var a = A([[1]]);
  print(a.runtimeType);
}

This actually prints A<List<List<int>>, Iterable<Object?>, dynamic> without the feature and A<List<List<int>>, List<int>, int> with the feature, which illustrates how much typing precision we're preserving.

Also (which wasn't entirely obvious ;-), it's nice to see that it does work also in both the mutually recursive case and in the "repeated" case.

// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

/// @assertion In section Constraint solution for a set of type variables we
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an issue comment which is found here: dart-lang/language#3009 (comment). However, that's not the actual new rules, it's just an early attempt to find a useful set of rules.

Chloe actually found a much more concise way to specify this, which is also a better solution, and it is the one which is implemented.

So I'd recommend that you use the change to inference.md in dart-lang/language#4140 as the @assertion.

The actual change is that one item in an itemized list is added.

Perhaps the most readable presentation of that change would be to list the algorithm as a whole (that is, line 719-738 of inference.md in that PR), and then inform the reader that the given item has been added.

I guess the extra information to the reader would not be given as part of the @assertion, given that the assertion is expected to be a verbatim excerpt from a specification document. However, I think we've had a similar situation earlier, and there was some kind of marker (@commentary, or something with a similar meaning) which could be used to say things that aren't taken from a spec.

void main() {
Expect.equals(B, f(B()));
Expect.equals(B, f<B>(C()));
Expect.equals(B, f(C()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we should definitely have this test, that was the starting point for the whole thing.

Comment on lines 53 to 55
A(X x) {
print('A<$X, $Y>');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just have this:

Suggested change
A(X x) {
print('A<$X, $Y>');
}
A(X x);

There is no need to include the print statement when we have a better solution: expectStaticType.

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Nov 1, 2024

  f(B1(), B2()); // Can only be inferred with this feature.
  f(C1(), C2()); // Ditto.

Only the second case cannot be inferred without the feature. Starting point (described in dart-lang/language#3009) is:

class A<X extends A<X>> {}
class B extends A<B> {}
class C extends B {}

void f<X extends A<X>>(X x) {}

void main() {
  f(B()); // OK.
  f(C()); // Inference fails, compile-time error.
  f<B>(C()); // OK.
}

f(B1(), B2()); and f(B()); seems similar and can be both inferred without the feature.

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Nov 1, 2024

Moved to type system and updated accordingly. PTAL.

@sgrekhov sgrekhov requested a review from eernstg November 1, 2024 14:23
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eernstg eernstg merged commit 5b8762f into dart-lang:master Nov 11, 2024
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Nov 15, 2024
2024-11-14 [email protected] dart-lang/co19#2956. Add more tests for the type `void` (dart-lang/co19#2977)
2024-11-12 [email protected] dart-lang/co19#2956. Rename, move and update existing return from `void` tests (dart-lang/co19#2975)
2024-11-12 [email protected] dart-lang/co19#2956. Add more type void tests. Update existing ones (dart-lang/co19#2974)
2024-11-11 [email protected] Fixes dart-lang/co19#2943. Use name `dartaotruntime` instead of `dart_precompiled_runtime` (dart-lang/co19#2973)
2024-11-11 [email protected] dart-lang/co19#2953. Add inference with bounds tests (dart-lang/co19#2957)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: I1f3c534a1e69af28c9adb93bdc13b8741a64e7f8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/395500
Reviewed-by: Erik Ernst <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Commit-Queue: Erik Ernst <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants