Skip to content

Conversation

@kimdv
Copy link
Contributor

@kimdv kimdv commented Nov 14, 2021

Initially I wanted to do something like

extension Array: ExpressibleAsExprList where Element: ExpressibleAsExprBuildable { 
  public func createExprList() -> ExprList {
    ExprList(self)
  }
}

So we could remove ExprList from places like here as conditions is ExpressibleAsConditionElementList`

IfStmt(conditions: ExprList([
    IntegerLiteralExpr(digits: "n"),

    BinaryOperatorExpr(operatorToken: TokenSyntax.unspacedBinaryOperator("<=")),

    IntegerLiteralExpr(1)
 ]), body: ifCodeBlock)

But I got some compile errors I couldn't solve:

  • Conditional conformance of type 'Array<Element>' to protocol 'ExpressibleAsExprList' does not imply conformance to inherited protocol 'ExpressibleAsConditionElement'
  • Conditional conformance of type 'Array<Element>' to protocol 'ExpressibleAsExprList' does not imply conformance to inherited protocol 'ExpressibleAsConditionElement'

What I could figure out was that we need extension Array: ExpressibleAsExprList to conform to all the protocols that ExpressibleAsExprList inheritance from. I could just figure out how to make that nice and clean

Maybe I will look into it later again if you don't have any idea, that could make it simple 😁
But I think this is an nice enhancement

@ahoppen
Copy link
Member

ahoppen commented Nov 15, 2021

Without trying it myself, have you tried

extension ExpressibleAsExprList: ExpressibleByArrayLiteral {
  
}

or, if that doesn’t work, add the protocol conformance to ExpressibleByArrayLiteral to the definition of ExpressibleAsExprList.


Unrelated to this PR: I just realized that the standard library protocols are called ExpressibleBy and ours are called ExpressibleAs. Maybe we should change our protocols to also be called ExpressibleBy.

@kimdv
Copy link
Contributor Author

kimdv commented Nov 15, 2021

Unrelated to this PR: I just realized that the standard library protocols are called ExpressibleBy and ours are called ExpressibleAs. Maybe we should change our protocols to also be called ExpressibleBy.

Funny. I actually also noticed that yesterday when playing around with this 😆
But didn't think more about it 🤷‍♂️

@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch 2 times, most recently from 9697a00 to 1d6a6fa Compare November 15, 2021 20:12
@kimdv
Copy link
Contributor Author

kimdv commented Nov 15, 2021

The problem is that protocols like:

public protocol ExpressibleByConditionElement: ExpressibleByConditionElementList {
  func createConditionElement() -> ConditionElement
}
public protocol ExpressibleByConditionElementList: ExpressibleByArrayLiteral {
  func createConditionElementList() -> ConditionElementList
}

ConditionElement then need to implement init(arrayLiteral elements: Self.ArrayLiteralElement...) because of this.

There will come more of those cases as we add more conformances in protocols.py

I think it will be best if we could make all syntax collections conform to this, and not just some of them.
As that will make the API a bit strange.

Second: We will have a Redundant conformance constraint 'Self' : 'ExpressibleByArrayLiteral'.
So either we should make some more smart way to figure out how to add this one.

@ahoppen
Copy link
Member

ahoppen commented Nov 16, 2021

This shouldn’t be an issue though if we implement it(arrayLiteral elements: Self.ArrayLiteralElement…) as an extension on ExpressibleByConditionElement right?

@kimdv
Copy link
Contributor Author

kimdv commented Nov 16, 2021

This shouldn’t be an issue though if we implement it(arrayLiteral elements: Self.ArrayLiteralElement…) as an extension on ExpressibleByConditionElement right?

I'm not sure I understand the question?
Do you think that ExpressibleByConditionElement need to implement the init(arrayLiteral elements: Self.ArrayLiteralElement…) or should ExpressibleByConditionElementList implement it?

Because I don't this this would be possible (I haven't tried it)

public protocol ExpressibleByConditionElement {
  init(arrayLiteral elements: Self.ArrayLiteralElement) {
  self. createConditionElement(self)  
  }
}

Do we not need to call an init method inside an init?

@ahoppen
Copy link
Member

ahoppen commented Nov 16, 2021

I was thinking of something like this.

// NEW: Add ExpressibleByArrayLiteral conformance here
protocol ExpressibleAsExprList : ExpressibleByArrayLiteral {}

protocol ExpressibleAsExprBuildable {}

// NEW: Generate this extension
extension ExpressibleAsExprList {
  init(arrayLiteral elements: ExpressibleAsExprBuildable...) {
    // self.init(elements) or something like this. `elements` has type `Array<ExpressibleAsExprBuildable>`
  }
}

struct ExprList : ExpressibleAsExprList {}

struct ExprBuildable : ExpressibleAsExprBuildable {}

@kimdv
Copy link
Contributor Author

kimdv commented Nov 16, 2021

I was thinking of something like this.

// NEW: Add ExpressibleByArrayLiteral conformance here
protocol ExpressibleAsExprList : ExpressibleByArrayLiteral {}

protocol ExpressibleAsExprBuildable {}

// NEW: Generate this extension
extension ExpressibleAsExprList {
  init(arrayLiteral elements: ExpressibleAsExprBuildable...) {
    // self.init(elements) or something like this. `elements` has type `Array<ExpressibleAsExprBuildable>`
  }
}

struct ExprList : ExpressibleAsExprList {}

struct ExprBuildable : ExpressibleAsExprBuildable {}

We cannot access the self.init on ExpressibleAsExprList as it is a protocol?
Or am I missing something?

When ExpressibleByConditionElement implements ExpressibleByConditionElementList be doing

https://github.com/apple/swift-syntax/blob/af253dc89d179b33bf8d0c4340678f42c18738ed/Sources/SwiftSyntaxBuilder/gyb_generated/Buildables.swift#L11205-L11209

So I'm not sure how we don't need to implement the init(arrayLiteral elements: ExpressibleAsExprBuildable...)

@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch 2 times, most recently from 8f9f4cd to da5de25 Compare November 20, 2021 14:57
@ahoppen
Copy link
Member

ahoppen commented Nov 22, 2021

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 73883de

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 73883de

@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch from da5de25 to 22a92c4 Compare December 2, 2021 19:52
@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch 2 times, most recently from 0463764 to b5892bb Compare December 25, 2021 10:48
@kimdv
Copy link
Contributor Author

kimdv commented Dec 25, 2021

@ahoppen when you have time, will you see my comment?

@ahoppen
Copy link
Member

ahoppen commented Jan 3, 2022

Sorry, I completely missed your comment. Thanks for pinging me.

Of course, you’re right. We don’t want to be able to create an arbitrary ExpressibleAsExprList from an array literal, but only an ExprList itself, so we shouldn’t be conforming ExpressibleAsExprList: ExpressibleByArrayLiteral, but ExprList: ExpressibleByArrayLiteral, so something like this:

extension ExprList: ExpressibleByArrayLiteral {
  init(arrayLiteral elements: ExpressibleAsExprBuildable...) {
    self.init(elements)
  }
}

This also matches your initial description of the PR where createExprList creates an ExprList and not an ExpressibleAsExprList.

@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch from b5892bb to ae96596 Compare January 3, 2022 11:24
@kimdv
Copy link
Contributor Author

kimdv commented Jan 3, 2022

@swift-ci Please test

@kimdv kimdv requested a review from ahoppen January 3, 2022 11:25
@swift-ci
Copy link
Contributor

swift-ci commented Jan 3, 2022

Build failed
Swift Test Linux Platform
Git Sha - b5892bb

@swift-ci
Copy link
Contributor

swift-ci commented Jan 3, 2022

Build failed
Swift Test OS X Platform
Git Sha - b5892bb

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.

Looks like a very nice addition to me.

@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch from ae96596 to 032639f Compare January 4, 2022 09:42
@kimdv
Copy link
Contributor Author

kimdv commented Jan 4, 2022

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Jan 4, 2022

Build failed
Swift Test Linux Platform
Git Sha - ae96596

@kimdv kimdv force-pushed the kimdv/make-array-conform-to-expressible-as branch from 032639f to 0102d5d Compare January 4, 2022 09:43
@swift-ci
Copy link
Contributor

swift-ci commented Jan 4, 2022

Build failed
Swift Test OS X Platform
Git Sha - ae96596

@kimdv
Copy link
Contributor Author

kimdv commented Jan 4, 2022

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

swift-ci commented Jan 4, 2022

Build failed
Swift Test Linux Platform
Git Sha - 032639f

@kimdv kimdv merged commit 6d5fb4e into swiftlang:main Jan 4, 2022
@kimdv kimdv deleted the kimdv/make-array-conform-to-expressible-as branch January 4, 2022 13:22
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.

3 participants