Skip to content

Conversation

@kimdv
Copy link
Contributor

@kimdv kimdv commented Dec 6, 2021

swiftlang/swift#40436

Comment from swiftlang/swift#40358

For a future PR: Would it make sense to add automatically conformances like ExpressibleAsFunctionCallExpr : ExpressibleAsExpr (just like we have FunctionCallExpr : ExprBuildable and then add ExpressibleAsExpr : ExpressibleAsCodeBlockItem? That way, we wouldn’t need to add conformances to ExpressibleAsCodeBlockItem for every expression node.

The same thing I just showed for expression also applies to statements.

I've had something in the works. Found a branch.

The difference is though, that I wanted to add default implementation to ExpressibleAsSyntaxBuildable, ExpressibleAsStmtBuildable etc.

@ahoppen you write

Would it make sense to add automatically conformances like ExpressibleAsFunctionCallExpr : ExpressibleAsExpr

Will you introduce a new protocol for all SYNTAX_BASE_KINDS or is it an type, and do you think of the already generated ExpressibleAsExprBuildable?

@ahoppen
Copy link
Member

ahoppen commented Dec 6, 2021

Will you introduce a new protocol for all SYNTAX_BASE_KINDS or is it an type, and do you think of the already generated ExpressibleAsExprBuildable?

Oh, that was a typo in my comment. I wasn’t suggesting to add a new protocol but meant ExpressibleAsExprBuildable.

@kimdv
Copy link
Contributor Author

kimdv commented Dec 6, 2021

OKay, cool!
I have actually been working on something, but never pushed it 🤷‍♂️

Will dig into it as it makes totally sense to make this type of conformances

@kimdv kimdv force-pushed the kimdv/make-default-implementation-of-expressible-as-prtocol branch from 0bf3f40 to 75052e4 Compare December 6, 2021 20:25
@kimdv kimdv force-pushed the kimdv/make-default-implementation-of-expressible-as-prtocol branch 2 times, most recently from 738323e to 2293f0c Compare December 25, 2021 10:38
@kimdv
Copy link
Contributor Author

kimdv commented Dec 25, 2021

Happy holidays @ahoppen 🎄

I'm currently looking into some problems I can't figure out how to solve.
The problem is Type 'SimpleTypeIdentifier' does not conform to protocol 'ExpressibleAsExprBuildable'.

ExpressibleAsSimpleTypeIdentifier conforms to ExpressibleAsTypeExpr that conforms to ExpressibleAsExprBuildable.
There is no default implementation of func createExprBuildable() -> ExprBuildable on ExpressibleAsTypeExpr.

If I add

extension ExpressibleAsTypeExpr {
  public func createExprBuildable() -> ExprBuildable {
    createTypeExpr()
  }
}

Then I get 1. Multiple matching functions named 'createExprBuildable()' with type '() -> ExprBuildable'

I hope you understand, otherwise I will try to explain it again 😁

@ahoppen
Copy link
Member

ahoppen commented Jan 3, 2022

I hope you also had happy and relaxing holidays. 💫

Could you push the source state in which you are seeing the issue you described? That would make it easier for me to play around with it.

@kimdv
Copy link
Contributor Author

kimdv commented Jan 3, 2022

It was very good, thanks!

It should be pushed and update to date with the latest state with the error I described.

@ahoppen ahoppen self-assigned this Jan 4, 2022
@ahoppen
Copy link
Member

ahoppen commented Jan 4, 2022

If I checkout your branch and build it using

./build-script.py --toolchain /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2021-12-04-a.xctoolchain/usr

it builds fine without any error.

@kimdv
Copy link
Contributor Author

kimdv commented Jan 4, 2022

That is strange! 🤷‍♂️

I have just tried ./build-script.py --toolchain /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2021-12-23-a.xctoolchain/usr --verbose, and it compiled fine.

I did try to run it verbose and noticed it was not building SwiftSyntaxBuilder. It did only run:
** Generating gyb Files **
** Building SwiftSyntax **
** Building SwiftSyntaxParser **

Can you verify that?

@ahoppen
Copy link
Member

ahoppen commented Jan 4, 2022

Indeed, we are only building SwiftSyntaxBuilder when running tests. Adding --test to the build-script.py invocation, I am now seeing the compilation errors.

@kimdv kimdv force-pushed the kimdv/make-default-implementation-of-expressible-as-prtocol branch from 2293f0c to 1339b31 Compare January 4, 2022 13:32
@ahoppen
Copy link
Member

ahoppen commented Jan 5, 2022

OK, I used this as an excuse to do refactoring on how the SwiftSyntaxBuilder files are being generated. That refactoring escalated a little bit but at least in my opinion the generation code is more maintainable in the new form. Please let me know what you think. #350

Back to your problem: The solution fell out while I was doing the refactoring. The problem was that some types had multiple default conformances, for example

                  PoundColumnExpr
                  /             \
                 /               \ 
                /                 \
       ExprBuildable         ExpressibleAsPoundColumnExpr
             |                            |
ExpressibleAsExprBuildable     ExpressibleAsExprBuildable

Both ExprBuildable and ExpressibleAsPoundColumnExpr provided an implementation of createExprBuildable, so the compiler didn’t know which one to use for PoundColumnExpr. The solution was to provide another implementation in PoundColumnExpr. In this simple example this doesn’t look like a huge problem but for the sake of argument, imagine PoundColumnExpr would also be convertible to a string literal that put quotes around it. In that case, it’s not clear whether createExprBuildable from ExpressibleAsStringLiteral, which puts quotes around the expression, or the one from ExprBuildable that doesn’t include quotes. I resolved this in the following: https://github.com/apple/swift-syntax/pull/350/files#diff-ab75c106f57629d7059d1f0c8b3fb7d59cd3f0b95da1bdfaca1fcac7a18e9cefR118-R123.

@kimdv
Copy link
Contributor Author

kimdv commented Jan 6, 2022

Both ExprBuildable and ExpressibleAsPoundColumnExpr provided an implementation of createExprBuildable, so the compiler didn’t know which one to use for PoundColumnExpr. The solution was to provide another implementation in PoundColumnExpr. In this simple example this doesn’t look like a huge problem but for the sake of argument, imagine PoundColumnExpr would also be convertible to a string literal that put quotes around it. In that case, it’s not clear whether createExprBuildable from ExpressibleAsStringLiteral, which puts quotes around the expression, or the one from ExprBuildable that doesn’t include quotes. I resolved this in the following: https://github.com/apple/swift-syntax/pull/350/files#diff-ab75c106f57629d7059d1f0c8b3fb7d59cd3f0b95da1bdfaca1fcac7a18e9cefR118-R123.

Just to be clear and ensure that I have understood it correctly (after have read in like 100 times):

let foo1: ExprBuildable = PoundColumnExpr()
let foo2: ExpressibleAsStringLiteral = PoundColumnExpr()

func buildSyntax(expo: ExprBuildable) -> ExprBuildable {
    return expr. createExprBuildable()
}

If I have understood it then, foo1 will be without quotes around the expression, and foo2 will be with, because we then explicitly tell it's a ExpressibleAsStringLiteral?

@ahoppen
Copy link
Member

ahoppen commented Jan 7, 2022

Probably the easiest way to explain it is by an example, so I created a reduce example, inspired by the one you were facing, without any dependencies on SwiftSyntax. Copy and paste this into an Xcode project and I hope you can play around with it. I hope that it’s self-explaining. If not, we’ve got a common base to talk about.

// MARK: - SwiftSyntax nodes

protocol Syntax {
  var sourceCode: String { get }
}

protocol ExprSyntax: Syntax {}

struct IntLiteralSyntax: ExprSyntax {
  let value: Int
  var sourceCode: String {
    return String(value)
  }
}

struct CodeBlockSyntax: ExprSyntax {
  let items: [Syntax]
  var sourceCode: String {
    let itemsSourceCode = items.map(\.sourceCode).joined(separator: "\n")
    return "{\(itemsSourceCode)}"
  }
}

// MARK: - ExpressibleAsProtocols

protocol ExpressibleAsSyntaxBuildable {
  func buildSyntax() -> Syntax
}

protocol ExpressibleAsExprSyntaxBuildable: ExpressibleAsSyntaxBuildable {
  func buildExprSyntax() -> ExprSyntax
}

extension ExpressibleAsExprSyntaxBuildable {
  func buildSyntax() -> Syntax {
    return self.buildExprSyntax()
  }
}

/// The problem is this conformance `ExpressibleAsIntLiteral: ExpressibleAsCodeBlock`
protocol ExpressibleAsIntLiteral: ExpressibleAsCodeBlock, ExpressibleAsExprSyntaxBuildable {
  func buildIntLiteral() -> IntLiteralSyntax
}

extension ExpressibleAsIntLiteral {
  func buildExprSyntax() -> ExprSyntax {
    return buildIntLiteral()
  }

  func buildCodeBlock() -> CodeBlockSyntax {
    return CodeBlockSyntax(items: [self.buildIntLiteral()])
  }

//  This is the solution to the problem, uncomment when reading "Solution" below
//  func buildSyntax() -> Syntax {
//    return buildIntLiteral()
//  }
}

protocol ExpressibleAsCodeBlock: ExpressibleAsSyntaxBuildable {
  func buildCodeBlock() -> CodeBlockSyntax
}

extension ExpressibleAsCodeBlock {
  func buildSyntax() -> Syntax {
    return buildCodeBlock()
  }
}

// MARK: - BuildableNodes

struct IntLiteral: ExpressibleAsIntLiteral {
  let value: Int

  func buildIntLiteral() -> IntLiteralSyntax {
    return IntLiteralSyntax(value: value)
  }
}

struct CodeBlock: ExpressibleAsCodeBlock {
  let items: [IntLiteral]

  func buildCodeBlock() -> CodeBlockSyntax {
    return CodeBlockSyntax(items: items.map({ $0.buildIntLiteral() }))
  }
}

// MARK: - Code that triggers the issue


let x = IntLiteral(value: 42)

print(x.buildSyntax().sourceCode)
/// Which path should the call to `buildSyntax` take?
/// Option 1 with call stack:
///  - `ExpressibleAsExprLiteral.buildSyntax()` (available because `IntLiteral` transitively conforms to `ExpressibleAsExprLiteral` via `ExpressibleAsIntLiteral`)
///  - `ExpressibleAsIntLiteral.buildExprSyntax()`
///  - `IntLiteral.buildIntLiteral()`
/// This option produces the result `42`.
///
/// Option 2 with call stack:
///  - `ExpressibleAsCodeBlock.buildSyntax()` (available because `IntLiteral` transitively conforms to `ExpressibleAsExprLiteral` via `ExpressibleAsIntLiteral`)
///  - `ExporessibleAsIntLiteral.buildCodeBlock()` (effectively wrapping `42` in braces)
///  - `IntLiteral.buildIntLiteral`
/// This option produces the result `{42}`.
///
/// The solution is to add the uncommented code above. In that case the `buildSyntax` methods in `ExpressibleAsExprLiteral` and `ExpressibleAsCodeBlock` are overriden by `ExpressibleAsIntLiteral.buildSyntax`, so there is no ambiguity anymore.

@kimdv
Copy link
Contributor Author

kimdv commented Jan 18, 2022

Fixed in #350

@kimdv kimdv closed this Jan 18, 2022
@kimdv kimdv deleted the kimdv/make-default-implementation-of-expressible-as-prtocol branch January 18, 2022 11:40
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