Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Further naming improvements for syntax node children #1896

Merged
merged 11 commits into from
Jul 23, 2023
Merged

Further naming improvements for syntax node children #1896

merged 11 commits into from
Jul 23, 2023

Conversation

ahoppen
Copy link
Contributor

@ahoppen ahoppen commented Jul 11, 2023

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 ahoppen requested a review from bnbarham July 11, 2023 13:17
@ahoppen ahoppen requested a review from DougGregor as a code owner July 11, 2023 13:17
@ahoppen ahoppen mentioned this pull request Jul 11, 2023
Merged
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 11, 2023

swiftlang/swift-format#562

@swift-ci Please test

ahoppen
ahoppen commented Jul 11, 2023
View reviewed changes
CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift
Comment on lines -1344 to +1347
name: "PatternExpr",
name: "Pack",
deprecatedName: "PatternExpr",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
ahoppen commented Jul 11, 2023
View reviewed changes
CodeGeneration/Sources/SyntaxSupport/TypeNodes.swift
Comment on lines -398 to +403
name: "PatternType",
name: "Pack",
deprecatedName: "PatternType",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ahoppen force-pushed the ahoppen/naming-improvements branch 2 times, most recently from 7ca9689 to c011db2 Compare July 11, 2023 22:09
@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 11, 2023

swiftlang/swift-format#562

@swift-ci Please test

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 11, 2023

swiftlang/swift-format#562

@swift-ci Please test Windows

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

ahoppen commented Jul 12, 2023

swiftlang/swift-format#562

@swift-ci Please test

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 12, 2023

swiftlang/swift-format#562

@swift-ci Please test Windows

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

ahoppen commented Jul 17, 2023

swiftlang/swift-format#562

@swift-ci Please test

bnbarham
bnbarham approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CodeGeneration/Sources/SyntaxSupport/DeclNodes.swift
@@ -1460,7 +1471,8 @@ public let DECL_NODES: [Node] = [
documentation: "The `#` sign."
),
Child(
name: "Macro",
name: "MacroName",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift
name: "Macro",
name: "MacroName",
deprecatedName: "Macro",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the decl case, ie. MacroName vs Name

CodeGeneration/Sources/SyntaxSupport/ExprNodes.swift Outdated
@@ -1398,7 +1411,8 @@ public let EXPR_NODES: [Node] = [
isOptional: true
),
Child(
name: "PostfixExpression",
name: "BaseExpression",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping Expression here on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, Base is better.

CodeGeneration/Tests/ValidateSyntaxNodes/ValidateSyntaxNodes.swift
)
}

/// Identifier is a wonderful ambiguous term. Almost always, 'name' or something similar is more expressive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CodeGeneration/Tests/ValidateSyntaxNodes/ValidateSyntaxNodes.swift Outdated
)
}

func testChildrenDontEndWithExprEtc() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

ahoppen commented Jul 22, 2023

@swift-ci Please test

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 22, 2023

swiftlang/swift-format#562

@swift-ci Please test

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 22, 2023

swiftlang/swift-format#562

@swift-ci Please test Windows

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

ahoppen commented Jul 22, 2023

swiftlang/swift-format#562

@swift-ci Please test

@ahoppen
Copy link
Contributor Author

ahoppen commented Jul 22, 2023

swiftlang/swift-format#562

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 1f6beda into swiftlang:main Jul 23, 2023
3 checks passed
@ahoppen ahoppen deleted the ahoppen/naming-improvements branch July 23, 2023 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham approved these changes

@kimdv kimdv kimdv approved these changes

@DougGregor DougGregor Awaiting requested review from DougGregor DougGregor is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ahoppen @bnbarham @kimdv