[XL] Change in dart/sdk[main]: DeCo. Add AST for primary constructors behind feature flag.
Paul Berry (Gerrit)
Paul Berry added 1 comment![]( "Open in Gerrit")
I feel like before landing this, it would be good to have a quick design review of what you plan for the ASTs to look like. Could you share a document with some examples of the declaring constructors syntax and what the corresponding ASTs would look like? I would find that way easier to review than jumping straight into looking at code right now.
Related details
- Brian Wilkerson
- 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 1 comment![]( "Open in Gerrit")
I feel like before landing this, it would be good to have a quick design review of what you plan for the ASTs to look like. Could you share a document with some examples of the declaring constructors syntax and what the corresponding ASTs would look like? I would find that way easier to review than jumping straight into looking at code right now.
1. This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors. In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
2. We don't have AST for in-body constructors yet, initially because I did not have time to design it, and now because it turned out that the language specification is apparently still changing.
Related details
- Brian Wilkerson
- Johnni Winther
- 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 added 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI feel like before landing this, it would be good to have a quick design review of what you plan for the ASTs to look like. Could you share a document with some examples of the declaring constructors syntax and what the corresponding ASTs would look like? I would find that way easier to review than jumping straight into looking at code right now.
1. This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors. In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
2. We don't have AST for in-body constructors yet, initially because I did not have time to design it, and now because it turned out that the language specification is apparently still changing.
This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors.
I don't recall seeing the CL for the primary constructor AST. Would you mind sending me a link to that CL?
In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
It looks like this CL contains over 2000 lines of new parser tests. In order to get an effective overview of your AST design, I would need to see a summary of what is being changed that is much more consise than that. Ideally a couple of paragraphs highlighting the essential differences. Some diagrams would be great too.
Related details
- Brian Wilkerson
- 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 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI feel like before landing this, it would be good to have a quick design review of what you plan for the ASTs to look like. Could you share a document with some examples of the declaring constructors syntax and what the corresponding ASTs would look like? I would find that way easier to review than jumping straight into looking at code right now.
Paul Berry1. This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors. In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
2. We don't have AST for in-body constructors yet, initially because I did not have time to design it, and now because it turned out that the language specification is apparently still changing.
This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors.
I don't recall seeing the CL for the primary constructor AST. Would you mind sending me a link to that CL?
In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
It looks like this CL contains over 2000 lines of new parser tests. In order to get an effective overview of your AST design, I would need to see a summary of what is being changed that is much more consise than that. Ideally a couple of paragraphs highlighting the essential differences. Some diagrams would be great too.
CL with primary constructor AST: https://dart-review.googlesource.com/c/sdk/+/454322
Related details
- Brian Wilkerson
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
2 comments
The api changes lgtm.
useDeclaringConstructorsAst (static getter: bool)I'm surprised that these aren't annotated as being experimental. I'm guessing that it's a bug in the api writer.
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. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI feel like before landing this, it would be good to have a quick design review of what you plan for the ASTs to look like. Could you share a document with some examples of the declaring constructors syntax and what the corresponding ASTs would look like? I would find that way easier to review than jumping straight into looking at code right now.
Paul Berry1. This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors. In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
2. We don't have AST for in-body constructors yet, initially because I did not have time to design it, and now because it turned out that the language specification is apparently still changing.
Konstantin ShcheglovThis is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors.
I don't recall seeing the CL for the primary constructor AST. Would you mind sending me a link to that CL?
In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
It looks like this CL contains over 2000 lines of new parser tests. In order to get an effective overview of your AST design, I would need to see a summary of what is being changed that is much more consise than that. Ideally a couple of paragraphs highlighting the essential differences. Some diagrams would be great too.
CL with primary constructor AST: https://dart-review.googlesource.com/c/sdk/+/454322
Here is a short document for two interesting cases: https://docs.google.com/document/d/1nJsSX7hi2nxWlB47GfyuE8aECi5SN8peOv7kqXGyoQo/edit?resourcekey=0-TEIdcONjnjxNV0Ei-gahSA&tab=t.0
Related details
- Johnni Winther
- 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 added 1 comment![]( "Open in Gerrit")
useDeclaringConstructorsAst (static getter: bool)I'm surprised that these aren't annotated as being experimental. I'm guessing that it's a bug in the api writer.
- Johnni Winther
- Konstantin Shcheglov
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 added 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI feel like before landing this, it would be good to have a quick design review of what you plan for the ASTs to look like. Could you share a document with some examples of the declaring constructors syntax and what the corresponding ASTs would look like? I would find that way easier to review than jumping straight into looking at code right now.
Paul Berry1. This is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors. In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
2. We don't have AST for in-body constructors yet, initially because I did not have time to design it, and now because it turned out that the language specification is apparently still changing.
Konstantin ShcheglovThis is not new AST, this is implementation for parser integration of previously reviewed AST for primary constructors.
I don't recall seeing the CL for the primary constructor AST. Would you mind sending me a link to that CL?
In fact, this CL serves exactly the same document that you are asking, IMHO: see all the parsing tests that show code and corresponding AST dumps. Do you need something more?
It looks like this CL contains over 2000 lines of new parser tests. In order to get an effective overview of your AST design, I would need to see a summary of what is being changed that is much more consise than that. Ideally a couple of paragraphs highlighting the essential differences. Some diagrams would be great too.
Konstantin ShcheglovCL with primary constructor AST: https://dart-review.googlesource.com/c/sdk/+/454322
Here is a short document for two interesting cases: https://docs.google.com/document/d/1nJsSX7hi2nxWlB47GfyuE8aECi5SN8peOv7kqXGyoQo/edit?resourcekey=0-TEIdcONjnjxNV0Ei-gahSA&tab=t.0
Thanks! The CL link and the doc were very helpful.
Paul Berry (Gerrit)
Paul Berry voted and added 4 comments![]( "Open in Gerrit")
Votes added by Paul Berry
| Code-Review | +1 |
4 comments
lgtm assuming comments are addressed.
final class BlockClassBodyImplStub implements BlockClassBodyImpl {Would you mind adding a TODO comment to remind us to remove this later?
E.g.:
```
// TODO(scheglov): Remove this stub once `useDeclaringConstructorsAst` is enabled
// by default.
```
A similar comment applies to `ClassBodyImplStub`, `ClassNamePartImplStub`, `EnumBodyImplStub`, and `RepresentationDeclarationImplStub`.
if (useDeclaringConstructorsAst) {
_becomeParentOf(namePart);
} else {
_becomeParentOf(typeParameters);
}
_becomeParentOf(extendsClause);Would it be possible to add some assertions to make sure that the parts we're not becoming a parent of behave in the expected way? E.g.:
```suggestion
if (useDeclaringConstructorsAst) {
_becomeParentOf(namePart);
assert(identical(typeParameters, namePart.typeParameters));
} else {
_becomeParentOf(typeParameters);
assert(namePart is ClassNamePartImplStub);
}
_becomeParentOf(extendsClause);
```
That would have the advantages of (a) helping to make sure we don't screw up in the AST builder, and (b) documenting (for ourselves) how these internal fields behave when the feature is enabled/disabled.
Similar comments apply to the other uses of `useDeclaringConstructorsAst` in AST node constructors.
useDeclaringConstructorsAst = false;If the test fails before reaching this line, `useDeclaringConstructorsAst` will not be reset to `false`, which might cause other tests to fail in confusing ways.
How about if we create methods in `ParserDiagnosticsTest` (or some other suitable base class or mixin):
```
void enableDeclaringConstructorsAst() {
tearDown(() {
useDeclaringConstructorsAst = false;
});
useDeclaringConstructorsAst = true;
}
void disableDeclaringConstructorsAst() {
useDeclaringConstructorsAst = false;
}
```Then, in these tests, we can replace each call to `useDeclaringConstructorsAst = true` with `enableDeclaringConstructorsAst()` and each call to `useDeclaringConstructorsAst = false` with `disableDeclaringConstructorsAst()`, and then we'll always be guaranteed that each test starts with `useDeclaringConstructorsAst` in the proper state.
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Konstantin Shcheglov
| Commit-Queue | +2 |
3 comments
final class BlockClassBodyImplStub implements BlockClassBodyImpl {Would you mind adding a TODO comment to remind us to remove this later?
E.g.:
```
// TODO(scheglov): Remove this stub once `useDeclaringConstructorsAst` is enabled
// by default.
```A similar comment applies to `ClassBodyImplStub`, `ClassNamePartImplStub`, `EnumBodyImplStub`, and `RepresentationDeclarationImplStub`.
Done
if (useDeclaringConstructorsAst) {
_becomeParentOf(namePart);
} else {
_becomeParentOf(typeParameters);
}
_becomeParentOf(extendsClause);Would it be possible to add some assertions to make sure that the parts we're not becoming a parent of behave in the expected way? E.g.:
```suggestion
if (useDeclaringConstructorsAst) {
_becomeParentOf(namePart);
assert(identical(typeParameters, namePart.typeParameters));
} else {
_becomeParentOf(typeParameters);
assert(namePart is ClassNamePartImplStub);
}
_becomeParentOf(extendsClause);
```That would have the advantages of (a) helping to make sure we don't screw up in the AST builder, and (b) documenting (for ourselves) how these internal fields behave when the feature is enabled/disabled.
Similar comments apply to the other uses of `useDeclaringConstructorsAst` in AST node constructors.
Done
If the test fails before reaching this line, `useDeclaringConstructorsAst` will not be reset to `false`, which might cause other tests to fail in confusing ways.
How about if we create methods in `ParserDiagnosticsTest` (or some other suitable base class or mixin):
```
void enableDeclaringConstructorsAst() {
tearDown(() {
useDeclaringConstructorsAst = false;
});
useDeclaringConstructorsAst = true;
}void disableDeclaringConstructorsAst() {
useDeclaringConstructorsAst = false;
}
```Then, in these tests, we can replace each call to `useDeclaringConstructorsAst = true` with `enableDeclaringConstructorsAst()` and each call to `useDeclaringConstructorsAst = false` with `disableDeclaringConstructorsAst()`, and then we'll always be guaranteed that each test starts with `useDeclaringConstructorsAst` in the proper state.
We already do this, see `ParserDiagnosticsTest` and methods `setUp()` and `tearDown()`.
This is `test_reflective_loader` magic.
Related details
- Johnni Winther
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 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/fasta/ast_builder.dart
Insertions: 13, Deletions: 6.
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: 16, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
DeCo. Add AST for primary constructors behind feature flag.
This change introduces experimental AST support for the "declaring
constructors" (DeCo) feature and wires the parser to build explicit
nodes for primary constructors. The new behavior is gated by a static
switch:
analyzer/dart/ast/ast.dart: useDeclaringConstructorsAst (experimental)
When the flag is **true** (must be set once before parsing and not
changed thereafter):
* Declarations expose structured bodies:
- `ClassDeclaration.body` / `MixinDeclaration.body`
- `EnumDeclaration.body`
- `ExtensionDeclaration.body`
- `ExtensionTypeDeclaration.body`
Legacy bracket/members tokens (`leftBracket`, `members`,
`rightBracket`) throw `UnsupportedError`.
* The declaration "name" area is modeled as `namePart: ClassNamePart`,
which is either:
- `NameWithTypeParameters` (e.g., `A<T>`), or
- `PrimaryConstructorDeclaration` (e.g., `A<T>.named(...)`).
Legacy fields (`name`, `typeParameters`) throw `UnsupportedError`.
For extension types, `representation` also throws under the flag and
is represented via `PrimaryConstructorDeclaration`.
* Child iteration (`childEntities`), visiting (`visitChildren`), and
token boundaries (`endToken`) adapt to the new shape.
Parser changes:
* Build `PrimaryConstructorDeclaration` / `PrimaryConstructorName` via a
new `_PrimaryConstructorBuilder`. For extension types, the existing
representation syntax is mapped to a primary constructor when the flag
is enabled.
* Add `NullValue.PrimaryConstructor` to the stack listener to carry the
parsed primary constructor through the builder pipeline.
Implementation details:
* Introduce small internal *Stub classes (e.g., `ClassBodyImplStub`,
`EnumBodyImplStub`, `RepresentationDeclarationImplStub`) to satisfy
constructor requirements for fields that are not surfaced under the
current mode, avoiding API churn while the feature is gated.
* Update API surface to make `body` and `namePart` available on the
affected declarations and document which legacy getters throw when the
flag is enabled.
Rationale:
This lays the AST groundwork for DeCo while preserving backwards
compatibility by default. Consumers can opt in to validate workflows and
migrate incrementally.
Bug: https://github.com/dart-lang/sdk/issues/61701
- M pkg/_fe_analyzer_shared/lib/src/parser/stack_listener.dart
- M pkg/analyzer/api.txt
- M pkg/analyzer/lib/dart/ast/ast.dart
- M pkg/analyzer/lib/src/dart/ast/ast.dart
- M pkg/analyzer/lib/src/fasta/ast_builder.dart
- D pkg/analyzer/test/generated/declaring_constructor_parser_test.dart
- M pkg/analyzer/test/generated/test_all.dart
- M pkg/analyzer/test/src/dart/parser/class_test.dart
- M pkg/analyzer/test/src/dart/parser/enum_test.dart
- M pkg/analyzer/test/src/dart/parser/extension_test.dart
- M pkg/analyzer/test/src/dart/parser/extension_type_test.dart
- M pkg/analyzer/test/src/dart/parser/mixin_test.dart
- M pkg/analyzer/test/src/diagnostics/parser_diagnostics.dart
- M pkg/analyzer/test/src/summary/resolved_ast_printer.dart
- M pkg/analyzer/tool/generators/ast_generator.dart
Code-Review: +1 by Paul Berry, +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther added 1 comment![]( "Open in Gerrit")
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS fileI guess this was deleted by accident.
Related details
- 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 1 comment![]( "Open in Gerrit")
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS fileI guess this was deleted by accident.
This was intentional. I moved these tests into corresponding `class_test.dart`, `enum_test.dart`, etc. So, that they are grouped by containers for better discovery. In these tests we check not only diagnostics, but also AST structures, so get full coverage.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |