Skip to content

Implement new-style mixin support for Dart 2.1 #1765

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

Merged
merged 14 commits into from
Oct 4, 2018
Merged

Implement new-style mixin support for Dart 2.1 #1765

merged 14 commits into from
Oct 4, 2018

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Sep 27, 2018

Fixes #1752, #1778, #1779.

This drops support for Dart 2.0.

As a side effect of this change, type inference is more accurate for deep inheritance cases and class members are more likely to correctly pick up documentation from superclasses. Additionally, some warnings WRT comment references inherited from superclasses referencing renamed type and parameter variables in subclasses were accidentally suppressed, and are not suppressed anymore (approximately doubling the raw number of warnings in the flutter repository).

Obligatory screenshots:
screenshot from 2018-09-27 15-17-13

and:

screenshot from 2018-09-27 15-15-59

.travis.yml Outdated
@@ -2,7 +2,6 @@ language: dart
sudo: false
dart:
- "dev/raw/latest"
- stable
Copy link
Member

Choose a reason for hiding this comment

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

We may not need to omit this; I think at least the analyzer would be happy with both 2.01 and 2.1, but your call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored testing with stable, as it seems to work.

@devoncarew
Copy link
Member

@bwilkerson, can you review this?

@jcollins-g
Copy link
Contributor Author

Reenabled stable tests in travis and changed version range to allow 2.0. We still require a version override due to dart-lang/build#1873 but we can hack around that to publish.

@@ -2681,7 +2776,11 @@ abstract class ModelElement extends Canonicalization
// Also handles enums
if (e is ClassElement) {
if (!e.isEnum) {
newModelElement = new Class(e, library, packageGraph);
if (e is MixinElementImpl) {
Copy link
Member

Choose a reason for hiding this comment

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

"e is MixinElementImpl" --> "e.isMixin"

Also, this would look cleaner to me as

if (e.isMixin) {
} else if (e.isEnum) {
} else {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

AstNode get astNode {
if (_astNode == null) {
_astNode = element?.computeNode();
}
Copy link
Member

Choose a reason for hiding this comment

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

Or replace the 'if' with "_astNode ??= element?.computeNode();"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -3314,12 +3441,14 @@ abstract class ModelElement extends Canonicalization
@override
String get name => element.name;

// TODO(jcollins-g): refactor once mixins can call super
Copy link
Member

Choose a reason for hiding this comment

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

If by "mixins" you mean mixin declarations, they already can call super (as long as the invoked method is defined by a superclass constraint).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified to make it clear that we have to go to Dart 2.1+ only to refactor this code. Head dartdoc still can run on Dart 2.0.

@jcollins-g jcollins-g merged commit 215ca55 into master Oct 4, 2018
@jcollins-g jcollins-g deleted the mixins branch October 4, 2018 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants