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

#2559. Add extension types and enums to augmenting constructors test. Part 2. #2972

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Nov 7, 2024

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! I do have a comment on a couple of situations where the feature specification is open to interpretation. I'm pushing for an error which seems to have been intended all the time. If it gets controversial then we may end up being allowed to do these things, otherwise these tests should expect an error in a few cases where they currently don't.

I think it would make sense to wait a bit for dart-lang/language#4157 because the spec text currently contains hints that there should be an error, but no clear statement to that effect. So we can't really say that "the spec currently says" that we should or shouldn't have those errors.

augment enum E {
augment e0;
augment const E.foo(int x): this.foo(x);
augment const E.foo(int x): this(x + 1);
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 this should be an error, but dart-lang/language#4157 will clarify that.

@sgrekhov sgrekhov added the status-blocked Blocked from making progress by another (referenced) issue label Nov 7, 2024
@sgrekhov
Copy link
Contributor Author

sgrekhov commented Nov 7, 2024

Thank you. Marked as blocked by dart-lang/language#4157.

@sgrekhov sgrekhov marked this pull request as draft November 7, 2024 14:57
@sgrekhov sgrekhov marked this pull request as ready for review November 15, 2024 08:41
@sgrekhov
Copy link
Contributor Author

Updated according to the dart-lang/language#4157. Please review.

@sgrekhov sgrekhov requested a review from eernstg November 15, 2024 08:41
@sgrekhov sgrekhov removed the status-blocked Blocked from making progress by another (referenced) issue label Nov 15, 2024
@sgrekhov
Copy link
Contributor Author

Marge conflics are resolved. Please review.

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, just a couple of nits.

@@ -5,7 +5,7 @@
/// @assertion Redirecting generative constructors
/// ...
/// It is a compile-time error if:
/// - The augmented constructor has any initializers or a body.
/// - The augmented factory constructor has a body, or it is redirecting.
Copy link
Member

Choose a reason for hiding this comment

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

Different from LanguageFeatures/Augmentation-libraries/augmenting_constructors_A18_t05.dart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an outdated diff. Doublechaked, the actual description is OK, the same as in LanguageFeatures/Augmentation-libraries/augmenting_constructors_A18_t05.dart

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.

Updated. PTAL.

@sgrekhov sgrekhov requested a review from eernstg November 25, 2024 15:47
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 53a0fdc into dart-lang:master Nov 25, 2024
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 1, 2024
2024-11-28 sgrekhov22@gmail.com dart-lang/co19#2976. Update assertion texts according to the new spec (dart-lang/co19#3000)
2024-11-28 sgrekhov22@gmail.com dart-lang/co19#2119. Add issue number to other_declarations_A04_t07.dart (dart-lang/co19#3001)
2024-11-28 sgrekhov22@gmail.com dart-lang/co19#2976. Add semantics tests (dart-lang/co19#2998)
2024-11-27 sgrekhov22@gmail.com Fixes dart-lang/co19#2996. Update CFE expected errors locations (dart-lang/co19#2999)
2024-11-25 sgrekhov22@gmail.com dart-lang/co19#2559. Add extension types and enums to augmenting constructors test. Part 2. (dart-lang/co19#2972)
2024-11-25 sgrekhov22@gmail.com dart-lang/co19#2956. Add more type `void` tests (dart-lang/co19#2997)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: I858dbf6908fe32f38a401ce4ebfe120e064de4aa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398160
Reviewed-by: Alexander Thomas <athom@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
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