[ASTGen] Move logic in `BridgedASTContext.getIdentifier()` to ASTGen #70220

rintaro · 2023-12-05T00:49:08Z

getIdentifier() has some logic includning _ -> Identifier() and stripping escaped identifier.
Ideally, ASTBridging should only do the necessary work for bridging. All non-trivial logic should be in ASTGen.

Also, move-back getBridgedIdentifier(), getBridgedSourceLoc(), etc to ASTGenVisitor. Now that getIdentifier() has some logic, so it's more natural in ASTGenVisitor. As for getBridgedSourceLoc() this provides syntactic consistency with generate(xxx:) functions, e.g.

    return.createParsed(
      self.ctx,
      leftParenLoc: self.generateSourceLoc(node.leftParen),
      parameters: self.generate(functionParameterList: node.parameters),
      rightParenLoc: self.generateSourceLoc(node.rightParen)
    )

rintaro · 2023-12-05T00:50:05Z

swiftlang/swift-syntax#2383
@swift-ci Please smoke test

rintaro · 2023-12-05T05:13:14Z

@swift-ci Please smoke test

hamishknight · 2023-12-05T15:11:52Z

lib/ASTGen/Sources/ASTGen/Bridge.swift

+    if rawText == "_" {
+      return nil
+    }


Can this now check for tokenKind == .wildcard instead?

tokenKind instantiate String. Let's expose TokenSyntax.tokenView.rawKind as TokenSyntax.rawTokenKind as a SPI, in followups.

hamishknight · 2023-12-05T15:24:05Z

lib/ASTGen/Sources/ASTGen/ASTGen.swift

+  func bridgedIdentifier(token: TokenSyntax?) -> BridgedIdentifier {
+    token.map(bridgedIdentifier(token:)) ?? nil
+  }
+
+  /// Obtains the start location of the node excluding leading trivia in the
+  /// source buffer.
+  func bridgedSourceLoc(syntax node: some SyntaxProtocol) -> BridgedSourceLoc {
+    BridgedSourceLoc(at: node.positionAfterSkippingLeadingTrivia, in: self.base)
+  }


IMO the token: and syntax: argument labels aren't adding much useful information, could they be replaced by for:?

Also the old variants of all these methods were marked @inline(__always), does that need to be preserved?

hamishknight · 2023-12-05T15:28:41Z

lib/ASTGen/Sources/ASTGen/Decls.swift

      path: node.path.lazy.map {
-        $0.name.bridgedIdentifierAndSourceLoc(in: self) as BridgedIdentifierAndSourceLoc
+        self.bridgedIdentifierAndSourceLoc(token: $0.name) as BridgedIdentifierAndSourceLoc


Didn't realize this was already the case, but the return type overloading here makes me sad, maybe we should rename BridgedIdentifierAndSourceLoc to BridgedLocatedIdentifier or something and rename this overload of the method too

hamishknight · 2023-12-05T15:30:12Z

lib/ASTGen/Sources/ASTGen/Bridge.swift

+    if rawText.count > 2 && rawText.hasPrefix("`") && rawText.hasSuffix("`") {
+      text = .init(rebasing: text.dropFirst().dropLast())


I guess SwiftSyntax doesn't have a better way of doing this?

Like .trim("`")? No.
This is basically just slicing RandomAccessCollection, and I think this is the simplest.

I just meant a utility for getting the actual identifier for an escaped identifier (e.g we have getText vs getRawText on Token in the old parser)

rintaro · 2023-12-05T19:55:09Z

@swift-ci Please smoke test

rintaro

As per offline discussion, renamed:
bridgedIdentifier(token:) -> generateIdentifier(_:)
bridgedSourceLoc(syntax:) -> generateSourceLoc(_:)
etc.

rintaro · 2023-12-05T21:06:41Z

@swift-ci Please smoke test

Ideally ASTBriding should only do the neccessary work for briding. All non-trivial logic should be in ASTGen.

Now that `getIdentifier()` has some logic in it. It's not a simple bridging.

To avoid conflicts while migration. When all usage are migrated, this commit should be reverted.

rintaro · 2023-12-05T22:33:09Z

@swift-ci Please smoke test

AnthonyLatsis · 2023-12-06T03:49:38Z

lib/ASTGen/Sources/ASTGen/Bridge.swift

+  @available(*, deprecated, message: "use ASTContext.bridgedIdentifier(token:)")
  @inline(__always)
  func bridgedIdentifier(in astgen: ASTGenVisitor) -> BridgedIdentifier {


I think you meant "use ASTGenVisitor.generateIdentifier(_:)". Ditto other deprecations. Do we even need them though? It looks like you already audited all the calls.

rintaro requested review from zoecarver, CodaFi, hborla, slavapestov and xedin as code owners December 5, 2023 00:49

rintaro removed request for xedin, slavapestov, CodaFi, hborla and zoecarver December 5, 2023 00:50

rintaro marked this pull request as draft December 5, 2023 00:51

rintaro force-pushed the astgen-getidentifier branch from 10c067f to 1f46f64 Compare December 5, 2023 04:33

rintaro marked this pull request as ready for review December 5, 2023 14:40

rintaro requested review from bnbarham, ahoppen and hamishknight December 5, 2023 14:40

hamishknight approved these changes Dec 5, 2023

View reviewed changes

rintaro force-pushed the astgen-getidentifier branch from 1f46f64 to 6adbd18 Compare December 5, 2023 19:54

rintaro commented Dec 5, 2023

View reviewed changes

rintaro force-pushed the astgen-getidentifier branch from 6adbd18 to d695726 Compare December 5, 2023 21:06

rintaro added 3 commits December 5, 2023 14:26

[ASTGen] Move getIdentifier logic to ASTGen side

71ecdfc

Ideally ASTBriding should only do the neccessary work for briding. All non-trivial logic should be in ASTGen.

[ASTGen] Move getIdentifier() etc to ASTGenVisitor

7ab4003

Now that `getIdentifier()` has some logic in it. It's not a simple bridging.

[ASTGen] Add back TokenSyntax.bridgedIdentifier(in:) etc.

2f46e6f

To avoid conflicts while migration. When all usage are migrated, this commit should be reverted.

rintaro force-pushed the astgen-getidentifier branch from d695726 to 2f46e6f Compare December 5, 2023 22:33

rintaro enabled auto-merge December 5, 2023 22:33

rintaro merged commit 7b4e58a into swiftlang:main Dec 6, 2023

AnthonyLatsis reviewed Dec 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ASTGen] Move logic in `BridgedASTContext.getIdentifier()` to ASTGen #70220

[ASTGen] Move logic in `BridgedASTContext.getIdentifier()` to ASTGen #70220

Uh oh!

rintaro commented Dec 5, 2023 •

edited

Loading

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

hamishknight Dec 5, 2023

Uh oh!

rintaro Dec 5, 2023

Uh oh!

hamishknight Dec 5, 2023

Uh oh!

hamishknight Dec 5, 2023

Uh oh!

hamishknight Dec 5, 2023

Uh oh!

hamishknight Dec 5, 2023

Uh oh!

rintaro Dec 5, 2023

Uh oh!

hamishknight Dec 6, 2023

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

rintaro left a comment

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

AnthonyLatsis Dec 6, 2023

Uh oh!

Uh oh!

		if rawText.count > 2 && rawText.hasPrefix("`") && rawText.hasSuffix("`") {
		text = .init(rebasing: text.dropFirst().dropLast())

[ASTGen] Move logic in BridgedASTContext.getIdentifier() to ASTGen #70220

[ASTGen] Move logic in BridgedASTContext.getIdentifier() to ASTGen #70220

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

rintaro commented Dec 5, 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!

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 Dec 5, 2023

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

rintaro commented Dec 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[ASTGen] Move logic in `BridgedASTContext.getIdentifier()` to ASTGen #70220

[ASTGen] Move logic in `BridgedASTContext.getIdentifier()` to ASTGen #70220

rintaro commented Dec 5, 2023 •

edited

Loading