Remove the with<childName> functions on syntax nodes #1253

ahoppen · 2023-01-20T16:21:05Z

Instead, provide a single with function that takes a key path of the child you want to replace. This significatnly reduces the amout of code that is being generated for SwiftSyntax.

While at it, also remove the without(Leading|Trailing)?Trivia functions.

@DougGregor I changed a few calls of token.withoutTrivia() in macros to token.text, which should perform the same thing while also being slightly more efficient.

ahoppen · 2023-01-20T16:21:15Z

@swift-ci Please test

ahoppen · 2023-01-20T16:46:25Z

swiftlang/swift-format#477
swiftlang/swift-stress-tester#224

@swift-ci Please test

bnbarham · 2023-01-20T22:42:48Z

Sources/SwiftParserDiagnostics/SyntaxExtensions.swift

@@ -71,7 +71,7 @@ extension SyntaxProtocol {
  /// diagnostic message), return that.
  /// Otherwise, return a generic message that describes the tokens in this node.
  var shortSingleLineContentDescription: String {
-    let contentWithoutTrivia = self.withoutLeadingTrivia().withoutTrailingTrivia().description
+    let contentWithoutTrivia = self.with(\.leadingTrivia, []).with(\.trailingTrivia, []).description


I wonder if it would be worth having a description without trivia, seems wasteful to create a new node (twice) just to remove trivia for printing.

Yeah, it seems wasteful but it seems like a fairly narrow use-case and I don’t want to blow up our API surface for such use cases.

Doesn't seem that narrow. Can leave for now, we can always add later.

Drive-by comment: it's useful for SwiftLint, we currently have 13 occurrences of withoutTrivia().description https://github.com/search?q=repo%3Arealm%2FSwiftLint%20withoutTrivia().description&type=code

Maybe there's a better way to accomplish this, but that's what we're doing now.

OK, you convinced me. I added trimmedDescription. I decided against naming it descriptionWithoutTrivia because that sounds like we would also remove trivia in the middle of the node.

bnbarham · 2023-01-20T23:06:51Z

Sources/SwiftSyntax/SyntaxNodes.swift.gyb.template

-      self = with${child.name}(value)
+      let arena = SyntaxArena()
+  %       if child.is_optional:
+      let raw = value?.raw
+  %       else:
+      let raw = value.raw
+  %       end
+      let newData = data.replacingChild(at: ${idx}, with: raw, arena: arena)
+      self = ${node.name}(newData)


Ah, I was hoping we could use SyntaxNodeStructure and the need for this entirely. We don't currently have the reverse mapping (ie. keypath -> index), but maybe that would be useful. That would be even more codegen though, not less 😅.

EDIT: We will need that mapping when we eventually make this variadic + want to set multiple in with(...) at once though.

How would you want to get rid of these using SyntaxNodeStructure?

You can't right, we need the reverse mapping (key path -> index).

bnbarham · 2023-01-20T23:38:25Z

CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntax/SyntaxTraitsFile.swift

+      @_disfavoredOverload
+      func with<T>(_ keyPath: WritableKeyPath<\(raw: trait.traitName)Syntax, T>, _ newChild: T) -> \(raw: trait.traitName)Syntax {
+        var copy: \(raw: trait.traitName)Syntax = self
+        copy[keyPath: keyPath] = newChild


layoutKeyPath or something instead? keyPath: keyPath just looks weird 😅

Well, it’s not just layouts right. keyPath can also point to trivia.

rintaro · 2023-01-20T23:57:07Z

I know we decided to do with() accepting keypaths. But maybe resurrecting Cursor (but with associated values) might be an option?
For example:

extension FunctionDeclSyntax {
  enum Cursor {
    case attributes(AttributeListSyntax)
    case modifiers(ModifierListSyntax?)
    ...
  }
  func with(_ values: Cursor...) -> Self {
    ...
    for cursor in values {
      switch cursor {
      case .attributes(let val): layout[0] = val.raw
      case .modifiers(let val): layout[1] = val?.raw
      ...
      }
    }
    ...
  }
}

So you can call foo = bar.with(.attribute(newAttr), .signature(rewritten))

rintaro · 2023-01-21T00:10:47Z

A concern accepting KeyPaths on Self is that it need to accept any properties including that is not meant to. For example:

newFn = fn.with(\.self, otherFn)

It works, but not sure we should accept this.

Also do we want to accept multi-level paths? fn.with(\.signature.input, newInput). This looks cool, so I think the answer is yes.

ahoppen · 2023-01-23T11:39:43Z

I know we decided to do with() accepting keypaths. But maybe resurrecting Cursor (but with associated values) might be an option?

That would just be trading some code generation for another. What I like about the key path approach is that it doesn’t require code generation.

A concern accepting KeyPaths on Self is that it need to accept any properties including that is not meant to. For example:
newFn = fn.with(\.self, otherFn)
It works, but not sure we should accept this.

Also do we want to accept multi-level paths? fn.with(\.signature.input, newInput). This looks cool, so I think the answer is yes.

I actually really liked how the with function generalized from layouts to trivia as well. And it’s really just a shorthand to write three lines in one, so I’m totally happy if it generalizes to multi-level key paths. And I don’t see any problem with fn.with(\.self, otherFn) working as well. Sure, it’s silly but it does exactly what you expect, right?

ahoppen · 2023-01-24T22:55:33Z

swiftlang/swift#63160
swiftlang/swift-format#477
swiftlang/swift-stress-tester#224

@swift-ci Please test macOS

ahoppen · 2023-01-25T08:15:21Z

swiftlang/swift#63160
swiftlang/swift-format#477
swiftlang/swift-stress-tester#224

@swift-ci Please test macOS

ahoppen · 2023-01-25T13:00:39Z

swiftlang/swift#63160
swiftlang/swift-format#477
swiftlang/swift-stress-tester#224

@swift-ci Please test Linux

Instead, provide a single `with` function that takes a key path of the child you want to replace. This significatnly reduces the amout of code that is being generated for SwiftSyntax. While at it, also remove the `without(Leading|Trailing)?Trivia` functions

ahoppen · 2023-01-26T07:32:23Z

swiftlang/swift#63160
swiftlang/swift-format#477
swiftlang/swift-stress-tester#224

@swift-ci Please test

ahoppen · 2023-01-26T09:22:36Z

swiftlang/swift#63160
swiftlang/swift-format#477
swiftlang/swift-stress-tester#224

@swift-ci Please test macOS

ahoppen requested a review from bnbarham January 20, 2023 16:21

ahoppen requested a review from DougGregor as a code owner January 20, 2023 16:21

ahoppen changed the title ~~Remove the with<childName> functions on syntax nodes~~ Remove the with<childName> functions on syntax nodes 🚥 #1252 Jan 20, 2023

This was referenced Jan 20, 2023

Adjustment for removed with<childName> functions swiftlang/swift-format#477

Merged

Adjustment for removed with<childName> functions swiftlang/swift-stress-tester#224

Merged

bnbarham approved these changes Jan 20, 2023

View reviewed changes

bnbarham reviewed Jan 20, 2023

View reviewed changes

ahoppen changed the title ~~Remove the with<childName> functions on syntax nodes 🚥 #1252~~ Remove the with<childName> functions on syntax nodes Jan 23, 2023

ahoppen mentioned this pull request Jan 23, 2023

Adjustment ASTGen for removed with<childName> functions swiftlang/swift#63160

Merged

ahoppen force-pushed the ahoppen/remove-with-functions branch 3 times, most recently from 0c105f1 to a63faeb Compare January 24, 2023 22:55

ahoppen mentioned this pull request Jan 25, 2023

Shorten getters and setters of syntax nodes #1270

Merged

ahoppen force-pushed the ahoppen/remove-with-functions branch from a63faeb to 1342762 Compare January 26, 2023 07:21

ahoppen force-pushed the ahoppen/remove-with-functions branch from 1342762 to 1940941 Compare January 26, 2023 07:32

kimdv mentioned this pull request Jan 26, 2023

Update swift-syntax #1273

Merged

ahoppen merged commit 00c6476 into swiftlang:main Jan 26, 2023

ahoppen deleted the ahoppen/remove-with-functions branch January 26, 2023 18:46

Remove the with<childName> functions on syntax nodes #1253

Remove the with<childName> functions on syntax nodes #1253

Uh oh!

Conversation

ahoppen commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Jan 20, 2023

Uh oh!

ahoppen commented Jan 20, 2023

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!

bnbarham Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rintaro commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Jan 21, 2023

Uh oh!

ahoppen commented Jan 23, 2023

Uh oh!

ahoppen commented Jan 24, 2023

Uh oh!

ahoppen commented Jan 25, 2023

Uh oh!

ahoppen commented Jan 25, 2023

Uh oh!

ahoppen commented Jan 26, 2023

Uh oh!

ahoppen commented Jan 26, 2023

Uh oh!

Uh oh!

ahoppen commented Jan 20, 2023 •

edited

Loading

bnbarham Jan 20, 2023 •

edited

Loading

rintaro commented Jan 20, 2023 •

edited

Loading