Skip to content
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

Delete dependsOn support #2876

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

meg-gupta
Copy link
Contributor

No description provided.

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@_expose(wasm, "swift_wasm_macro_v1_pump")
@_expose(wasm,"swift_wasm_macro_v1_pump")
Copy link
Member

Choose a reason for hiding this comment

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

I assume this will cause formatting verification to fail.

return self.as(LifetimeTypeSpecifierSyntax.self)!
}
}
public typealias Element = SimpleTypeSpecifierSyntax
Copy link
Member

Choose a reason for hiding this comment

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

This is an API-breaking change because clients might be switching over the element enum and using the simpleTypeSpecifier case to extract the value. It’s a little unfortunate that we did release swift-syntax 600 with TypeSpecifierListSyntax.Element as a SyntaxChildChoices and now need to undo it again.

@rintaro @hamishknight @bnbarham Do you think we should take the API breakage again or should we somehow keep Element: SyntaxChildChoices even if it only has a single case?

Copy link
Member

Choose a reason for hiding this comment

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

@rintaro and I just discussed this and decided that what you did, ie. having a SyntacChildChoices with a single element, here is the best choice. It is one less API break and set us up in case we want to add type specifiers with detail in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I mixed up what we wanted and what we have here. TypeSpecifierListSyntax. Element should continue being a SyntaxChildChoices enum, just with a single case and not change to SimpleTypeSpecifierSyntax in order to maintain API compatibility.

@meg-gupta meg-gupta force-pushed the deletedependson branch 2 times, most recently from a4e9fe3 to 8f28526 Compare October 9, 2024 17:51
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#77030

@swift-ci smoke test

@meg-gupta
Copy link
Contributor Author

Please test with following pull request:
swiftlang/swift#77030
swiftlang/swift-testing#765

@swift-ci smoke test

@meg-gupta
Copy link
Contributor Author

@swift-ci test

When elementChoices of a Node has a single syntax node as a choice, it's
`Element` type is of that syntax node type.

Add a flag disableSameTypeForUniqueChoice to disable this behavior.

This is added for a TypeSpecifier node to avoid the api break from
deleting LifetimeTypeSpecifier from its elementChoices.
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test windows platform

Comment on lines +92 to +94
} else if let node = self.collectionNode, node.elementChoices.count == 1, !node.disableSameTypeForUniqueChoice {
return []
} else if let node = self.collectionNode, node.elementChoices.count >= 1 {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified to

Suggested change
} else if let node = self.collectionNode, node.elementChoices.count == 1, !node.disableSameTypeForUniqueChoice {
return []
} else if let node = self.collectionNode, node.elementChoices.count >= 1 {
} else if let node = self.collectionNode, (node.elementChoices.count > 1 || node.disableSameTypeForUniqueChoice) {

@@ -55,6 +55,8 @@ public class Node: NodeChoiceConvertible {
/// function that should be invoked to create this node.
public let parserFunction: TokenSyntax?

public let disableSameTypeForUniqueChoice: Bool
Copy link
Member

Choose a reason for hiding this comment

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

Please correct me if I’m wrong but I think we only support this for syntax collections, right? If so, I think it would be a cleaner design to modify Node.Data.collection to

fileprivate enum SyntaxCollectionElement {
  /// The syntax collection can only have a single kind of syntax node as its element.
  case singleOption(SyntaxNodeKind)
  /// Create a `SyntaxChildChoices` enum for the collection’s elements.
  case choices([SyntaxNodeKind])
}

case collection(SyntaxCollectionElement)

And with that CollectionNode can gain

var generateSyntaxChoicesEnum: Bool {
    switch node.data {
    case .layout:
      preconditionFailure("NodeLayoutView must wrap a Node with data `.collection`")
    case .collection(.singleOption(_)): 
      return false
    case .collection(.choices(_)): 
      return true
    }
} 

That way we don’t need to repeat the > 1 || disableSameTypeForUniqueChoice check everywhere.

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