Conversation
WalkthroughThis pull request extends the Swift package manifest parser to recognize and handle version requirements expressed using SwiftPM's 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Swift/PackageSwift.hs`:
- Around line 98-140: The SwiftVersion type currently allows any number of
positional parts; change the model and parser to enforce exactly three numeric
components (major, minor, patch) and preserve prerelease/build lists: replace
the parts :: [Text] field with explicit major/minor/patch fields (or ensure
assembleParts validates exactly three Component values) and update
parseVersionConstructor/parseParts/parseVersionArgument/assembleParts to reject
inputs with fewer or more than three numeric components (returning a parse
failure) while still parsing PrereleaseIdentifiers and BuildMetadataIdentifiers
as before; also update toText (ToText SwiftVersion) to format from the three
numeric fields.
- Around line 120-127: parseVersionArgument currently drops unknown labeled keys
but leaves their values unconsumed, breaking later parsing; update the wildcard
branch so that when key is neither "prereleaseIdentifiers" nor
"buildMetadataIdentifiers" you explicitly parse-and-discard the corresponding
value before returning Nothing. Concretely, in parseVersionArgument replace `_
-> pure Nothing` with a branch that attempts to consume parseStringArray (for
bracketed arrays) or a single token/value (e.g. digits/quoted string/identifier
via the same lexeme/character parsers used elsewhere such as digitChar or
takeWhile1P) and ignore the result, then return pure Nothing; this ensures
parseVersionArgument (and the sepEndBy1 loop that calls it) will not leave the
value for the next iteration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c176a8fb-a347-4b83-8d44-1302ab889b22
📒 Files selected for processing (5)
Changelog.mdsrc/Strategy/Swift/PackageSwift.hstest/Swift/PackageSwiftSpec.hstest/Swift/testdata/Package.full.swifttest/Swift/testdata/Package.swift
| -- | Represents https://developer.apple.com/documentation/packagedescription/version | ||
| data SwiftVersion = SwiftVersion | ||
| { parts :: [Text] | ||
| , prereleaseIdentifiers :: [Text] | ||
| , buildMetadataIdentifiers :: [Text] | ||
| } | ||
|
|
||
| instance ToText SwiftVersion where | ||
| toText SwiftVersion{..} = version <> prerelease <> build | ||
| where | ||
| version = (intercalate "." parts) | ||
| prerelease = if not $ null prereleaseIdentifiers then "-" <> (intercalate "." prereleaseIdentifiers) else "" | ||
| build = if not $ null buildMetadataIdentifiers then "+" <> (intercalate "." buildMetadataIdentifiers) else "" | ||
|
|
||
| data SwiftVersionPart = Component Text | PrereleaseIdentifiers [Text] | BuildMetadataIdentifiers [Text] | ||
|
|
||
| parseVersionConstructor :: Parser SwiftVersion | ||
| parseVersionConstructor = (symbol "Version") >> assembleParts <$> parseParts | ||
| where | ||
| parseParts :: Parser [SwiftVersionPart] | ||
| parseParts = betweenBrackets $ catMaybes <$> sepEndBy1 parseVersionArgument (symbol ",") | ||
|
|
||
| parseVersionArgument :: Parser (Maybe SwiftVersionPart) | ||
| parseVersionArgument = do | ||
| key <- (optional . try) (lexeme $ takeWhile1P (Just "package key") (`notElem` (":," :: String)) <* symbol ":") | ||
| case key of | ||
| Nothing -> (Just . Component) . toText <$> some digitChar | ||
| Just ("prereleaseIdentifiers") -> (Just . PrereleaseIdentifiers) <$> parseStringArray | ||
| Just ("buildMetadataIdentifiers") -> (Just . BuildMetadataIdentifiers) <$> parseStringArray | ||
| _ -> pure Nothing | ||
|
|
||
| parseStringArray :: Parser [Text] | ||
| parseStringArray = betweenSquareBrackets (sepEndBy parseQuotedText (symbol ",")) | ||
|
|
||
| assembleParts :: [SwiftVersionPart] -> SwiftVersion | ||
| assembleParts = | ||
| foldl' | ||
| ( \version part -> case part of | ||
| Component p -> version{parts = (parts version) ++ [p]} | ||
| PrereleaseIdentifiers p -> version{prereleaseIdentifiers = p} | ||
| BuildMetadataIdentifiers p -> version{buildMetadataIdentifiers = p} | ||
| ) | ||
| (SwiftVersion{parts = [], prereleaseIdentifiers = [], buildMetadataIdentifiers = []}) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: tighten SwiftVersion invariants.
SwiftVersion models parts as an arbitrary [Text], but Swift's Version always has exactly three numeric components (major, minor, patch). The current shape lets the parser silently accept malformed calls like Version(1, 2) or Version(1, 2, 3, 4) and round-trip them through toText, producing versions that won't resolve downstream. Consider modeling it as SwiftVersion { major, minor, patch, prereleaseIdentifiers, buildMetadataIdentifiers } and enforcing the three positional components during parsing. Not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Strategy/Swift/PackageSwift.hs` around lines 98 - 140, The SwiftVersion
type currently allows any number of positional parts; change the model and
parser to enforce exactly three numeric components (major, minor, patch) and
preserve prerelease/build lists: replace the parts :: [Text] field with explicit
major/minor/patch fields (or ensure assembleParts validates exactly three
Component values) and update
parseVersionConstructor/parseParts/parseVersionArgument/assembleParts to reject
inputs with fewer or more than three numeric components (returning a parse
failure) while still parsing PrereleaseIdentifiers and BuildMetadataIdentifiers
as before; also update toText (ToText SwiftVersion) to format from the three
numeric fields.
| parseVersionArgument :: Parser (Maybe SwiftVersionPart) | ||
| parseVersionArgument = do | ||
| key <- (optional . try) (lexeme $ takeWhile1P (Just "package key") (`notElem` (":," :: String)) <* symbol ":") | ||
| case key of | ||
| Nothing -> (Just . Component) . toText <$> some digitChar | ||
| Just ("prereleaseIdentifiers") -> (Just . PrereleaseIdentifiers) <$> parseStringArray | ||
| Just ("buildMetadataIdentifiers") -> (Just . BuildMetadataIdentifiers) <$> parseStringArray | ||
| _ -> pure Nothing |
There was a problem hiding this comment.
Unknown keyword arguments silently drop the value and will break parsing.
When parseVersionArgument matches a keyed argument whose name is neither prereleaseIdentifiers nor buildMetadataIdentifiers, the _ -> pure Nothing branch consumes the key: prefix but leaves the value unconsumed. The next iteration of sepEndBy1 will then attempt to parse the leftover value (e.g. a [...] array or quoted string) as a new positional argument and fail, even though from the user's perspective the Version(...) call is well-formed.
Today SwiftPM's Version initializer has a fixed signature (major, minor, patch, prereleaseIdentifiers, buildMetadataIdentifiers) so this isn't reachable in practice, but it makes the parser brittle against future additions and against any unexpected label. Consider consuming and discarding the value for unknown keys so parsing continues.
🔧 Suggested fix
parseVersionArgument :: Parser (Maybe SwiftVersionPart)
parseVersionArgument = do
key <- (optional . try) (lexeme $ takeWhile1P (Just "package key") (`notElem` (":," :: String)) <* symbol ":")
case key of
Nothing -> (Just . Component) . toText <$> some digitChar
Just ("prereleaseIdentifiers") -> (Just . PrereleaseIdentifiers) <$> parseStringArray
Just ("buildMetadataIdentifiers") -> (Just . BuildMetadataIdentifiers) <$> parseStringArray
- _ -> pure Nothing
+ _ -> Nothing <$ (parseStringArray $> () <|> void parseQuotedText <|> void (some digitChar))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Strategy/Swift/PackageSwift.hs` around lines 120 - 127,
parseVersionArgument currently drops unknown labeled keys but leaves their
values unconsumed, breaking later parsing; update the wildcard branch so that
when key is neither "prereleaseIdentifiers" nor "buildMetadataIdentifiers" you
explicitly parse-and-discard the corresponding value before returning Nothing.
Concretely, in parseVersionArgument replace `_ -> pure Nothing` with a branch
that attempts to consume parseStringArray (for bracketed arrays) or a single
token/value (e.g. digits/quoted string/identifier via the same lexeme/character
parsers used elsewhere such as digitChar or takeWhile1P) and ignore the result,
then return pure Nothing; this ensures parseVersionArgument (and the sepEndBy1
loop that calls it) will not leave the value for the next iteration.
tjugdev
force-pushed
the
ane-2911-swift-version-syntax
branch
from
f78d89a to
a6ad085
Compare
tjugdev
force-pushed
the
ane-2911-swift-version-syntax
branch
from
a6ad085 to
ec8283f
Compare
1 participant
Overview
Swift lets a version string be defined as either a string, or as an instance of
Version(docs). The parser did not previously support this version syntax, and this PR adds support for it.Note: Package.swift files are just Swift files, supporting the full Swift grammar. Since we're not going to write a full Swift interpreter, we're just choosing to support a reasonable subset of the Swift grammar in a limited context in order to parse most Package.swift files. For complicated Package.swift files, other strategies will be more suitable, such as parsing the JSON Package.resolved.
Acceptance criteria
Package.swift files using the
Version(...)syntax for versions can be parsed.Testing plan
Risks
Metrics
References
Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.