[ANE-2908] Swift Package.swift parser improvements#1695
Conversation
WalkthroughThis pull request fixes the Swift 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 206-208: The parser incorrectly treats the "products" and
"targets" skips as mandatory because `_ <- try $ parseNonDepSection "products"`
and `_ <- try $ parseNonDepSection "targets"` still require success; change
these to truly optional parses (e.g., use optional (parseNonDepSection
"products") or parseNonDepSection "products" <|> pure () and similarly for
"targets") so the control flow can reach `try parseDeps <|> pure []` when those
sections are absent; update the occurrences of parseNonDepSection for "products"
and "targets" accordingly to avoid rejecting valid manifests.
- Around line 208-211: The current `try parseDeps <|> pure []` silently
backtracks on any parse error under `parseDeps`, hiding malformed
`dependencies:` entries; replace that fallback with a non-backtracking optional
so malformed content fails but a missing section yields []. Concretely, remove
the `try ... <|> pure []` pattern and use `option [] parseDeps` (or `optional
parseDeps >>= maybe [] id`) so `parseDeps` (which uses `parsePackageDep` and
`betweenSquareBrackets`) can report real errors, and ensure you do not wrap
`parseDeps` in `try` which would swallow its failures; keep
`parseNonDepSection`, `parsePackageDep`, and `betweenSquareBrackets` unchanged.
🪄 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: 852ce174-6a88-4410-b964-8f15d2adde61
📒 Files selected for processing (5)
Changelog.mdsrc/Strategy/Swift/PackageSwift.hstest/Swift/PackageSwiftSpec.hstest/Swift/testdata/Package.no-deps.swifttest/Swift/testdata/Package.swift
| _ <- try $ parseNonDepSection "products" | ||
| _ <- try $ parseNonDepSection "targets" | ||
| try parseDeps <|> pure [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "PackageSwift.hs" -type f 2>/dev/null | head -5Repository: fossas/fossa-cli
Length of output: 42
🏁 Script executed:
cd /tmp && git ls-files | grep -i packageswiftRepository: fossas/fossa-cli
Length of output: 128
🏁 Script executed:
git ls-files | grep -E "\.hs$" | head -20Repository: fossas/fossa-cli
Length of output: 858
🏁 Script executed:
find . -name "PackageSwift.hs" -o -name "*.hs" | grep -i swift | head -10Repository: fossas/fossa-cli
Length of output: 425
🏁 Script executed:
git ls-files "**/*PackageSwift*"Repository: fossas/fossa-cli
Length of output: 125
🏁 Script executed:
rg -l "PackageSwift" --type haskellRepository: fossas/fossa-cli
Length of output: 217
🏁 Script executed:
git ls-files | grep -E "\.hs$" | xargs grep -l "parsePackageDependencies" 2>/dev/nullRepository: fossas/fossa-cli
Length of output: 94
🏁 Script executed:
git ls-files | grep -E "Swift.*\.hs$"Repository: fossas/fossa-cli
Length of output: 437
🏁 Script executed:
find . -type f -name "*.hs" | xargs grep -l "parseNonDepSection" 2>/dev/null | head -5Repository: fossas/fossa-cli
Length of output: 96
🏁 Script executed:
cat -n src/Strategy/Swift/PackageSwift.hs | sed -n '195,215p'Repository: fossas/fossa-cli
Length of output: 1278
🏁 Script executed:
cat -n src/Strategy/Swift/PackageSwift.hs | sed -n '1,40p'Repository: fossas/fossa-cli
Length of output: 1679
🏁 Script executed:
wc -l src/Strategy/Swift/PackageSwift.hsRepository: fossas/fossa-cli
Length of output: 98
🏁 Script executed:
cat -n test/Swift/PackageSwiftSpec.hs | head -100Repository: fossas/fossa-cli
Length of output: 5494
🏁 Script executed:
grep -A 30 "parsePackageDependencies" test/Swift/PackageSwiftSpec.hs | head -50Repository: fossas/fossa-cli
Length of output: 42
🏁 Script executed:
rg "products.*targets.*dependencies" test/Swift/PackageSwiftSpec.hs -A 10 -B 2Repository: fossas/fossa-cli
Length of output: 42
🏁 Script executed:
ls -la test/Swift/testdata/Repository: fossas/fossa-cli
Length of output: 512
🏁 Script executed:
cat test/Swift/testdata/Package.swift | head -100Repository: fossas/fossa-cli
Length of output: 2472
🏁 Script executed:
cat test/Swift/testdata/Package.no-deps.swiftRepository: fossas/fossa-cli
Length of output: 511
🏁 Script executed:
cat -n test/Swift/testdata/v1/Package.swift test/Swift/testdata/v2/Package.swift test/Swift/testdata/v3/Package.swift test/Swift/testdata/v4/Package.swift 2>/dev/null | head -60Repository: fossas/fossa-cli
Length of output: 42
🏁 Script executed:
# Check if there are any test cases that omit products or targets sections
grep -r "let package = Package" test/Swift/testdata/ -A 20 | head -100Repository: fossas/fossa-cli
Length of output: 2307
🏁 Script executed:
cat -n src/Strategy/Swift/PackageSwift.hs | sed -n '40,50p'Repository: fossas/fossa-cli
Length of output: 315
🏁 Script executed:
# Let's trace through the parser logic carefully
python3 << 'PYTHON'
# In Haskell's do notation with <-:
# x <- parser means the parser MUST succeed
# If parser fails and backtracks with try, the do block still fails
# only the last line has the <|> alternative
# So:
# _ <- try $ parseNonDepSection "products" -- MUST succeed (even with try backtrack)
# _ <- try $ parseNonDepSection "targets" -- MUST succeed (even with try backtrack)
# try parseDeps <|> pure [] -- Can fail and fall back to pure []
print("In Haskell do-notation:")
print(" _ <- try parser1 means: parser1 must succeed (try only enables backtracking, not optional parsing)")
print(" _ <- try parser2 means: parser2 must succeed")
print(" try parser3 <|> pure [] means: parser3 can fail and fall back to pure []")
print()
print("Therefore:")
print(" If manifest lacks 'products:' section entirely:")
print(" - Line 206 fails (can't find 'products:')")
print(" - Even with 'try', the do-block fails")
print(" - Never reaches the 'pure []' fallback")
print()
print(" If manifest has 'products:' and 'targets:' but no 'dependencies:':")
print(" - Lines 206-207 succeed")
print(" - Line 208: parseD eps fails (can't find 'dependencies:')")
print(" - Falls back to 'pure []' ✓ This works")
PYTHONRepository: fossas/fossa-cli
Length of output: 708
🏁 Script executed:
# Let's verify what the actual Swift Package.swift specification allows
# Check Apple's documentation context - does products/targets need to be present?
python3 << 'PYTHON'
# Verify parser behavior with a concrete example
print("=== Swift Package.swift Specification ===")
print()
print("In Apple's Swift Package Manager documentation:")
print("- 'products' section is OPTIONAL (defaults to library if omitted)")
print("- 'targets' section is OPTIONAL (inferred from sources if omitted)")
print("- 'dependencies' section is OPTIONAL")
print()
print("=== Current Parser Behavior ===")
print()
print("Lines 206-207 in do-block:")
print(" _ <- try $ parseNonDepSection \"products\"")
print(" _ <- try $ parseNonDepSection \"targets\"")
print()
print("These lines REQUIRE both sections to exist and be parseable.")
print()
print("Counter-example (valid but will FAIL):")
print("""
let package = Package(
name: "Example",
dependencies: [
.package(url: "...", from: "1.0.0")
]
)
""")
print()
print("Why it fails:")
print(" 1. Parser tries to find 'products:' at line 206")
print(" 2. Can't find it → skipManyTill fails")
print(" 3. try enables backtracking, but do-block still fails")
print(" 4. Never reaches 'pure []' fallback at line 208")
PYTHONRepository: fossas/fossa-cli
Length of output: 897
🏁 Script executed:
cat -n src/Strategy/Swift/PackageSwift.hs | sed -n '210,212p'Repository: fossas/fossa-cli
Length of output: 421
🏁 Script executed:
# Also verify that the test data only includes manifests with both products and targets
find test/Swift/testdata -name "*.swift" -type f -exec echo "=== {} ===" \; -exec grep -E "^\s*(products|targets|dependencies):" {} \;Repository: fossas/fossa-cli
Length of output: 289
🏁 Script executed:
# Let's verify the Megaparsec documentation/semantics are as I understand
# by checking if try in do-notation makes parsers optional or just changes backtracking
python3 << 'PYTHON'
print("=== Megaparsec Parser Semantics Verification ===")
print()
print("In Haskell Megaparsec with do-notation:")
print()
print("CASE 1: _ <- parser")
print(" - Parser must succeed")
print(" - If parser fails → entire do-block fails")
print()
print("CASE 2: _ <- try parser")
print(" - Parser still must succeed for the do-block to continue")
print(" - 'try' ONLY affects how the parser backtracks on failure")
print(" - Does NOT make the parser optional in a do-block")
print(" - If parser fails → entire do-block still fails")
print()
print("CASE 3: last_expr <|> alternative")
print(" - last_expr can fail and fall back to alternative")
print(" - This is the ONLY way to make something optional in do-block")
print()
print("=== Current Code Analysis ===")
print()
print("_ <- try $ parseNonDepSection \"products\"")
print(" → parseNonDepSection \"products\" MUST succeed")
print()
print("_ <- try $ parseNonDepSection \"targets\"")
print(" → parseNonDepSection \"targets\" MUST succeed")
print()
print("try parseDeps <|> pure []")
print(" → parseDeps can fail and fall back to pure []")
print()
print("=== Conclusion ===")
print()
print("The review comment is CORRECT:")
print(" ✓ Both products and targets sections are REQUIRED")
print(" ✓ Manifests lacking these (valid per Swift spec) will FAIL")
print(" ✓ The pure [] fallback is unreachable for such manifests")
print(" ✓ This is a structural defect in the parser")
PYTHONRepository: fossas/fossa-cli
Length of output: 1187
The products and targets section skips are mandatory, not optional.
In Haskell do-notation, _ <- try parser still requires the parser to succeed—try only controls backtracking behavior, not optionality. A Package.swift manifest that omits either the products: or targets: section (both valid per Apple's specification) will fail at lines 206–207 and never reach the pure [] fallback at line 208. The parser currently enforces a specific manifest shape and won't parse valid manifests that lack these optional sections. Make these section skips genuinely optional, perhaps by wrapping them with optional or using <|> pure ().
🤖 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 206 - 208, The parser
incorrectly treats the "products" and "targets" skips as mandatory because `_ <-
try $ parseNonDepSection "products"` and `_ <- try $ parseNonDepSection
"targets"` still require success; change these to truly optional parses (e.g.,
use optional (parseNonDepSection "products") or parseNonDepSection "products"
<|> pure () and similarly for "targets") so the control flow can reach `try
parseDeps <|> pure []` when those sections are absent; update the occurrences of
parseNonDepSection for "products" and "targets" accordingly to avoid rejecting
valid manifests.
| try parseDeps <|> pure [] | ||
| where | ||
| parseNonDepSection name = skipManyTill anySingle (symbol (name <> ":")) *> nestedBrackets | ||
| parseDeps = skipManyTill anySingle (symbol "dependencies:") *> betweenSquareBrackets (sepEndBy (lexeme parsePackageDep) $ symbol ",") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Strategy/Swift/PackageSwift.hs | head -220 | tail -30Repository: fossas/fossa-cli
Length of output: 1726
🏁 Script executed:
cat -n src/Strategy/Swift/PackageSwift.hs | head -250 | tail -70Repository: fossas/fossa-cli
Length of output: 3690
🏁 Script executed:
rg -n "try parseDeps|parseNonDepSection|betweenSquareBrackets" src/Strategy/Swift/PackageSwift.hs -A 5 -B 5Repository: fossas/fossa-cli
Length of output: 1460
🏁 Script executed:
head -50 src/Strategy/Swift/PackageSwift.hsRepository: fossas/fossa-cli
Length of output: 1575
🏁 Script executed:
rg -n "parsePackageDep\s*=" src/Strategy/Swift/PackageSwift.hs -A 10Repository: fossas/fossa-cli
Length of output: 668
🏁 Script executed:
rg -n "data SwiftPackageDep|type SwiftPackageDep" src/Strategy/Swift/PackageSwift.hs -B 2 -A 5Repository: fossas/fossa-cli
Length of output: 364
🏁 Script executed:
rg -n "parseGitDep\s*=" src/Strategy/Swift/PackageSwift.hs -A 20Repository: fossas/fossa-cli
Length of output: 937
🏁 Script executed:
fd -i test src/Strategy/Swift/ -type f | head -10Repository: fossas/fossa-cli
Length of output: 230
🏁 Script executed:
rg -l "PackageSwift|parsePackageDependencies" --type hs | grep -i testRepository: fossas/fossa-cli
Length of output: 122
🏁 Script executed:
cat test/Swift/PackageSwiftSpec.hsRepository: fossas/fossa-cli
Length of output: 8570
🏁 Script executed:
cat test/Swift/testdata/Package.swift | head -80Repository: fossas/fossa-cli
Length of output: 2472
🏁 Script executed:
cat test/Swift/testdata/Package.no-deps.swiftRepository: fossas/fossa-cli
Length of output: 511
Fix parser error handling to fail on malformed dependencies: entries.
try parseDeps <|> pure [] backtracks on any failure, so a malformed top-level dependency entry after the dependencies: label is silently accepted as "no dependencies". This hides real parse errors and drops dependency data from analysis.
Safer fallback
- try parseDeps <|> pure []
+ hasDeps <- optional . try $ skipManyTill anySingle (symbol "dependencies:")
+ case hasDeps of
+ Nothing -> pure []
+ Just _ ->
+ betweenSquareBrackets
+ (sepEndBy (lexeme parsePackageDep) $ symbol ",")🤖 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 208 - 211, The current `try
parseDeps <|> pure []` silently backtracks on any parse error under `parseDeps`,
hiding malformed `dependencies:` entries; replace that fallback with a
non-backtracking optional so malformed content fails but a missing section
yields []. Concretely, remove the `try ... <|> pure []` pattern and use `option
[] parseDeps` (or `optional parseDeps >>= maybe [] id`) so `parseDeps` (which
uses `parsePackageDep` and `betweenSquareBrackets`) can report real errors, and
ensure you do not wrap `parseDeps` in `try` which would swallow its failures;
keep `parseNonDepSection`, `parsePackageDep`, and `betweenSquareBrackets`
unchanged.
tjugdev
force-pushed
the
ane-2908-swift-bug
branch
from
21300eb to
4924232
Compare
nficca
left a comment
nficca
left a comment
There was a problem hiding this comment.
I have one nit/question I'm unsure about, but otherwise this looks good.
2 participants
Overview
Updates the parser for
Package.swiftfiles to handle a few more cases:dependenciesarguments withintargets. If these came before the dependencies section, we'd end up parsing these dependencies instead of the actual package dependencies, and fail because they use different syntax.dependenciesargument. The previous parser didn't handle this section missing completely. Now it does, and works even when there are dependencies listed withintargets(which we correctly ignore).Acceptance criteria
We correctly handle
Package.swiftfiles that fall into either of the two cases outlined in the Overview section.Testing plan
cabal run fossa -- . --outputon a directory containing either of thePackage.swiftfiles in test/Swift/testdata/Package.swiftfiles in ANE-2910 are analyzed successfully.References
ANE-2908
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.