Add --enable-experimental-feature to enable those features in the parser.
#876
Conversation
allevato
requested review from
ahoppen,
bnbarham and
shawnhyam
as code owners
| operatorTable.foldAll(Parser.parse(source: sourceBytes, experimentalFeatures: experimentalFeaturesSet)) { _ in }.as( | ||
| SourceFileSyntax.self | ||
| )! |
There was a problem hiding this comment.
| operatorTable.foldAll(Parser.parse(source: sourceBytes, experimentalFeatures: experimentalFeaturesSet)) { _ in }.as( | |
| SourceFileSyntax.self | |
| )! | |
| operatorTable.foldAll(Parser.parse(source: sourceBytes, experimentalFeatures: experimentalFeaturesSet)) { _ in } | |
| .as(SourceFileSyntax.self)! |
Reads better, I think.
| input: input, | ||
| expected: expected, | ||
| linelength: 20, | ||
| experimentalFeatures: ["valueGenerics"] |
There was a problem hiding this comment.
Should we use ExperimentalFeatures enum cases here instead of spelling out the feature names?
There was a problem hiding this comment.
Yeah, I guess that's fine for tests since we don't have to be worried about SPI leakage there. Updated.
allevato
force-pushed
the
experimental-features
branch
2 times, most recently
from
cd992a8 to
e0009ed
Compare
| public func parseForTesting( | ||
| source: String, | ||
| experimentalFeatures: Parser.ExperimentalFeatures | ||
| ) -> SourceFileSyntax { |
There was a problem hiding this comment.
Do we need this still? I think there’s a Parser.parse function that does exactly this now that we don’t need to map experimental features anymore.
There was a problem hiding this comment.
Since the Parser.parse overload that takes experimental features only takes a UTF-8 pointer, I mainly kept it to tidy up the handful of call sites in tests where we would otherwise have to repeat the "make a mutable copy of the string, convert it to a UTF-8 pointer, and then parse it" dance.
There was a problem hiding this comment.
Oh, I see. I would make it an extension of Parser then.
allevato
force-pushed
the
experimental-features
branch
from
e0009ed to
ebc8bce
Compare
…arser. Also add a couple small tests for value generics to exercise the capability in tests. Fixes swiftlang#875.
allevato
force-pushed
the
experimental-features
branch
from
ebc8bce to
fee42c9
Compare
| ) | ||
| var followSymlinks: Bool = false | ||
|
|
||
| @Option( |
There was a problem hiding this comment.
Sorry I didn't see hit PR go by, using String here is not great because the user has no idea what options are valid. Instead using a custom type and conforming it to ExpressibleByArgument and CaseIterable will allow ArgumentParser to provide a list of valid options in the help cli and error output.
There was a problem hiding this comment.
As mentioned in the PR description, we would need Parser.ExperimentalFeatures to become non-SPI for that to be feasible, first. Otherwise, we can't pass the values down through the various API layers because they're public methods and thus would also need to be hidden behind their own SPI.
There was a problem hiding this comment.
Ah thats unfortunate, but good to know!
3 participants
Also add a couple small tests for value generics to exercise the capability in tests, and I took the opportunity to clean up some error handling with the addition of this new user-facing error when an experimental feature isn't recognized.
I'm passing the experimental features through as strings deeper than I'd like, but trying to resolve them to the
Parser.ExperimentalFeaturestype higher in the stack causes more problems since that type is behind an@_spi. To prevent SPI leakage in swift-format's public interfaces (and thus require those interfaces to also have their own@_spi-restricted overloads), just leave them as aSet<String>until we callParser.parsedirectly.This PR requires swiftlang/swift-syntax#2895.
Fixes #875.