Introduce `tokenSpecSetType` and `syntaxChoicesType` for child nodes in CodeGeneration #2011

Matejkob · 2023-08-06T09:22:37Z

This PR ties back to our discussion in #1908. Specifically, @ahoppen proposed this approach:

I just found out about the usage of capitalized name for the unexpected nodes yesterday as well. The way I would go about this that:

The child names as specified in SyntaxSupport should be lowercased because the name of the property is how we primarily think about the child

Child.name should be private (and will probably store the lowercase name from the Child initializer)

There’s varName (which I’m renaming to varOrCaseName in Improve modeling of Child in CodeGeneration #1914 to be consistent with Node.varOrCaseName), which should return the lowercase name

I’d introduce typeName, which returns a capitalized version of the child name (same as Child.name currently does). We should be able to use that in all the cases where we currently rely on Child.name being uppercase.

Here are the tweaks I made:

Added a type property to Child. This returns a TypeSyntax built from the capitalized child name. @ahoppen initially suggested using typeName, but I felt that type of TypeSyntax type fit better. I'm open to feedback though – should we stick to typeName of type TokenSyntax?
I replaced references from name to type in some scenarios:
- When creating enum names, like enum accessorSpecifierOptions: TokenSpecSet, and then when used as types, e.g., ElseBody for IfExprSyntax.
- When constructing "unexpected" node names.
Added deprecatedType and hasDeprecatedName properties. This aligns with other parts of Child, and it means we can make deprecatedName private, improving encapsulation.

Matejkob · 2023-08-06T09:38:56Z

CodeGeneration/Sources/SyntaxSupport/Child.swift

@@ -58,16 +58,14 @@ public enum ChildKind {
 /// A child of a node, that may be declared optional or a token with a
 /// restricted subset of acceptable kinds or texts.
 public class Child {
+  /// If the child has been renamed, its old, now deprecated, name.
+  private let deprecatedName: String?
+
  /// The name of the child.
  ///
  /// The first character of the name is always uppercase.
  public let name: String


I've been thinking about privatizing the name property as proposed in the original discussion but we are using it in the role of id to identify children in tests ValidateSyntaxNodes.swift for example: https://github.com/apple/swift-syntax/blob/e340af46cd6725e1567c42adcbb1ec979bf7f7b4/CodeGeneration/Tests/ValidateSyntaxNodes/ValidateSyntaxNodes.swift#L83-L85

You should be able to do type.description == other.type.description instead of name == other.name here.

ahoppen · 2023-08-07T16:15:25Z

...on/Sources/generate-swiftsyntax/templates/swiftsyntax/RenamedChildrenCompatibilityFile.swift

-          let childType: TypeSyntax = child.kind.isNodeChoicesEmpty ? child.syntaxNodeKind.syntaxType : "\(raw: child.name)"
-          let type = child.isOptional ? TypeSyntax("\(raw: childType)?") : TypeSyntax("\(raw: childType)")
+          let childType: TypeSyntax = child.kind.isNodeChoicesEmpty ? child.syntaxNodeKind.syntaxType : "\(child.type)"
+          let type = child.isOptional ? TypeSyntax("\(childType)?") : TypeSyntax("\(childType)")


Can be shortened a little bit more to

Suggested change

let type = child.isOptional ? TypeSyntax("\(childType)?") : TypeSyntax("\(childType)")

let type = child.isOptional ? TypeSyntax("\(childType)?") : childType

Nice catch! Thanks. Done.

ahoppen · 2023-08-07T16:22:22Z

CodeGeneration/Sources/SyntaxSupport/Node.swift

@@ -118,14 +118,14 @@ public class Node {
          let unexpectedDeprecatedName: String?

          if i == 0 {
-            unexpectedName = "UnexpectedBefore\(child.name)"
-            unexpectedDeprecatedName = child.deprecatedName.map { "UnexpectedBefore\($0)" }
+            unexpectedName = "UnexpectedBefore\(child.type)"


I would prefer to do child.name.withFirstLetterUppercased when building the unexpected name. I know that the result is the same as if we used the type name, but that’s more of a coincidence right now. What we’re intending to do here is to stitch the names together and the code should reflect that in my opinion. Otherwise every time I read this, I would wonder: What has the type name to do with the property name here.

I fully agree with you. I guess I was trying too hard to change the code base in order to make child.name private.

ahoppen · 2023-08-07T16:23:44Z

CodeGeneration/Sources/SyntaxSupport/Child.swift

+  /// If the child has been renamed, its old, now deprecated, name.
+  private let deprecatedName: String?
+


Any reason why you changed the order of deprecatedName and name?

Oh yeah :/ - habit from all my projects where we are trying to separate public properties from private ones. Should I revert the changes?

I'm gonna use deprecatedName not deprecatedType to create unexpected nodes names in the same way as you proposed using name instead of type here: #2011 (comment). So I'm gonna revers changes with making deprecatedName private and reordering

ahoppen · 2023-08-07T16:24:47Z

CodeGeneration/Sources/SyntaxSupport/Child.swift

@@ -58,16 +58,14 @@ public enum ChildKind {
 /// A child of a node, that may be declared optional or a token with a
 /// restricted subset of acceptable kinds or texts.
 public class Child {
+  /// If the child has been renamed, its old, now deprecated, name.
+  private let deprecatedName: String?
+
  /// The name of the child.
  ///
  /// The first character of the name is always uppercase.
  public let name: String


You should be able to do type.description == other.type.description instead of name == other.name here.

ahoppen · 2023-08-07T20:31:18Z

CodeGeneration/Sources/SyntaxSupport/Node.swift

-            unexpectedName = "UnexpectedBefore\(child.name)"
-            unexpectedDeprecatedName = child.deprecatedName.map { "UnexpectedBefore\($0)" }
+            unexpectedName = "UnexpectedBefore\(childName)"
+            unexpectedDeprecatedName = child.deprecatedType.map { "UnexpectedBefore\($0)" }


Just like we do child.name.withFirstCharacterUppercased, we should also do child.deprecatedName.withFirstCharacterUppercased.

And I think below are couple usages of deprecatedName that need to be uppercased once we lowercase deprecatedName.

At least one Child has a deprecated name that begins with a lowercase letter, which is inconsistent with the majority that starts with uppercase. This has led to the generation of an unexpected node name like UnexpectedBeforedeprecatedName. If we want to avoid changing the API, it seems we must adhere to this implementation.

And I think below are couple usages of deprecatedName that need to be uppercased once we lowercase deprecatedName.

Actually, I've not considered changing deprecatedName before. Do you think that we should do that?
I guess it's necessary if we are changing name. But what would be the best course of action regarding the aforementioned edge case?

At least one Child has a deprecated name that begins with a lowercase

Oh, that’s a bug. The deprecated name of FunctionType.paramters should be uppercased. Fixing in #2023.

Actually, I've not considered changing deprecatedName before.

Yes, absolutely, we should also lowercase deprecatedName.

ahoppen · 2023-08-07T20:47:55Z

CodeGeneration/Sources/SyntaxSupport/Child.swift

+  /// A type of this child.
+  public var type: TypeSyntax {
+    return "\(raw: name.withFirstCharacterUppercased)"
+  }


Sorry, after reading this PR again, I noticed that this isn’t actually the type of the child, unless it has multiple node choices. What this technically is, is the name that a nested SyntaxChoices type gets, if it gets created. With this in mind, a better name would be syntaxChoicesType and it should fail if the child doesn’t have kind == .nodeChoices to prevent misuse.

Suggested change

/// A type of this child.

public var type: TypeSyntax {

return "\(raw: name.withFirstCharacterUppercased)"

}

/// If this child has node choices, the type that the nested `SyntaxChildChoices` type should get.

///

/// For any other kind of child nodes, accessing this property crashes.

public var syntaxChoicesType: TypeSyntax {

precondition(kind.isNodeChoices, "Cannot get syntaxChoicesType for node that doesn’t have nodeChoices")

return "\(raw: name.withFirstCharacterUppercased)"

}

And I think most of the uses of type in ValidateSyntaxNodes should be varOrCaseName or, where necessary varOrCaseName.description.

It seems very reasonable. I just wondering what about places like this one: https://github.com/apple/swift-syntax/blob/83348e7eb826b978896421b2a233a4290596eb58/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftparser/ParserTokenSpecSetFile.swift#L23C4-L29 ? Do we wanna use name.withFirstCharacterUppercased instead of syntaxChoicesType?

I would introduce a new member on Child. We should really stop sticking type names together in CodeGeneration like this

/// If this child only has tokens, the type that the generated `TokenSpecSet` should get. /// /// For any other kind of child nodes, accessing this property crashes. public var syntaxChoicesType: TypeSyntax { precondition(kind.isToken /* <- you need to write this property, I think */, "Cannot get syntaxChoicesType for node that isn’t a token") return "\(raw: name.withFirstCharacterUppercased)Options" }

@ahoppen I'm sorry, but I got lost. Could you please summarize once more what we want to achieve?

Did you mean introducing two new properties on Child: one for kind.isNodeChoices and the other for kind.isToken?

Yes, exactly.

ahoppen

Wow, this is a lot nicer. I have one naming suggestion (and I was probably the one who suggested the old name that I now dislike, so sorry for that), otherwise this looks great.

ahoppen · 2023-08-09T21:29:35Z

CodeGeneration/Sources/SyntaxSupport/Child.swift

+  /// If this child only has tokens, the type that the generated `TokenSpecSet` should get.
+  ///
+  /// For any other kind of child nodes, accessing this property crashes.
+  public var syntaxTokenType: TypeSyntax {


I’m not sure if I suggested syntaxTokenType but in light of syntaxChoicesType, I think tokenSpecSetType would be a better name.

Also remember to update the precondition below.

Thanks. Done

ahoppen

Thanks for doing this @Matejkob!

ahoppen · 2023-08-11T00:37:45Z

@swift-ci Please test

ahoppen · 2023-08-11T15:24:48Z

@Matejkob Looks like there are some casing issues in the CodeGeneration package. You should be able to reproduce them locally by opening the CodeGeneration package and running the tests in there

Matejkob · 2023-08-11T15:59:38Z

@Matejkob Looks like there are some casing issues in the CodeGeneration package. You should be able to reproduce them locally by opening the CodeGeneration package and running the tests in there

Oh, something went wrong during the merge. Now everything should work fine.

ahoppen · 2023-08-11T16:33:20Z

@swift-ci Please test

ahoppen · 2023-08-11T20:37:05Z

@swift-ci Please test

Matejkob requested a review from ahoppen as a code owner August 6, 2023 09:22

Matejkob force-pushed the introduce-typeName-on-Child branch from ac4c0b0 to 529ccd1 Compare August 6, 2023 09:24

Matejkob commented Aug 6, 2023

View reviewed changes

ahoppen reviewed Aug 7, 2023

View reviewed changes

Matejkob force-pushed the introduce-typeName-on-Child branch from 529ccd1 to deda831 Compare August 7, 2023 17:45

Matejkob mentioned this pull request Aug 7, 2023

Update code-gen tool for lowercase child name consistency #1908

Closed

ahoppen reviewed Aug 7, 2023

View reviewed changes

Matejkob force-pushed the introduce-typeName-on-Child branch from deda831 to aeb8180 Compare August 9, 2023 17:29

Matejkob changed the title ~~Introduce type naming for Child nodes in CodeGeneration~~ Introduce syntaxTokenType and syntaxChoicesType for child nodes in CodeGeneration Aug 9, 2023

ahoppen reviewed Aug 9, 2023

View reviewed changes

Matejkob force-pushed the introduce-typeName-on-Child branch 2 times, most recently from 5850462 to d8d9a43 Compare August 10, 2023 06:10

Matejkob changed the title ~~Introduce syntaxTokenType and syntaxChoicesType for child nodes in CodeGeneration~~ Introduce tokenSpecSetType and syntaxChoicesType for child nodes in CodeGeneration Aug 10, 2023

ahoppen approved these changes Aug 11, 2023

View reviewed changes

ahoppen enabled auto-merge August 11, 2023 00:37

Introduce type naming for Child nodes in CodeGeneration

0381d8f

auto-merge was automatically disabled August 11, 2023 15:49
Head branch was pushed to by a user without write access

Matejkob force-pushed the introduce-typeName-on-Child branch from d8d9a43 to 0381d8f Compare August 11, 2023 15:49

ahoppen enabled auto-merge August 11, 2023 16:33

ahoppen merged commit a5ecb62 into swiftlang:main Aug 11, 2023

	let type = child.isOptional ? TypeSyntax("\(childType)?") : TypeSyntax("\(childType)")
	let type = child.isOptional ? TypeSyntax("\(childType)?") : childType

		/// If the child has been renamed, its old, now deprecated, name.
		private let deprecatedName: String?

Introduce tokenSpecSetType and syntaxChoicesType for child nodes in CodeGeneration #2011

Introduce tokenSpecSetType and syntaxChoicesType for child nodes in CodeGeneration #2011

Uh oh!

Conversation

Matejkob commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Aug 11, 2023

Uh oh!

ahoppen commented Aug 11, 2023

Uh oh!

Matejkob commented Aug 11, 2023

Uh oh!

ahoppen commented Aug 11, 2023

Uh oh!

ahoppen commented Aug 11, 2023

Uh oh!

Uh oh!

Introduce `tokenSpecSetType` and `syntaxChoicesType` for child nodes in CodeGeneration #2011

Introduce `tokenSpecSetType` and `syntaxChoicesType` for child nodes in CodeGeneration #2011

Matejkob commented Aug 6, 2023 •

edited

Loading