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

#2976. Add more tests for nullable and FutureOr types #3050

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sgrekhov
Copy link
Contributor

No description provided.

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, with several comments. It looks like a handful of tests might be based on language that we shouldn't test (because it isn't normative, even though the feature specification does not mention this fact explicitly).

/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
///
/// @description Checks that static access shorthand can be used with nullable
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Suggested change
/// @description Checks that static access shorthand can be used with nullable
/// @description Checks that static access shorthands can be used with nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced the entire description here and below.

///
/// It allows `int? v = .tryParse(42);` to work. That’s a pretty good reason. It
/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we'd otherwise have assertions from non-normative parts of a feature specification.

Also, it contradicts the normative parts of the feature specification (at this time, perhaps there was a time in the past where it didn't): The static namespace of T? is always the static namespace of T.

https://github.com/dart-lang/language/blob/main/working/3616%20-%20enum%20value%20shorthand/proposal-simple-lrhn.md#type-inference

So maybe find a different location in the spec to provide the assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the type inference section. There are type_inference_A01_t01.dart and type_inference_A01_t02.dart that already check the similar functionality. But I prefer to leave them as tests that check that the correct type parameter is infered. Like C<int> c2 = .id<String>();.

So, I simply added these new tests checking nullable and FutureOr types for constructors (including factory ones) and static getters/methoods/variables.

/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
///
/// @description Checks that static access shorthand can be used with nullable
Copy link
Member

Choose a reason for hiding this comment

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

Same as the previous test.

/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
///
/// @description Checks that static access shorthand can be used with nullable
Copy link
Member

Choose a reason for hiding this comment

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

Same again.

/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
///
/// @description Checks that static access shorthand can be used with nullable
Copy link
Member

Choose a reason for hiding this comment

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

And again.

/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
///
/// @description Checks that static access shorthand can be used with nullable
Copy link
Member

Choose a reason for hiding this comment

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

And again.

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Thank you! Moved tests to type inference section. Descriptions fixed. PTAL.

/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
///
/// @description Checks that static access shorthand can be used with nullable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced the entire description here and below.

FutureOr<C<int>?>? c5 = .staticMethod<int>(5);
Expect.equals(5, (await c5)?.value);

FutureOr<List<C<String>?>?>? c6 = .instances;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Fixed in these tests. There are more tests containing this issue. I'll prepare a separate PR with will fix them.

///
/// It allows `int? v = .tryParse(42);` to work. That’s a pretty good reason. It
/// also allows `int x = .tryParse(input) ?? 0;` to work, which it wouldn’t
/// otherwise because the context type of `.tryParse(input)` is `int?`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the type inference section. There are type_inference_A01_t01.dart and type_inference_A01_t02.dart that already check the similar functionality. But I prefer to leave them as tests that check that the correct type parameter is infered. Like C<int> c2 = .id<String>();.

So, I simply added these new tests checking nullable and FutureOr types for constructors (including factory ones) and static getters/methoods/variables.

@sgrekhov sgrekhov requested a review from eernstg January 16, 2025 13:21
@sgrekhov sgrekhov changed the title #2976. Add tests for nullable and union types #2976. Add more tests for nullable and FutureOr types Jan 16, 2025
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