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

#1959. Tests for class modifiers restrictions and type aliases #2030

Merged
merged 5 commits into from
May 10, 2023

Conversation

sgrekhov
Copy link
Contributor

No description provided.


typedef LocalTypedefSealedClass = SealedClass;

mixin MixinOnLocalTypedefSealedClasson LocalTypedefSealedClass {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is on missing between MixinOnLocalTypedefSealedClasson and LocalTypedefSealedClass?

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

// [cfe] unspecified

base mixin class MixinClassIndirectlyImplementsBaseClass implements TypedefWithBaseMixinClass {}
// ^^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the error location marker ^^^ be extended for the entire TypedefWithBaseMixinClass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `final`. Test
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood the spec correctly, a declaration that has a proper superdeclaration marked final is a compile-time error regardless of the declaration itself having base, final, or sealed modifiers. Some exceptions may apply, based on the version of the libraries and them being core libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

final class can be extended by final, base or sealed classes defined in the same library. One of the tests that checks it (and passes) is https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Class-modifiers/basic_restrictions_A02_t07.dart

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks for the clarification!

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `final`. Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `final`. Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `final`. Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `sealed`. Test
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood the specification correctly, the restriction imposed by the sealed declarations is that they can be extended only in the same library they are declared. It is a compile-time error to use them as superdeclarations outside of their own library, regardless of the modifiers base, final, and sealed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. And we have basic_restrictions_A01_t01.dart (basic_restrictions_A01_t09.dart for type aliases) testsing that it's an error to extend a sealed class outside of its library by a class with any modifier. Deleted this test. Thank you for noticing

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.

Typos fixed, wrong tests deleted. Please take another look


typedef LocalTypedefSealedClass = SealedClass;

mixin MixinOnLocalTypedefSealedClasson LocalTypedefSealedClass {}
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

// [cfe] unspecified

base mixin class MixinClassIndirectlyImplementsBaseClass implements TypedefWithBaseMixinClass {}
// ^^^^^^^^^^^^^^^^^^
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `final`. Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

final class can be extended by final, base or sealed classes defined in the same library. One of the tests that checks it (and passes) is https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Class-modifiers/basic_restrictions_A02_t07.dart

/// marked base, final or sealed.
///
/// @description Check that it is a compile-time error if a declaration is not
/// `base`, `final` or `sealed` and has a superdeclaration marked `sealed`. Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. And we have basic_restrictions_A01_t01.dart (basic_restrictions_A01_t09.dart for type aliases) testsing that it's an error to extend a sealed class outside of its library by a class with any modifier. Deleted this test. Thank you for noticing

@sgrekhov sgrekhov requested a review from chloestefantsova May 3, 2023 13:54
Copy link
Contributor

@chloestefantsova chloestefantsova left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@sgrekhov
Copy link
Contributor Author

sgrekhov commented May 9, 2023

Merge conflicts resolved. Please merge this PR

@chloestefantsova chloestefantsova merged commit 56fc2a7 into dart-lang:master May 10, 2023
@sgrekhov sgrekhov deleted the co19-1959-7 branch July 18, 2023 11:31
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