Skip to content

[SwiftParser] Improve ergonomics for initializers of RawXXXSyntax #2805

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.

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

Merged

Conversation

AppAppWorks
Copy link
Contributor

This PR aims to improve developer ergonomics when using the designated initializers of raw syntax nodes. Simply put, it's through achieving feature parity with the non-raw side.

Prefer generic types over type erasure types

Currently the designated initializer of RawXXXSyntax always uses type-erased types instead of generic types as parameter types, leading to much clunkiness, e.g.

public struct RawPatternExprSyntax: RawExprSyntaxNodeProtocol {
  public init(
    _ unexpectedBeforePattern: RawUnexpectedNodesSyntax? = nil, 
    pattern: RawPatternSyntax, // type erasure
    _ unexpectedAfterPattern: RawUnexpectedNodesSyntax? = nil, 
    arena: __shared SyntaxArena
  )
}

let pattern = RawIdentifierPatternSyntax(
  identifier: identifier,
  arena: self.arena
)
return RawExprSyntax(RawPatternExprSyntax(pattern: RawPatternSyntax(pattern), arena: self.arena))

The initializer body, however, only invokes pattern.raw, therefore we could just replace pattern: RawPatternSyntax with pattern: some RawPatternSyntaxNodeProtocol transparently,

public struct RawPatternExprSyntax: RawExprSyntaxNodeProtocol {
  public init(
    _ unexpectedBeforePattern: RawUnexpectedNodesSyntax? = nil, 
    pattern: some RawPatternSyntaxNodeProtocol, // generic type
    _ unexpectedAfterPattern: RawUnexpectedNodesSyntax? = nil, 
    arena: __shared SyntaxArena
  )
}

let pattern = RawIdentifierPatternSyntax(
  identifier: identifier,
  arena: self.arena
)
// clunky type-erasure removed
return RawExprSyntax(RawPatternExprSyntax(pattern: pattern, arena: self.arena))

Actually it's what PatternExprSyntax.init has been doing,

public init(
  leadingTrivia: Trivia? = nil,
  _ unexpectedBeforePattern: UnexpectedNodesSyntax? = nil,
  pattern: some PatternSyntaxProtocol,
  _ unexpectedAfterPattern: UnexpectedNodesSyntax? = nil,
  trailingTrivia: Trivia? = nil
)

Generate convenience generic initializers for choices enum

e.g.

public struct RawCodeBlockItemSyntax: RawSyntaxNodeProtocol {
  public enum Item: RawSyntaxNodeProtocol {
    case `decl`(RawDeclSyntax)
    case `stmt`(RawStmtSyntax)
    case `expr`(RawExprSyntax)

    public init(_ other: some RawDeclSyntaxNodeProtocol) {
      self = .decl(RawDeclSyntax(other))
    }
    
    public init(_ other: some RawStmtSyntaxNodeProtocol) {
      self = .stmt(RawStmtSyntax(other))
    }
    
    public init(_ other: some RawExprSyntaxNodeProtocol) {
      self = .expr(RawExprSyntax(other))
    }

So call sites might get rid of type erasure as follows,

// requires type erasure
.expr(RawExprSyntax(RawMissingExprSyntax(arena: self.arena)))

// supports generic type 
.init(RawMissingExprSyntax(arena: self.arena))

Remove type erasure in call sites

This PR has removed unnecessary uses of type-erased types in many call sites thanks to the new initializers.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you. That does simplify the construction of raw syntax nodes.

I am not personally a huge fan of using .init to create objects. It’s easy to write but I think it hurts readability along the way because you don’t know which object is being constructed. Could you change those to the actual type names or stick with the enum case name if we don’t actually need to go through the initializer?

@AppAppWorks AppAppWorks force-pushed the improve-ergonomics-raw-syntax-initializers branch from 39015d3 to 9e630fd Compare August 15, 2024 19:56
@AppAppWorks
Copy link
Contributor Author

AppAppWorks commented Aug 15, 2024

Could you change those to the actual type names or stick with the enum case name if we don’t actually need to go through the initializer?

I've reverted most changes back to their original states, however for the convenience initializers I've implemented another idea, e.g.

/// convenience init for `decl`
public init(decl: some RawDeclSyntaxNodeProtocol) {
  self = .decl(RawDeclSyntax(decl))
}

The call sites should be more readable now?

item: .init(expr: RawMissingExprSyntax(arena: self.arena)),

@AppAppWorks AppAppWorks force-pushed the improve-ergonomics-raw-syntax-initializers branch from 9e630fd to e95eff3 Compare August 15, 2024 20:23
added convenience initializers for choice enum
changed parameter types in RawNode's initializers from concrete types to generic whenever possible
@AppAppWorks AppAppWorks force-pushed the improve-ergonomics-raw-syntax-initializers branch from e95eff3 to ec9a775 Compare August 15, 2024 20:46
@ahoppen
Copy link
Member

ahoppen commented Aug 15, 2024

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member

ahoppen commented Aug 15, 2024

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Aug 15, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit e0971a7 into swiftlang:main Aug 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants