Further naming improvements for syntax node children #1896

ahoppen · 2023-07-11T13:17:12Z

Add a few more syntax child name validation rules and update child names

No child should end with Token
All children that refer to a collection should have a plural name
Children shouldn’t be named Identifier -> Name is usually the better name
In fact, no child should even contain the substring Identifier -> often it doesn’t provide any additional value
Don’t use abbreviations in child names like Args
Children shouldn’t end with Expr, Expression, Type etc. -> usually we can just omit these words
And a couple more hand-picked naming improvements

ahoppen · 2023-07-11T13:18:05Z

ahoppen · 2023-07-11T13:28:13Z

CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift

-        name: "PatternExpr",
+        name: "Pack",
+        deprecatedName: "PatternExpr",


@hborla I renamed the expression in PackExpansionExprSyntax from patternExpr to pack. Does this make sense to you. PatternExpr didn’t make sense to me because the expression after repeat isn’t a pattern in the Swift pattern matching sense.

ahoppen · 2023-07-11T13:29:06Z

CodeGeneration/Sources/SyntaxSupport/TypeNodes.swift

-        name: "PatternType",
+        name: "Pack",
+        deprecatedName: "PatternType",


@hborla I renamed the underlying type in PackExpansionTypeSyntax from patternType to pack. Does this make sense to you. PatternType didn’t make sense to me because the expression after repeat isn’t a pattern in the Swift pattern matching sense and it’s not really a pattern here.

ahoppen · 2023-07-11T22:10:07Z

swiftlang/swift-format#562

@swift-ci Please test

ahoppen · 2023-07-11T22:10:58Z

swiftlang/swift-format#562

@swift-ci Please test Windows

ahoppen · 2023-07-12T06:05:42Z

swiftlang/swift-format#562

@swift-ci Please test

ahoppen · 2023-07-12T06:07:17Z

swiftlang/swift-format#562

@swift-ci Please test Windows

ahoppen · 2023-07-17T16:12:46Z

swiftlang/swift-format#562

@swift-ci Please test

bnbarham

Nice improvements in general IMO. I do much prefer "s" over "list". "returnClause.returnType" is amusing too, nice catch.

bnbarham · 2023-07-21T21:39:40Z

CodeGeneration/Sources/SyntaxSupport/DeclNodes.swift

@@ -1460,7 +1471,8 @@ public let DECL_NODES: [Node] = [
        documentation: "The `#` sign."
      ),
      Child(
-        name: "Macro",
+        name: "MacroName",


We use Name for most other nodes, any reason to prefer MacroName here?

We use Name for nodes that declare something (e.g. StructDeclSyntax), while this node refers to something. And I just thought that name alone here is a little ambiguous because it’s not entirely sure what the name is. I don’t have super strong opinions here though.

bnbarham · 2023-07-21T21:41:54Z

CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift

-        name: "Macro",
+        name: "MacroName",
+        deprecatedName: "Macro",


Same as the decl case, ie. MacroName vs Name

bnbarham · 2023-07-21T21:42:40Z

CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift

@@ -1398,7 +1411,8 @@ public let EXPR_NODES: [Node] = [
        isOptional: true
      ),
      Child(
-        name: "PostfixExpression",
+        name: "BaseExpression",


Keeping Expression here on purpose?

Good catch, Base is better.

bnbarham · 2023-07-21T21:46:39Z

CodeGeneration/Tests/ValidateSyntaxNodes/ValidateSyntaxNodes.swift

+    )
+  }
+
+  /// Identifier is a wonderful ambiguous term. Almost always, 'name' or something similar is more expressive


FWIW I don't see that much difference between name and identifier. Other than name being a little simpler. Sure, in a general sense "identifier" is probably more ambiguous than "name". But in the parser sense not so much. Also all our names are IdentifierToken 🤷.

For me Identifier in the SwiftSyntax context is an identifier token, ie. technically just a word. And that identifier token can server multiple purposes, refer to a value, name a type, be a function parameter label, …

And wherever we have a clear usage of that identifier token (eg. when it is the name of a nominal type), I think we should name the child by the usage.

bnbarham · 2023-07-21T21:47:33Z

CodeGeneration/Tests/ValidateSyntaxNodes/ValidateSyntaxNodes.swift

+    )
+  }
+
+  func testChildrenDontEndWithExprEtc() {


testChildrenDontEndWithNodeKind? Doesn't really matter though (and "Syntax" isn't a kind) 😅

Yes, Syntax isn‘t a kind, children still shouldn’t end with Syntax 😉

…al name

ahoppen · 2023-07-22T04:27:41Z

@swift-ci Please test

ahoppen · 2023-07-22T04:39:25Z

swiftlang/swift-format#562

@swift-ci Please test

ahoppen · 2023-07-22T04:41:50Z

swiftlang/swift-format#562

@swift-ci Please test Windows

ahoppen · 2023-07-22T13:50:30Z

swiftlang/swift-format#562

@swift-ci Please test

ahoppen · 2023-07-22T23:21:23Z

swiftlang/swift-format#562

@swift-ci Please test Windows

ahoppen requested a review from bnbarham July 11, 2023 13:17

ahoppen requested a review from DougGregor as a code owner July 11, 2023 13:17

ahoppen mentioned this pull request Jul 11, 2023

Update swift-format for renamed children in SwiftSyntax swiftlang/swift-format#562

Merged

ahoppen commented Jul 11, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/naming-improvements branch 2 times, most recently from 7ca9689 to c011db2 Compare July 11, 2023 22:09

ahoppen force-pushed the ahoppen/naming-improvements branch from c011db2 to 7f32d45 Compare July 12, 2023 06:05

kimdv approved these changes Jul 12, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/naming-improvements branch from 7f32d45 to a04a35b Compare July 17, 2023 16:01

bnbarham approved these changes Jul 21, 2023

View reviewed changes

ahoppen added 10 commits July 21, 2023 21:24

Add validation that children should not end with 'Token'

850fcc8

Validate that all children containing a syntax collection have a plur…

91639f1

…al name

Verify that children are not named 'Identifier'

5c2923f

Validate that no child contains the substring 'Identifier'

431c77b

Validate that we don’t use abbreviations in child names

1aa235d

Validate that children don’t end with 'Expr', 'Expression', 'Type' etc

e85288d

Improve naming of some more syntax children

8e53ebe

Update swift-syntax for renamed API

89d38e4

Slightly improve syntax compatibility layer

1062c22

Update CodeGeneration for renamed properties

fcee3e1

ahoppen force-pushed the ahoppen/naming-improvements branch from a04a35b to fcee3e1 Compare July 22, 2023 04:26

Address review comments

b9a3830

ahoppen force-pushed the ahoppen/naming-improvements branch from a714bed to b9a3830 Compare July 22, 2023 13:50

ahoppen merged commit 1f6beda into swiftlang:main Jul 23, 2023

ahoppen deleted the ahoppen/naming-improvements branch July 23, 2023 03:05

Further naming improvements for syntax node children #1896

Further naming improvements for syntax node children #1896

Uh oh!

Conversation

ahoppen commented Jul 11, 2023

Uh oh!

ahoppen commented Jul 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 11, 2023

Uh oh!

ahoppen commented Jul 11, 2023

Uh oh!

ahoppen commented Jul 12, 2023

Uh oh!

ahoppen commented Jul 12, 2023

Uh oh!

ahoppen commented Jul 17, 2023

Uh oh!

bnbarham 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!

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 commented Jul 22, 2023

Uh oh!

ahoppen commented Jul 22, 2023

Uh oh!

ahoppen commented Jul 22, 2023

Uh oh!

ahoppen commented Jul 22, 2023

Uh oh!

ahoppen commented Jul 22, 2023

Uh oh!

Uh oh!