Replace expression in `ClosureCaptureSyntax` with initializer clause by MAJKFL · Pull Request #2763 · swiftlang/swift-syntax

MAJKFL · 2024-07-29T19:44:06Z

This PR adjusts ClosureCaptureSyntax node.

name is no longer optional, making it more convenient to refer it by name (as discussed under [SwiftLexicalLookup][GSoC] Add initial name lookup functionality #2719).
equal and expression were replaced by the optional initializer property of the InitializerClauseSyntax type, making it more uniform with variable declarations like let a = b and function parameters like a: Int = 123.
Due to the new node structure, removed the convenience initializer introduced with Add a convenience init to ClosureCaptureSyntax #2127 as name is not optional and equal sign is no longer part of the node itself.

ahoppen

Instead of removing any public API, could you add the existing API signatures as deprecated methods to SwiftSyntaxCompatibility.swift with the most reasonable implementation possible?

Also, could you add the changes to the 601 release notes?

ahoppen · 2024-07-29T22:28:13Z

+          let name: RawTokenSyntax
+          let initializer: RawInitializerClauseSyntax?


You should be able to hoist the parsing of name out of the if statement and then use self.at(.equal) instead of self.peek(isAt: .equal)

Thanks for the suggestion. I moved name parsing to before the if statement.

As an interesting artifact, self token will not be remapped to identifier when parsing capture of [self = 123] form. Out of curiosity, was there a particular reason for this remapping? As far as I checked, e.g. in let self = 123 self is classified as a keyword.

…to release notes.

ahoppen · 2024-07-30T21:08:50Z

-extension ClosureCaptureSyntax {
-  @available(*, deprecated, renamed: "unexpectedBetweenNameAndEqual")
-  public var unexpectedBetweenNameAndAssignToken: UnexpectedNodesSyntax? {
-    get {
-      return unexpectedBetweenNameAndEqual
-    }
-    set {
-      unexpectedBetweenNameAndEqual = newValue
-    }
-  }
-
-  @available(*, deprecated, renamed: "equal")
-  public var assignToken: TokenSyntax? {
-    get {
-      return equal
-    }
-    set {
-      equal = newValue
-    }
-  }
-
-  @available(*, deprecated, renamed: "unexpectedBetweenEqualAndExpression")
-  public var unexpectedBetweenAssignTokenAndExpression: UnexpectedNodesSyntax? {
-    get {
-      return unexpectedBetweenEqualAndExpression
-    }
-    set {
-      unexpectedBetweenEqualAndExpression = newValue
-    }
-  }
-
-  @available(*, deprecated, renamed: "ClosureCaptureSyntax(leadingTrivia:_:specifier:_:name:_:equal:_:expression:_:trailingComma:_:trailingTrivia:)")
-  @_disfavoredOverload
-  public init(
-      leadingTrivia: Trivia? = nil,
-      _ unexpectedBeforeSpecifier: UnexpectedNodesSyntax? = nil,
-      specifier: ClosureCaptureSpecifierSyntax? = nil,
-      _ unexpectedBetweenSpecifierAndName: UnexpectedNodesSyntax? = nil,
-      name: TokenSyntax? = nil,
-      _ unexpectedBetweenNameAndAssignToken: UnexpectedNodesSyntax? = nil,
-      assignToken: TokenSyntax? = nil,
-      _ unexpectedBetweenAssignTokenAndExpression: UnexpectedNodesSyntax? = nil,
-      expression: some ExprSyntaxProtocol,
-      _ unexpectedBetweenExpressionAndTrailingComma: UnexpectedNodesSyntax? = nil,
-      trailingComma: TokenSyntax? = nil,
-      _ unexpectedAfterTrailingComma: UnexpectedNodesSyntax? = nil,
-      trailingTrivia: Trivia? = nil
-
-  ) {
-    self.init(
-        leadingTrivia: leadingTrivia, 
-        unexpectedBeforeSpecifier, 
-        specifier: specifier, 
-        unexpectedBetweenSpecifierAndName, 
-        name: name, 
-        unexpectedBetweenNameAndAssignToken, 
-        equal: assignToken, 
-        unexpectedBetweenAssignTokenAndExpression, 
-        expression: expression, 
-        unexpectedBetweenExpressionAndTrailingComma, 
-        trailingComma: trailingComma, 
-        unexpectedAfterTrailingComma, 
-        trailingTrivia: trailingTrivia
-      )
-  }
-}
-


People might still be relying on these compatibility overloads, so we shouldn’t remove them.

I added those to SwiftSyntaxCompatibility.swift.

ahoppen · 2024-07-30T21:11:14Z

We should continue to offer deprecated compatibility versions of all the old signatures in this file. SwiftSyntaxCompatibility.swift should have a few examples.

I added those to SwiftSyntaxCompatibility.swift.

ahoppen · 2024-07-30T21:13:11Z

-  public var name: TokenSyntax? {
+  public var name: TokenSyntax {


Changing name to be non-optional is an API-breaking change because eg. closuereCapture.name?.text will stop compiling. There is no way of making it API-compatible. Could you highlight it in the release notes?

Thanks for pointing this out. I added a note under "API-Incompatible Changes" in the release notes.

ahoppen · 2024-07-30T21:14:08Z

+- `ClosureCaptureSyntax` now has a new node structure.
+  - Description: `ClosureCaptureSyntax` now has an `initializer` property instead of `equal` and `expression`. Additionally, the `name` property is no longer optional.
+  - Pull request: https://github.com/swiftlang/swift-syntax/pull/2763


I would move this to the Deprecations section because that’s the way that users are most likely to hit it: They were using it before and now they have a deprecation warning.

New APIs is intended for features that didn’t exist before.

Thanks for the suggestion. I moved it to the note under "Deprecations".

… release notes.

ahoppen

Thanks for adding the compatibility layers. Looks good to me 👍🏽

ahoppen · 2024-07-31T14:07:22Z

@swift-ci Please test

MAJKFL · 2024-08-01T09:12:13Z

The CI failed because we removed the self remapping to identifier earlier in this PR and I didn't add it as a possible kind in CodeGeneration. Should be fixed now.

ahoppen · 2024-08-01T16:05:01Z

There’s a merge conflict in the release notes now, could you resolve that @MAJKFL?

MAJKFL · 2024-08-01T17:51:29Z

Should be good to go now. Could you please test it @ahoppen?

ahoppen · 2024-08-01T22:07:30Z

@swift-ci Please test

MAJKFL · 2024-08-02T07:56:52Z

Had to fix failing testConsistentNamingOfChildren test. Seems like ClosureCaptureSyntax was picked up as mostCommonChild over MatchingPatternConditionSyntax because of alphabetical precedence.

DougGregor · 2024-08-02T15:00:49Z

@swift-ci please test

DougGregor · 2024-08-02T15:01:32Z

@swift-ci please test Windows

DougGregor · 2024-08-02T16:09:20Z

@swift-ci please test Windows

DougGregor · 2024-08-02T17:17:26Z

Ah, drat:

/home/build-user/swift-format/Tests/SwiftFormatTests/PrettyPrint/ClosureExprTests.swift:266: error: ClosureExprTests.testClosureCapture : failed - Pretty-printed result was not what was expected - Actual output (+) differed from expected output (-):
 let a = funcCall() { [weak self] (a: Int) in
   return a + 1
 }
 let a = funcCall() {
-  [weak self, weak a = self.b] (a: Int) in
+  [weak self, weak a  =  self.b] (a: Int) in
   return a + 1
 }
 let b = funcCall() {
-  [unowned self, weak delegate = self.delegate!]
+  [unowned self, weak delegate  =  self.delegate!]
   (a: Int, b: String) -> String in
   return String(a) + b
 }

MAJKFL · 2024-08-02T20:50:55Z

Hmm... this is an interesting one. Inside TokenStreamCreator, when visiting ClosureCaptureSyntax node, the method adds spaces around the = token which seem to be not necessary anymore. I was able to fix the failure locally just by removing those two lines of code (which used deprecated by this PR ClosureCaptureSyntax.equal property btw).

override func visit(_ node: ClosureCaptureSyntax) -> SyntaxVisitorContinueKind {
  before(node.firstToken(viewMode: .sourceAccurate), tokens: .open)
  after(node.specifier?.lastToken(viewMode: .sourceAccurate), tokens: .break)
  // before(node.equal, tokens: .break)
  // after(node.equal, tokens: .break)
  if let trailingComma = node.trailingComma {
    before(trailingComma, tokens: .close)
    after(trailingComma, tokens: .break(.same))
  } else {
    after(node.lastToken(viewMode: .sourceAccurate), tokens: .close)
  }
  return .visitChildren
}

Does it mean we need to fix it there? Or do you think there is a way to fix it in this PR and I'm missing something?

DougGregor · 2024-08-02T21:51:56Z

Hmm... this is an interesting one. Inside TokenStreamCreator, when visiting ClosureCaptureSyntax node, the method adds spaces around the = token which seem to be not necessary anymore. I was able to fix the failure locally just by removing those two lines of code (which used deprecated by this PR ClosureCaptureSyntax.equal property btw).
override func visit(_ node: ClosureCaptureSyntax) -> SyntaxVisitorContinueKind {
  before(node.firstToken(viewMode: .sourceAccurate), tokens: .open)
  after(node.specifier?.lastToken(viewMode: .sourceAccurate), tokens: .break)
  // before(node.equal, tokens: .break)
  // after(node.equal, tokens: .break)
  if let trailingComma = node.trailingComma {
    before(trailingComma, tokens: .close)
    after(trailingComma, tokens: .break(.same))
  } else {
    after(node.lastToken(viewMode: .sourceAccurate), tokens: .close)
  }
  return .visitChildren
}
Does it mean we need to fix it there? Or do you think there is a way to fix it in this PR and I'm missing something?

Yes, I think it's okay to fix it there.

Edit: Argh, I'm not really sure now. Could it be that the now-deprecated equal should produce the node without whitespace? That doesn't seem quite right, either. @ahoppen or @allevato , do either of you have an opinion?

ahoppen · 2024-08-06T15:55:26Z

Edit: Argh, I'm not really sure now. Could it be that the now-deprecated equal should produce the node without whitespace? That doesn't seem quite right, either. @ahoppen or @allevato , do either of you have an opinion?

The equal property should return the equal token with its trivia because the equal token has trivia attached to it. The compatibility layers are best-effort and this sort of issue might happen because we’re now effectively visiting the equal token twice. We should just fix swift-format like you suggested. @MAJKFL Could you open a PR with that change to swift-format and then we can merge it together with the swift-syntax PR.

allevato · 2024-08-06T16:03:51Z

I agree that we should just update swift-format to stop using the compatibility layer nodes. In cases like this where the fundamental structure of a node changes, it's important that we visit the tree in the way that reflects the actual structure.

ahoppen · 2024-08-06T21:49:39Z

swiftlang/swift-format#790

@swift-ci Please test

Adjust closure capture syntax node.

22347b8

MAJKFL requested review from ahoppen and bnbarham as code owners July 29, 2024 19:44

ahoppen reviewed Jul 29, 2024

View reviewed changes

Bring back the convenience initializer and deprecate it. Add entries …

d5bbac9

…to release notes.

MAJKFL requested review from ahoppen July 30, 2024 08:51

ahoppen reviewed Jul 30, 2024

View reviewed changes

MAJKFL added 2 commits July 31, 2024 11:58

Add deprecated signatures to SwiftSyntaxCompatibility.swift. Adjust…

abc705f

… release notes.

Format.

2984ad8

ahoppen reviewed Jul 31, 2024

View reviewed changes

ahoppen approved these changes Jul 31, 2024

View reviewed changes

Add self keyword as a possible name kind in ClosureCaptureSyntax.

612109f

Merge branch 'main' into closure-capture-syntax-node-update

98f40b1

ahoppen enabled auto-merge (squash) August 1, 2024 22:07

Fix failing inconsistent initializer naming test.

e3f6436

auto-merge was automatically disabled August 2, 2024 07:50
Head branch was pushed to by a user without write access

MAJKFL mentioned this pull request Aug 6, 2024

Remove closure capture equal sign spacing. swiftlang/swift-format#790

Merged

allevato mentioned this pull request Aug 6, 2024

[DO NOT MERGE] allevato's dummy PR for swift-format CI #1086

Open

ahoppen merged commit 8eaab36 into swiftlang:main Aug 7, 2024

rintaro mentioned this pull request Aug 9, 2024

Revert "Replace expression in ClosureCaptureSyntax with initializer clause" #2793

Closed

MAJKFL mentioned this pull request Aug 9, 2024

Remove deprecated ClosureCaptureSyntax initializer with assignToken #2795

Merged

MAJKFL deleted the closure-capture-syntax-node-update branch October 16, 2024 11:23

		let name: RawTokenSyntax
		let initializer: RawInitializerClauseSyntax?

		public var name: TokenSyntax? {
		public var name: TokenSyntax {

Uh oh!

Conversation

MAJKFL commented Jul 29, 2024

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!

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!

ahoppen commented Jul 31, 2024

Uh oh!

MAJKFL commented Aug 1, 2024

Uh oh!

ahoppen commented Aug 1, 2024

Uh oh!

MAJKFL commented Aug 1, 2024

Uh oh!

ahoppen commented Aug 1, 2024

Uh oh!

MAJKFL commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

MAJKFL commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Aug 6, 2024

Uh oh!

allevato commented Aug 6, 2024

Uh oh!

ahoppen commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DougGregor commented Aug 2, 2024 •

edited

Loading