[XL] Change in dart/sdk[main]: DeCo. Build elements from new ASTs.
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
Related details
- Johnni Winther
- Konstantin Shcheglov
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Berry (Gerrit)
Paul Berry voted and added 13 comments![]( "Open in Gerrit")
Votes added by Paul Berry
| Code-Review | +1 |
13 comments
lgtm assuming comments are addressed.
if (member.body case BlockClassBody body) {I believe this code is correct, but it's hard to tell why. You have to know that there are only two possible types that `member.body` can have in practice: `BlockClassBody` and `EmptyClassBody`; if you know that, then you can see that for an empty class body there's nothing that needs to be done.
What if we changed line 32 to `case ClassDeclarationImpl()` and then changed this `if` statement to a `switch` statement, so that exhaustiveness checking would ensure that all cases are handled? E.g.:
```
switch (member.body) {
case BlockClassBodyImpl body:
body.members.forEach(appendClassMemberName);
case EmptyClassBodyImpl():
// An empty class body declarations.
break;
case EnumBodyImpl():
assert(
false,
'A class declaration should never have an enum body',
);
}
```
Alternatively, add an abstract `members` getter to `ClassBodyImpl` and provide an implementation in `EmptyClassBodyImpl` that returns `const []`. Then this `if` wouldn't be needed, and we could just unconditionally do:
```
member.body.members.forEach(appendClassMemberName);
```
Aside: why is `EnumBodyImpl` a subtype of `ClassBodyImpl`? It seems like `EnumBodyImpl` should just extend `AstNodeImpl` (and similarly, `EnumBody` should just implement `AstNode`). After all, an enum body can't be used where a class body is expected, right?
if (member.body case BlockClassBody body) {Similar issue here
if (node.body case BlockClassBody body) {Similar issue here
if (node.body case BlockClassBody body) {Similar issue here
if (useDeclaringConstructorsAst) {It should be possible to do this without so much code duplication:
```
void _addEnum(EnumDeclaration node) {
var members = useDeclaringConstructorsAst
? node.body.members
: node.members;
// If not enhanced, include the whole node.
var firstMember = members.firstOrNull;
if (firstMember == null) {
_addNode(node);
return;
}
_addTokens(node.beginToken, firstMember.beginToken);
_addClassMembers(members, true);
}
```
if (node.body case BlockClassBody body) {Similar issue to the issue raised in `pkg/analyzer/lib/src/dart/analysis/defined_names.dart`.
void _builtRepresentationDeclaration2({There's a lot of code duplication between `_builtRepresentationDeclaration` and `_builtRepresentationDeclaration2`. How about if we move the differences to the call site so that a single method can be used? Something like:
```
({
FieldFragmentImpl fieldFragment,
ConstructorFragmentImpl constructorFragment,
FieldFormalParameterFragmentImpl formalParameterFragment,
})
_builtRepresentationDeclaration({
required ExtensionTypeFragmentImpl extensionFragment,
required PrimaryConstructorDeclarationImpl representation,
required TypeAnnotationImpl fieldType,
required Token? fieldNameToken,
required NodeListImpl<AnnotationImpl> fieldMetadata,
}) {
var fieldFragment = FieldFragmentImpl(
name: _getFragmentName(fieldNameToken),
);
fieldFragment.isAugmentation = extensionFragment.isAugmentation;
fieldFragment.isFinal = true;
fieldFragment.metadata = _buildMetadata(fieldMetadata);
_linker.elementNodes[fieldFragment] = representation;
_addChildFragment(fieldFragment);
var formalParameterFragment = FieldFormalParameterFragmentImpl(
name: _getFragmentName(fieldNameToken),
nameOffset: null,
parameterKind: ParameterKind.REQUIRED,
)..hasImplicitType = true;
var constructorFragment =
ConstructorFragmentImpl(
name: representation.constructorName?.name.lexeme ?? 'new',
)
..isAugmentation = extensionFragment.isAugmentation
..isConst = representation.constKeyword != null
..formalParameters = [formalParameterFragment];
constructorFragment.typeName = extensionFragment.name;
_linker.elementNodes[constructorFragment] = representation;
_addChildFragment(constructorFragment);
fieldType.accept(this);
return (
fieldFragment: fieldFragment,
constructorFragment: constructorFragment,
formalParameterFragment: formalParameterFragment,
);
}
```
if (useDeclaringConstructorsAst) {It should be possible to avoid the code duplication by pushing the test into the `members` expression:
```
_InfoExtensionDeclaration _buildExtension(ExtensionDeclaration node) {
return _InfoExtensionDeclaration(
data: _buildInstanceData(
node,
name: node.name,
typeParameters: node.typeParameters,
members: useDeclaringConstructorsAst ? node.body.members : node.members,
),
);
}
```
if (useDeclaringConstructorsAst) {Similar issue here.
_InfoExtensionTypeRepresentation _buildRepresentation2(Can we use a more descriptive name here? `2` doesn't convey any useful information.
class ClassElementTest_fromBytes2 extends ClassElementTest {Similar issue here (need a more descriptive name suffix than `2`).
class ClassElementTest_keepLinking2 extends ClassElementTest {Similar issue here, and also in the files that follow.
Related details
- Johnni Winther
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 12 comments![]( "Open in Gerrit")
I believe this code is correct, but it's hard to tell why. You have to know that there are only two possible types that `member.body` can have in practice: `BlockClassBody` and `EmptyClassBody`; if you know that, then you can see that for an empty class body there's nothing that needs to be done.
What if we changed line 32 to `case ClassDeclarationImpl()` and then changed this `if` statement to a `switch` statement, so that exhaustiveness checking would ensure that all cases are handled? E.g.:
```
switch (member.body) {
case BlockClassBodyImpl body:
body.members.forEach(appendClassMemberName);
case EmptyClassBodyImpl():
// An empty class body declarations.
break;
case EnumBodyImpl():
assert(
false,
'A class declaration should never have an enum body',
);
}
```Alternatively, add an abstract `members` getter to `ClassBodyImpl` and provide an implementation in `EmptyClassBodyImpl` that returns `const []`. Then this `if` wouldn't be needed, and we could just unconditionally do:
```
member.body.members.forEach(appendClassMemberName);
```Aside: why is `EnumBodyImpl` a subtype of `ClassBodyImpl`? It seems like `EnumBodyImpl` should just extend `AstNodeImpl` (and similarly, `EnumBody` should just implement `AstNode`). After all, an enum body can't be used where a class body is expected, right?
I added `members` to `ClassBodyImpl`.
And in a separate CL `EnumBodyImpl` is not `ClassBodyImpl` anymore.
if (member.body case BlockClassBody body) {Konstantin ShcheglovSimilar issue here
Done
if (node.body case BlockClassBody body) {Konstantin ShcheglovSimilar issue here
Done
if (node.body case BlockClassBody body) {Konstantin ShcheglovSimilar issue here
Done
It should be possible to do this without so much code duplication:
```
void _addEnum(EnumDeclaration node) {
var members = useDeclaringConstructorsAst
? node.body.members
: node.members;// If not enhanced, include the whole node.
var firstMember = members.firstOrNull;
if (firstMember == null) {
_addNode(node);
return;
}_addTokens(node.beginToken, firstMember.beginToken);
_addClassMembers(members, true);
}
```
Done
Similar issue to the issue raised in `pkg/analyzer/lib/src/dart/analysis/defined_names.dart`.
Done. Actually here it cannot be anything else than `BlockClassBody`.
void _builtRepresentationDeclaration2({I decided that here I prefer simplicity over code duplication.
It should be possible to avoid the code duplication by pushing the test into the `members` expression:
```
_InfoExtensionDeclaration _buildExtension(ExtensionDeclaration node) {
return _InfoExtensionDeclaration(
data: _buildInstanceData(
node,
name: node.name,
typeParameters: node.typeParameters,
members: useDeclaringConstructorsAst ? node.body.members : node.members,
),
);
}
```
Done
if (useDeclaringConstructorsAst) {Konstantin ShcheglovSimilar issue here.
Done
Can we use a more descriptive name here? `2` doesn't convey any useful information.
Done
class ClassElementTest_fromBytes2 extends ClassElementTest {Similar issue here (need a more descriptive name suffix than `2`).
Done
class ClassElementTest_keepLinking2 extends ClassElementTest {Similar issue here, and also in the files that follow.
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/analyzer/lib/src/dart/analysis/referenced_names.dart
Insertions: 19, Deletions: 20.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/lib/src/dart/analysis/unlinked_api_signature.dart
Insertions: 20, Deletions: 35.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/lib/src/dart/ast/ast.dart
Insertions: 6, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/lib/src/summary2/element_builder.dart
Insertions: 47, Deletions: 46.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/lib/src/summary2/informative_data.dart
Insertions: 42, Deletions: 63.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/test/src/summary/elements/class_test.dart
Insertions: 6, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/test/src/summary/elements/enum_test.dart
Insertions: 5, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/test/src/summary/elements/extension_test.dart
Insertions: 8, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/test/src/summary/elements/extension_type_test.dart
Insertions: 10, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/test/src/summary/elements/mixin_test.dart
Insertions: 6, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/analyzer/lib/src/dart/analysis/defined_names.dart
Insertions: 5, Deletions: 8.
The diff is too large to show. Please review the diff.
```
Change information
DeCo. Build elements from new ASTs.
For now just to understand the same syntax represented with new AST
nodes. We now run element tests for classes, enums, extensions,
extension types, and mixins in both modes: old AST and new AST.
- M pkg/analyzer/api.txt
- M pkg/analyzer/lib/dart/ast/ast.dart
- M pkg/analyzer/lib/src/dart/analysis/defined_names.dart
- M pkg/analyzer/lib/src/dart/analysis/driver.dart
- M pkg/analyzer/lib/src/dart/analysis/file_state.dart
- M pkg/analyzer/lib/src/dart/analysis/referenced_names.dart
- M pkg/analyzer/lib/src/dart/analysis/unlinked_api_signature.dart
- M pkg/analyzer/lib/src/dart/ast/ast.dart
- M pkg/analyzer/lib/src/dartdoc/dartdoc_directive_info.dart
- M pkg/analyzer/lib/src/fasta/ast_builder.dart
- M pkg/analyzer/lib/src/summary2/default_types_builder.dart
- M pkg/analyzer/lib/src/summary2/element_builder.dart
- M pkg/analyzer/lib/src/summary2/extension_type.dart
- M pkg/analyzer/lib/src/summary2/informative_data.dart
- M pkg/analyzer/lib/src/summary2/library_builder.dart
- M pkg/analyzer/lib/src/summary2/metadata_resolver.dart
- M pkg/analyzer/lib/src/summary2/reference_resolver.dart
- M pkg/analyzer/lib/src/summary2/simply_bounded.dart
- M pkg/analyzer/lib/src/summary2/type_alias.dart
- M pkg/analyzer/lib/src/summary2/variance_builder.dart
- M pkg/analyzer/test/src/diagnostics/parser_diagnostics.dart
- M pkg/analyzer/test/src/summary/elements/class_test.dart
- M pkg/analyzer/test/src/summary/elements/enum_test.dart
- M pkg/analyzer/test/src/summary/elements/extension_test.dart
- M pkg/analyzer/test/src/summary/elements/extension_type_test.dart
- M pkg/analyzer/test/src/summary/elements/mixin_test.dart
Code-Review: +1 by Brian Wilkerson, +1 by Paul Berry
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |