#2342. Update nullability tests according to the new rules #2385

sgrekhov · 2023-11-20T08:37:46Z

No description provided.

eernstg

Looks good! There are a couple of typos.

A more pervasive issue is that an @assertion includes a sentence from the feature specification which is misleading (it will be fixed, but hasn't been fixed at this time).

I'm not sure what's the best way to proceed with that, maybe just leave it as is and create an issue about updating it when the spec is updated?

LanguageFeatures/Extension-types/nullability_A01_t01.dart

LanguageFeatures/Extension-types/nullability_A01_t02.dart

eernstg · 2023-11-20T11:12:24Z

LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t03.dart

-/// @description Checks that if an instantiated representation type `R` is
-/// not non-nullable then it is a proper subtype of `Object?` (can be assigned
-/// to `Object?`)
+/// @description Checks that if an extension type doesn't implement subtype of


Perhaps:

Suggested change

/// @description Checks that if an extension type doesn't implement subtype of

/// @description Checks that even if an extension type doesn't implement a subtype of

The word 'even' is there to indicate that the following isn't implied: "If an extension type E does implement a subtype of Object then E isn't a subtype of Object?".

There are a couple of other occurrences of 'implement subtype'.

sgrekhov

Updated. Please take another look

LanguageFeatures/Extension-types/nullability_A01_t01.dart

LanguageFeatures/Extension-types/nullability_A01_t02.dart

eernstg

Just one confusing test:LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t04.dart.

LanguageFeatures/Extension-types/nullability_A01_t02.dart

eernstg · 2023-11-20T15:18:27Z

LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t04.dart

-/// compile-time error to assign `Object?` or `null` to `V`)
+/// @description Checks that if an extension type doesn't implement a subtype of
+/// `Object` then is potentially non-nullable (it's a compile-time error to
+/// assign `Object?` or `null` to it)


Perhaps I overlooked something that Object? can do here? Otherwise I'd think it can be omitted:

'Being potentially non-nullable' is the same thing as the negation of 'being nullable'. This means that it is a compile-time error to assign null to it. However, assigning something of type Object? to it is a compile-time error for a lot of other reasons, so that's probably not worth testing here (it's going to test a bunch of other properties, but they are probably being tested in various other tests already).

The idea here is to test that an extension type is a proper subtype of Object?, i.e. it can be assigned to Object? but Object? cannot be assigned to it. And this test checks the latter. Do you believe that we should check here assigning of the null only?

OK, but then we just need to make sure that the description says 'proper' at some point, because that's the part that justifies checking that we can't assign an expression of type Object? to a variable whose type is an extension type that doesn't implement Object.

Looking again, it seems likely that the test where 'proper subtype' should actually be tested is LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t03.dart.

So if the tests about assigning Object? to an extension typed variable are added over there and deleted from here then I think we'll have a good alignment between the contents of the tests and the descriptions of the tests.

Of course there could be other ways to reorganize that would create a similarly precise alignment, but I do think it would be great if something is done about the fact that t03 says "proper subtype" but doesn't test more than half of it, and this test doesn't say "proper subtype", but it does test the halfpart that was missing from t03. ;-)

eernstg · 2023-11-20T15:22:46Z

LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t04.dart


 main() {
  V1 v1_1 = V1(42) as Object?;
-//          ^^^^^^^^^^^^^^^^^
+//                 ^


Surely I did miss something - why would the cast be flagged (unnecessary_cast?), but the initializing expression as a whole is not (even though it has static type Object? and is used to initialize a variable of type V1)?

It's just according the current behaviour of the tools. They report an error there

static_analysis_extension_types_A17_t04.dart:24:20: Error: A value of type 'Object?' can't be assigned to a variable of type 'V1'. - 'Object' is from 'dart:core'. V1 v1_1 = V1(42) as Object?; ^

eernstg

Looks good!

Just one thing: I think it would be great if the discrepancy between the description and the actual test for LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t0{3.4}.dart is resolved.

LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t03.dart

eernstg · 2023-11-21T12:54:11Z

LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t04.dart

-/// compile-time error to assign `Object?` or `null` to `V`)
+/// @description Checks that if an extension type doesn't implement a subtype of
+/// `Object` then is potentially non-nullable (it's a compile-time error to
+/// assign `Object?` or `null` to it)


OK, but then we just need to make sure that the description says 'proper' at some point, because that's the part that justifies checking that we can't assign an expression of type Object? to a variable whose type is an extension type that doesn't implement Object.

eernstg · 2023-11-21T13:12:01Z

LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t04.dart

-/// compile-time error to assign `Object?` or `null` to `V`)
+/// @description Checks that if an extension type doesn't implement a subtype of
+/// `Object` then is potentially non-nullable (it's a compile-time error to
+/// assign `Object?` or `null` to it)


Looking again, it seems likely that the test where 'proper subtype' should actually be tested is LanguageFeatures/Extension-types/static_analysis_extension_types_A17_t03.dart.

So if the tests about assigning Object? to an extension typed variable are added over there and deleted from here then I think we'll have a good alignment between the contents of the tests and the descriptions of the tests.

Of course there could be other ways to reorganize that would create a similarly precise alignment, but I do think it would be great if something is done about the fact that t03 says "proper subtype" but doesn't test more than half of it, and this test doesn't say "proper subtype", but it does test the halfpart that was missing from t03. ;-)

sgrekhov · 2023-11-21T14:32:52Z

Moved V1 v1_1 = V1(42) as Object?; tests from static_analysis_extension_types_A17_t03.dart to static_analysis_extension_types_A17_t04.dart

eernstg

LGTM

eernstg · 2023-11-21T14:39:19Z

Great, thanks!

2023-11-23 [email protected] Merge pull request dart-lang/co19#2395 from dart-lang/merge-pre-nnbd 2023-11-23 [email protected] Fixes dart-lang/co19#2390. Fix tests according to the new method/setter rules (dart-lang/co19#2392) 2023-11-23 [email protected] Fixes dart-lang/co19#2388. Add tests for the new method/setter rules. Update assertions (dart-lang/co19#2393) 2023-11-23 [email protected] Merge remote-tracking branch 'origin/pre-nnbd' into master 2023-11-22 [email protected] Fixes dart-lang/co19#2389. Add additional error expectation for analyzer (dart-lang/co19#2391) 2023-11-21 [email protected] dart-lang/co19#2342. Update nullability tests according to the new rules (dart-lang/co19#2385) 2023-11-21 [email protected] Fixes dart-lang/co19#2384. Fix not well-bound extension types. Add function-type dynamic test (dart-lang/co19#2387) Change-Id: Ic3848f6f39fd42b01bfed5feac3d922c6f5a53d5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338120 Reviewed-by: Erik Ernst <[email protected]> Reviewed-by: Alexander Thomas <[email protected]>

dart-lang#2342. Update nullability tests according to the new rules

d8f07c0

sgrekhov requested review from chloestefantsova and eernstg November 20, 2023 08:37

eernstg reviewed Nov 20, 2023

View reviewed changes

Implement review recommendations

3b0d55b

sgrekhov commented Nov 20, 2023

View reviewed changes

LanguageFeatures/Extension-types/nullability_A01_t01.dart Outdated Show resolved Hide resolved

LanguageFeatures/Extension-types/nullability_A01_t02.dart Show resolved Hide resolved

sgrekhov requested a review from eernstg November 20, 2023 14:46

eernstg reviewed Nov 20, 2023

View reviewed changes

sgrekhov requested a review from eernstg November 21, 2023 12:54

eernstg reviewed Nov 21, 2023

View reviewed changes

Implement review recommendations.

4070348

sgrekhov requested a review from eernstg November 21, 2023 14:32

eernstg approved these changes Nov 21, 2023

View reviewed changes

eernstg merged commit 55f9ad0 into dart-lang:master Nov 21, 2023

sgrekhov deleted the co19-2342 branch January 21, 2025 09:33

	/// @description Checks that if an extension type doesn't implement subtype of
	/// @description Checks that even if an extension type doesn't implement a subtype of

#2342. Update nullability tests according to the new rules #2385

#2342. Update nullability tests according to the new rules #2385

Uh oh!

Conversation

sgrekhov commented Nov 20, 2023

Uh oh!

eernstg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgrekhov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eernstg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eernstg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgrekhov commented Nov 21, 2023

Uh oh!

eernstg left a comment

Choose a reason for hiding this comment

Uh oh!

eernstg commented Nov 21, 2023

Uh oh!

Uh oh!