sealed class ClassBody implements AstNode {}
I like `ClassBody`, but it would be more consistent if we also used it for extensions and mixins. I understand that only one of the two subclasses could validly be used in either of these cases, but I think the consistency (and possibly the ability to pass a `ClassBody` around no matter what kind of parent it has) would make it worthwhile.
It would also be a good way to handle recovery if someone types an extension or a mixing with an empty body. It would make it easier for the fix to find the semicolon.
sealed class ClassNamePart implements AstNode {
I don't like the name `ClassNamePart` because it's more than the name of the class. In it's most complete form it represents the name, type parameters, and primary constructor.
What would you think of calling this a `ClassHeader`? I'm not completely thrilled with the name because I would tend to include the `extends`, `with`, and `implements` clauses in the header, but I don't have a better name at this point and I think it might be better than `ClassNamePart`.
Also, as with `ClassBody`, it would be more consistent if we used this class for all of the top-level declarations that can have both a name and type parameters, even though some of those declarations can't have a primary constructor and hence would never return one of the two subclasses.
final class IdentifierWithTypeParametersImpl extends ClassNamePartImpl
Would it be worthwhile to have a `ClassHeaderWithIdentifierImpl` subclass for cases where there are no type parameters (with a getter that just returns `null`)? Just a thought, not something I care about either way.
abstract final class PrimaryConstructorName implements AstNode {
The classes `PrimaryConstructorName` and `PrimaryConstructorNameImpl` don't appear to be used. I'm not sure whether I'm just missing something or whether you forgot to remove them.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
sealed class ClassBody implements AstNode {}
I like `ClassBody`, but it would be more consistent if we also used it for extensions and mixins. I understand that only one of the two subclasses could validly be used in either of these cases, but I think the consistency (and possibly the ability to pass a `ClassBody` around no matter what kind of parent it has) would make it worthwhile.
It would also be a good way to handle recovery if someone types an extension or a mixing with an empty body. It would make it easier for the fix to find the semicolon.
I agree that we can at least use `ClassBodyWithMembers` for extensions and mixins, this already will let us pass it as `ClassBody` somewhere.
I'm not sure whether going up to `ClassBody` is useful. There is a small drawback, we would have to check the type. Recovery can be tricky, but it feels wrong to tie representation to recovery. There are many ways to wring wrong class body, e.g. unbalanced `{` and `}`, or write nothing at all, and using `;` is just one of them.
We also have enums, which unfortunately does not fit at all: we have `leftBracket`, `constants`, `semicolon`, `members`, `rightBracket`. Theoretically `EnumBody` is a superset of `ClassBodyWithMembers`, adding `constants` and `semicolon`. But AFAIK we previously avoided subclasses concrete node classes. And this would also violate LSP, e.g. removing the first member is not removing everything from the end of this member to `{`, there are constants now. So, maybe make `EnumBody` a subtype of `ClassBody`?
sealed class ClassNamePart implements AstNode {
I don't like the name `ClassNamePart` because it's more than the name of the class. In it's most complete form it represents the name, type parameters, and primary constructor.
What would you think of calling this a `ClassHeader`? I'm not completely thrilled with the name because I would tend to include the `extends`, `with`, and `implements` clauses in the header, but I don't have a better name at this point and I think it might be better than `ClassNamePart`.
Also, as with `ClassBody`, it would be more consistent if we used this class for all of the top-level declarations that can have both a name and type parameters, even though some of those declarations can't have a primary constructor and hence would never return one of the two subclasses.
Yeah, unfortunately I also consider "header" to be everything from modifiers and `class` keyword to the body of the class, so including `extends`, `with`, and `implements`. So, I don't like calling it `ClassHeader`. It is a part of the declaration that contains the name (1), so `ClassNamePart`. Using the word "part" also feel lame to me, but it has benefits: short and descriptive (if you buy my explanation (1)).
abstract final class PrimaryConstructorName implements AstNode {
The classes `PrimaryConstructorName` and `PrimaryConstructorNameImpl` don't appear to be used. I'm not sure whether I'm just missing something or whether you forgot to remove them.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
sealed class BlockClassBody implements ClassBody {
I like this name better.
sealed class ClassBody implements AstNode {}
Konstantin ShcheglovI like `ClassBody`, but it would be more consistent if we also used it for extensions and mixins. I understand that only one of the two subclasses could validly be used in either of these cases, but I think the consistency (and possibly the ability to pass a `ClassBody` around no matter what kind of parent it has) would make it worthwhile.
It would also be a good way to handle recovery if someone types an extension or a mixing with an empty body. It would make it easier for the fix to find the semicolon.
I agree that we can at least use `ClassBodyWithMembers` for extensions and mixins, this already will let us pass it as `ClassBody` somewhere.
I'm not sure whether going up to `ClassBody` is useful. There is a small drawback, we would have to check the type. Recovery can be tricky, but it feels wrong to tie representation to recovery. There are many ways to wring wrong class body, e.g. unbalanced `{` and `}`, or write nothing at all, and using `;` is just one of them.
We also have enums, which unfortunately does not fit at all: we have `leftBracket`, `constants`, `semicolon`, `members`, `rightBracket`. Theoretically `EnumBody` is a superset of `ClassBodyWithMembers`, adding `constants` and `semicolon`. But AFAIK we previously avoided subclasses concrete node classes. And this would also violate LSP, e.g. removing the first member is not removing everything from the end of this member to `{`, there are constants now. So, maybe make `EnumBody` a subtype of `ClassBody`?
I'm not sure whether going up to `ClassBody` is useful.
I'm fine with using the more specific subtype where applicable.
... it feels wrong to tie representation to recovery.
It feels wrong to consider representation to be more important than the UX, and recovery is a big part of the UX. So I think that feeling is not a good design principle.
Of course, in this particular case we could still search through the token stream to find the semicolon in order to fix the problem, so this is more about the implementation cost than it is about the UX, so I'm not going to fight for making the type more general.
But AFAIK we previously avoided subclasses concrete node classes.
True.
And this would also violate LSP ...
I don't understand that point.
So, maybe make `EnumBody` a subtype of `ClassBody`?
I think that would be reasonable. The presence of enum constants makes it feel less consistent than I was originally thinking it would be, but it would probably prove to be a good choice anyway.
sealed class ClassNamePart implements AstNode {
Konstantin ShcheglovI don't like the name `ClassNamePart` because it's more than the name of the class. In it's most complete form it represents the name, type parameters, and primary constructor.
What would you think of calling this a `ClassHeader`? I'm not completely thrilled with the name because I would tend to include the `extends`, `with`, and `implements` clauses in the header, but I don't have a better name at this point and I think it might be better than `ClassNamePart`.
Also, as with `ClassBody`, it would be more consistent if we used this class for all of the top-level declarations that can have both a name and type parameters, even though some of those declarations can't have a primary constructor and hence would never return one of the two subclasses.
Yeah, unfortunately I also consider "header" to be everything from modifiers and `class` keyword to the body of the class, so including `extends`, `with`, and `implements`. So, I don't like calling it `ClassHeader`. It is a part of the declaration that contains the name (1), so `ClassNamePart`. Using the word "part" also feel lame to me, but it has benefits: short and descriptive (if you buy my explanation (1)).
I agree that 'part' feels lame. Yes, this subsection of the grammar is part of the declaration, but `ClassNamePart` sounds like it's claiming to be part of the _name_, which it isn't. The grammar piece is also part of the header, so `ClassHeaderPart` would be more accurate, though only slightly better.
The problem is that there's no coherent concept behind this production other than to avoid duplication in the grammar (and by extension in the node structure). It isn't a thing, it's just a semi-random collection of nodes. But `SharedSubsetOfAClassDeclaration` would be horrid.
We could also take inspiration from a comment above ("The name of the class ... or a primary constructor.") and call it `ClassNameOrPrimaryConstructor`, since it's a thing that could be either of those.
I don't have a good solution. :-/
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
sealed class ClassBody implements AstNode {}
Konstantin ShcheglovI like `ClassBody`, but it would be more consistent if we also used it for extensions and mixins. I understand that only one of the two subclasses could validly be used in either of these cases, but I think the consistency (and possibly the ability to pass a `ClassBody` around no matter what kind of parent it has) would make it worthwhile.
It would also be a good way to handle recovery if someone types an extension or a mixing with an empty body. It would make it easier for the fix to find the semicolon.
Brian WilkersonI agree that we can at least use `ClassBodyWithMembers` for extensions and mixins, this already will let us pass it as `ClassBody` somewhere.
I'm not sure whether going up to `ClassBody` is useful. There is a small drawback, we would have to check the type. Recovery can be tricky, but it feels wrong to tie representation to recovery. There are many ways to wring wrong class body, e.g. unbalanced `{` and `}`, or write nothing at all, and using `;` is just one of them.
We also have enums, which unfortunately does not fit at all: we have `leftBracket`, `constants`, `semicolon`, `members`, `rightBracket`. Theoretically `EnumBody` is a superset of `ClassBodyWithMembers`, adding `constants` and `semicolon`. But AFAIK we previously avoided subclasses concrete node classes. And this would also violate LSP, e.g. removing the first member is not removing everything from the end of this member to `{`, there are constants now. So, maybe make `EnumBody` a subtype of `ClassBody`?
I'm not sure whether going up to `ClassBody` is useful.
I'm fine with using the more specific subtype where applicable.
... it feels wrong to tie representation to recovery.
It feels wrong to consider representation to be more important than the UX, and recovery is a big part of the UX. So I think that feeling is not a good design principle.
Of course, in this particular case we could still search through the token stream to find the semicolon in order to fix the problem, so this is more about the implementation cost than it is about the UX, so I'm not going to fight for making the type more general.
But AFAIK we previously avoided subclasses concrete node classes.
True.
And this would also violate LSP ...
I don't understand that point.
So, maybe make `EnumBody` a subtype of `ClassBody`?
I think that would be reasonable. The presence of enum constants makes it feel less consistent than I was originally thinking it would be, but it would probably prove to be a good choice anyway.
I'm not sure whether going up to `ClassBody` is useful.
I'm fine with using the more specific subtype where applicable.
... it feels wrong to tie representation to recovery.
It feels wrong to consider representation to be more important than the UX, and recovery is a big part of the UX. So I think that feeling is not a good design principle.
This is fair, I agree that UX is the starting point.
> Of course, in this particular case we could still search through the token stream to find the semicolon in order to fix the problem, so this is more about the implementation cost than it is about the UX, so I'm not going to fight for making the type more general.
Yes, this is what I thought, that we have ability to implement the fix without necessary representing it as a node.
> But AFAIK we previously avoided subclasses concrete node classes.True.
And this would also violate LSP ...
I don't understand that point.
LSP here is "Liskov Substitution Principle".
> > So, maybe make `EnumBody` a subtype of `ClassBody`?
>
> I think that would be reasonable. The presence of enum constants makes it feel less consistent than I was originally thinking it would be, but it would probably prove to be a good choice anyway.
OK, will update the CL a bit later, after another discussion.
sealed class ClassNamePart implements AstNode {
Konstantin ShcheglovI don't like the name `ClassNamePart` because it's more than the name of the class. In it's most complete form it represents the name, type parameters, and primary constructor.
What would you think of calling this a `ClassHeader`? I'm not completely thrilled with the name because I would tend to include the `extends`, `with`, and `implements` clauses in the header, but I don't have a better name at this point and I think it might be better than `ClassNamePart`.
Also, as with `ClassBody`, it would be more consistent if we used this class for all of the top-level declarations that can have both a name and type parameters, even though some of those declarations can't have a primary constructor and hence would never return one of the two subclasses.
Brian WilkersonYeah, unfortunately I also consider "header" to be everything from modifiers and `class` keyword to the body of the class, so including `extends`, `with`, and `implements`. So, I don't like calling it `ClassHeader`. It is a part of the declaration that contains the name (1), so `ClassNamePart`. Using the word "part" also feel lame to me, but it has benefits: short and descriptive (if you buy my explanation (1)).
I agree that 'part' feels lame. Yes, this subsection of the grammar is part of the declaration, but `ClassNamePart` sounds like it's claiming to be part of the _name_, which it isn't. The grammar piece is also part of the header, so `ClassHeaderPart` would be more accurate, though only slightly better.
The problem is that there's no coherent concept behind this production other than to avoid duplication in the grammar (and by extension in the node structure). It isn't a thing, it's just a semi-random collection of nodes. But `SharedSubsetOfAClassDeclaration` would be horrid.
We could also take inspiration from a comment above ("The name of the class ... or a primary constructor.") and call it `ClassNameOrPrimaryConstructor`, since it's a thing that could be either of those.
I don't have a good solution. :-/
I discussed this with LLMs, and got a few options.
`ClassDesignator`, field `designator`.
`ClassNameSpecifier`, field `nameSpecifier`.
`ClassDeclarator` - probably not good, too close to `ClassDeclaration`, field `declarator`.
Also `ClassNameDesignator`, but it might be "wet water"; although I like that it says "name" explicitly, field `nameDesignator`.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
sealed class ClassNamePart implements AstNode {
Konstantin ShcheglovI don't like the name `ClassNamePart` because it's more than the name of the class. In it's most complete form it represents the name, type parameters, and primary constructor.
What would you think of calling this a `ClassHeader`? I'm not completely thrilled with the name because I would tend to include the `extends`, `with`, and `implements` clauses in the header, but I don't have a better name at this point and I think it might be better than `ClassNamePart`.
Also, as with `ClassBody`, it would be more consistent if we used this class for all of the top-level declarations that can have both a name and type parameters, even though some of those declarations can't have a primary constructor and hence would never return one of the two subclasses.
Brian WilkersonYeah, unfortunately I also consider "header" to be everything from modifiers and `class` keyword to the body of the class, so including `extends`, `with`, and `implements`. So, I don't like calling it `ClassHeader`. It is a part of the declaration that contains the name (1), so `ClassNamePart`. Using the word "part" also feel lame to me, but it has benefits: short and descriptive (if you buy my explanation (1)).
Konstantin ShcheglovI agree that 'part' feels lame. Yes, this subsection of the grammar is part of the declaration, but `ClassNamePart` sounds like it's claiming to be part of the _name_, which it isn't. The grammar piece is also part of the header, so `ClassHeaderPart` would be more accurate, though only slightly better.
The problem is that there's no coherent concept behind this production other than to avoid duplication in the grammar (and by extension in the node structure). It isn't a thing, it's just a semi-random collection of nodes. But `SharedSubsetOfAClassDeclaration` would be horrid.
We could also take inspiration from a comment above ("The name of the class ... or a primary constructor.") and call it `ClassNameOrPrimaryConstructor`, since it's a thing that could be either of those.
I don't have a good solution. :-/
I discussed this with LLMs, and got a few options.
`ClassDesignator`, field `designator`.
`ClassNameSpecifier`, field `nameSpecifier`.
`ClassDeclarator` - probably not good, too close to `ClassDeclaration`, field `declarator`.
Also `ClassNameDesignator`, but it might be "wet water"; although I like that it says "name" explicitly, field `nameDesignator`.
My two cents is I'm fine with either `ClassNamePart`, `ClassHeaderPart` or `ClassHeader`. Naming it designator or specifier isn't super clear IMO.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
sealed class ClassNamePart implements AstNode {
Konstantin ShcheglovI don't like the name `ClassNamePart` because it's more than the name of the class. In it's most complete form it represents the name, type parameters, and primary constructor.
What would you think of calling this a `ClassHeader`? I'm not completely thrilled with the name because I would tend to include the `extends`, `with`, and `implements` clauses in the header, but I don't have a better name at this point and I think it might be better than `ClassNamePart`.
Also, as with `ClassBody`, it would be more consistent if we used this class for all of the top-level declarations that can have both a name and type parameters, even though some of those declarations can't have a primary constructor and hence would never return one of the two subclasses.
Brian WilkersonYeah, unfortunately I also consider "header" to be everything from modifiers and `class` keyword to the body of the class, so including `extends`, `with`, and `implements`. So, I don't like calling it `ClassHeader`. It is a part of the declaration that contains the name (1), so `ClassNamePart`. Using the word "part" also feel lame to me, but it has benefits: short and descriptive (if you buy my explanation (1)).
Konstantin ShcheglovI agree that 'part' feels lame. Yes, this subsection of the grammar is part of the declaration, but `ClassNamePart` sounds like it's claiming to be part of the _name_, which it isn't. The grammar piece is also part of the header, so `ClassHeaderPart` would be more accurate, though only slightly better.
The problem is that there's no coherent concept behind this production other than to avoid duplication in the grammar (and by extension in the node structure). It isn't a thing, it's just a semi-random collection of nodes. But `SharedSubsetOfAClassDeclaration` would be horrid.
We could also take inspiration from a comment above ("The name of the class ... or a primary constructor.") and call it `ClassNameOrPrimaryConstructor`, since it's a thing that could be either of those.
I don't have a good solution. :-/
Kallen TuI discussed this with LLMs, and got a few options.
`ClassDesignator`, field `designator`.
`ClassNameSpecifier`, field `nameSpecifier`.
`ClassDeclarator` - probably not good, too close to `ClassDeclaration`, field `declarator`.
Also `ClassNameDesignator`, but it might be "wet water"; although I like that it says "name" explicitly, field `nameDesignator`.
Brian WilkersonMy two cents is I'm fine with either `ClassNamePart`, `ClassHeaderPart` or `ClassHeader`. Naming it designator or specifier isn't super clear IMO.
I agree that the names from the LLM aren't good.
`ClassDesignator` is what a class name is when used elsewhere in code (such as in a type annotation), not what the declaration of the class name is.
`ClassNameDesignator` is even worse. It means a thing that refers to the name of the class, not to the class itself. Too many levels of indirection to be useful.
`ClassNamePart` and `ClassNameSpecifier` are too limited; they sound like they're only the part that declares the class name.
`ClassHeader` is too grandiose; it's not the whole header.
`ClassHeaderPart` is accurate, but vague; it could be any part of the class header. Nevertheless, it's the best we've come up with so far.
As I said before, the problem is that there's no semantic for these two pieces of the class header, so naming it is really hard. Vague but accurate might be the best we can hope for. That or not trying to artificially group them together.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Now, with your explanation, I see that `ClassDesignator` and `ClassNameDesignator` are not good.
`NodeWithClassName`, `ClassNameContainer`, `ClassName`.
I read `ClassNamePart` with the right pause: "class" <pause> "name part".
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
sealed class ClassBody implements AstNode {}
I like `ClassBody`, but it would be more consistent if we also used it for extensions and mixins. I understand that only one of the two subclasses could validly be used in either of these cases, but I think the consistency (and possibly the ability to pass a `ClassBody` around no matter what kind of parent it has) would make it worthwhile.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
sealed class ClassBody implements AstNode {}
Thanks.
I still don't like it. It isn't the 'name' part, it's the 'name, type parameters, and possibly primary constructor part'.
But it isn't reasonable to block a CL for an issue that nobody can solve, so I won't.
/// The body of the class declaration.
nit: "class" --> "enum"
(Not worth holding up the CL over.)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
nit: "class" --> "enum"
(Not worth holding up the CL over.)
Done
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
7 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/ast/ast.dart
Insertions: 1, Deletions: 1.
@@ -7484,7 +7484,7 @@
/// The `augment` keyword, or `null` if the keyword was absent.
Token? get augmentKeyword;
- /// The body of the class declaration.
+ /// The body of the enum declaration.
///
/// Will become not `null` when [Feature.declaring_constructors] is
/// implemented, replaces [leftBracket], [constants], [semicolon], [members],
```
DeCo. AST for PrimaryConstructorDeclaration and ClassBody.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
/// implemented, replaces [name].
Shouldn't this be `replaces [name] and [typeParameters]`?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
/// implemented, replaces [name].
Shouldn't this be `replaces [name] and [typeParameters]`?
This was fixed in a way in https://dart-review.googlesource.com/c/sdk/+/455820, replaced completely.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |