-
Notifications
You must be signed in to change notification settings - Fork 198
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
Update class modifiers specification. #2889
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A couple of comments added.
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
For the record, I continue to think this is a mistake, for all the reasons laid out in #2723 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very much not on board with this change. If we actually feel that we need to ability to have variants of these that can be mixed in internally but not externally, we should instead remove the restriction that you cannot mix in a class which is not marked as mixin
inside of your own library.
Reverted the change to allow PTAL |
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
* Allow `interface mixin class` etc. * Tighten the modifiers implied on anonymous mixin class applications. * A little rephrasing, to allow reusing a concept.
But do allow mixing in non-mixin platform classes from pre-feature libraries.
Fix grammatical cases.
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Indicated a handful of typo level issues (syntax errors in examples and such).
More importantly, the definition of 'prohibits inheritance' talks about 'declarations', and the actual wording of subsequent lines does not make sense unless we count each anonymous mixin application as a 'declaration' and consider that declaration to be 'marked with' each of its implicitly induced modifiers.
However, I still think the rules about anonymous mixin applications are simpler to understand if they are expressed directly in terms of the class modifiers (sealed
as well as final
/base
/interface
/none, explicitly specified as well as implicitly induced), so I inserted a 'suggestion' about how that could be done.
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
// These are valid, but cannot be implemented locally: | ||
sealed class DE extends S {} | ||
final class DM with S {} | ||
base mixin MO on S {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems worthwhile to show that a mixin on S
is usable, and to mention that the resulting class also cannot be implemented locally.
base mixin MO on S {} | |
base mixin MO on S {} | |
final class DMO extends DM with MO {} |
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments.
accepted/future-releases/class-modifiers/feature-specification.md
Outdated
Show resolved
Hide resolved
As of dart-lang/language#2889 it is an error to declare a mixin with an `on` type which is declared in a different library. Change-Id: I02d6b3b6c203a46c74d2eacd006181db8e563726 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/290022 Reviewed-by: Kallen Tu <kallentu@google.com>
enum
declarations arefinal
.But do not allow
interface mixin class
etc. anyway.@munificent @kallentu