Skip to content
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

A potentially spurious expected upper bound in an extension types test #2313

Closed
chloestefantsova opened this issue Oct 16, 2023 · 8 comments
Closed
Assignees
Labels
bad-test Report tests in need of updates. When closed, the tests should be considered good

Comments

@chloestefantsova
Copy link
Contributor

It seems that in the following lines Object should be the expected upper bound between ET3("String") and ET4(3), not Object?, because both types are non-nullable.

var v3 = 2 > 1 ? ET3("String") : ET4(3);
v3.expectStaticType<Exactly<Object?>>();

@chloestefantsova
Copy link
Contributor Author

/cc @eernstg

@eernstg
Copy link
Member

eernstg commented Oct 16, 2023

Right, ET3<String> and ET4<int> both have Object as a supertype because their representation type is non-nullable. It is intended that this should cause Object to be encountered in their superinterface graphs, and the standard upper bound could then not be Object?, it would be at least Object.

However, I think the specification fails to make this work in cases like this one. We have the following rule:

If DV does not include an <interfaces> clause then DV has Object? or Object as a direct superinterface, according to the subtype relation which was specified earlier.

This is sufficient to give rise to the desired superinterface graph for an extension type with no <interfaces> (that is, with no implements ... clause), but it does not suffice for the case we have here. We should actually have this rule:

DV has Object? or Object as a direct superinterface, according to the subtype relation which was specified earlier.

It may well be a seemingly redundant edge in the superinterface graph because there is some other way to Object or Object?, but in cases like in this issue we do not have that other way to Object.

So please consider the specification to be adjusted as mentioned above, and keep an eye on dart-lang/language#3402.

@lrhn
Copy link
Member

lrhn commented Oct 16, 2023

The intention is that an extension type is non-nullable if its representation type is, because we make it have Object as supertype then, and definitely not non-nullable if its representation type isn't, so it's always precisely the same non-nullability as the representation type. (An extension type is never nullable.)

I agree that the current text fails to capture that.

Whether an extension type is non-nullable is a property of the type, not the declaration. It doesn't necessarily make sense to ask whether a generic extension type declaration is non-nullable or not.

Sometimes it's easy, and we can see that the declared representation type is non-nullable, which means that it will be non-nullable no matter how the type variables are instantiated.

Other times the representation type is trivially always nullable.

But in cases like:

extension type E<T>(T _) {
  void foo() {
    this ?? print("really?");  // Valid code, shouldn't have warnings.
  }
}

we can't say whether E, the declaration, is non-nullable and has Object as superinterface, because it's not a question that can answer at the level of the declaration. It's only known for actual types.

@sgrekhov sgrekhov self-assigned this Oct 17, 2023
@sgrekhov sgrekhov added the bad-test Report tests in need of updates. When closed, the tests should be considered good label Oct 17, 2023
@sgrekhov
Copy link
Contributor

Fixed by #2314. But I'm not closing this issue. First, please note that the tests is still failing in both analyzer and CFE

var v2 = 2 > 1 ? ET1<String>("String") : ET2<String?>("String");
v2.expectStaticType<Exactly<Object?>>();

Tools are expecting Object on line 33. Is this a tests issue?

I'm also going to add more tests regarding nullability as part of this issue. In particular we have no test that there should be no warning in case of

extension type E<T>(T _) {
  void foo() {
    this ?? print("really?");  // Valid code, shouldn't have warnings. But we have!
  }
}

@eernstg
Copy link
Member

eernstg commented Oct 17, 2023

Tools are expecting Object on line 33. Is this a tests issue?

ET1<String> has Object as a direct superinterface and in turn Object?. ET2<String?> has Object? as a direct superinterface. The standard upper bound of the two should be Object?. I can't see any other choice, so this would be a tools issue.

I'm also going to add more tests

Sounds good! The fact that this can be null should be a property which is shared with extension declarations (and with the class Null ;-), but it would in any case be possible for the ?? expression to evaluate the right hand expression. So we should not get a warning.

@chloestefantsova
Copy link
Contributor Author

The CFE compiles line 33 without emitting an error message. That may be due to the landing of dart-lang/sdk@c658b46 earlier today.

@sgrekhov
Copy link
Contributor

The CFE compiles line 33 without emitting an error message. That may be due to the landing of dart-lang/sdk@c658b46 earlier today.

Yes, confirming. No CFE issue on the top SDK. Analyzer issue only

@sgrekhov
Copy link
Contributor

dart-lang/sdk#53781

sgrekhov added a commit to sgrekhov/co19 that referenced this issue Oct 20, 2023
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 24, 2023
2023-10-23 sgrekhov22@gmail.com Fixes dart-lang/co19#2319. Fix roll failures (dart-lang/co19#2321)
2023-10-23 sgrekhov22@gmail.com dart-lang/co19#1400. [Extension types] Add more top types tests (dart-lang/co19#2322)
2023-10-18 sgrekhov22@gmail.com dart-lang/co19#1400. [Extension types] Add more superinterfaces tests (dart-lang/co19#2315)
2023-10-18 sgrekhov22@gmail.com dart-lang/co19#2291. Add more Link.createSync() tests. Part 2 (dart-lang/co19#2316)
2023-10-17 sgrekhov22@gmail.com Fixes dart-lang/co19#2304. Add more `Object` member tests (dart-lang/co19#2312)
2023-10-17 sgrekhov22@gmail.com dart-lang/co19#2313. Fix expected static type in upper_bound_A01_t05.dart (dart-lang/co19#2314)

Change-Id: Id5279b7bad5e45c7e8a5d2fa7cbffe49bd1b2093
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331860
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
eernstg pushed a commit that referenced this issue Oct 24, 2023
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 27, 2023
2023-10-25 sgrekhov22@gmail.com Fixes dart-lang/co19#2329. Fix FileSystemEntity.type() on Windows (dart-lang/co19#2332)
2023-10-25 sgrekhov22@gmail.com Fixes dart-lang/co19#2330. Remove obsolete error expectation (dart-lang/co19#2331)
2023-10-25 sgrekhov22@gmail.com Fixes dart-lang/co19#2317. Add tests for extension type type parameters in non-covariant position (dart-lang/co19#2318)
2023-10-25 sgrekhov22@gmail.com dart-lang/co19#2291. Update `Link.createSync()` according to the new documentation. Part 1 (dart-lang/co19#2305)
2023-10-24 sgrekhov22@gmail.com dart-lang/co19#2313. [Extension types] Add nullability tests (dart-lang/co19#2324)
2023-10-24 sgrekhov22@gmail.com dart-lang/co19#2323. Delete obsolete test (dart-lang/co19#2327)
2023-10-24 sgrekhov22@gmail.com Fixes dart-lang/co19#2323. Don't test datagram size more than 65503 (dart-lang/co19#2325)

Change-Id: I23b57a58b5cd6452e2beb923dd8cde6fd587f051
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332206
Reviewed-by: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bad-test Report tests in need of updates. When closed, the tests should be considered good
Projects
None yet
Development

No branches or pull requests

4 participants