Pnpm lock refactor#1725
Conversation
…d helpers - Extract per-version graph builders (buildGraphV4or5, buildGraphV678, buildGraphV9) - Replace Bool useLabels with LabelingMode sum type - Share getPkgNameVersionV6/V9 via parseAtKey - Extract buildGraphPreV9 to eliminate duplicate bodies - Lift shared helpers to top level (toDependency, toDep, applyLabels, etc.) - Fix context string 'Npm Lockfile' -> 'Pnpm Lockfile' - Remove unused Control.Monad (when) import
…r BuildGraphConfig
WalkthroughThe PR adds shared pnpm lockfile types and parsing for multiple lockfile versions, introduces version-specific key parsing and graph config modules for pnpm v4/v5, v6/v7/v8, and v9, and refactors 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 27-35: The test-only partial-function allowance in CLAUDE.md is
too broad and should be narrowed to match the actual policy. Update the “Tests”
guidance so it only permits the specific partial functions already exempted in
HLint for test/spec modules, and keep the rest of the style guidance aligned
with proving safety or avoiding partials elsewhere. Use the existing “Tests” and
Rust style sections in CLAUDE.md as the place to tighten the wording.
In `@src/Strategy/Node/Pnpm/PnpmLock.hs`:
- Around line 236-240: The warning branch in the pnpm lockfile version check
uses a match guard on the result of Text.uncons in the LockfileV4Or5 handling,
which violates the Haskell style guideline against guards in normal code.
Refactor the logic in PnpmLock.hs around the lockfileRawVersion check to use a
nested case or a small shared helper that parses the major version, then keep
the same warning behavior in the branch that detects versions starting with 1,
2, or 3.
- Around line 150-156: The lookup order in the package resolution logic is too
permissive: it checks the stripped version before the exact peer-suffixed
version, which can bind edges/envs to the wrong package instance. Update the
resolution in PnpmLock to prefer the exact depVersion first and only fall back
to withoutPeerDepSuffix for both maybeNonRegistrySrcPackage and
maybeRegistrySrcPackage, keeping the existing Map.lookup flow but reversing the
precedence around depVersion and strippedVersion.
In `@src/Strategy/Node/Pnpm/Types.hs`:
- Around line 335-344: The version classifier in classifyVersion only inspects
the first character of the lockfile version, so multi-digit majors like 10 are
misrouted through the v4/v5 path. Update classifyVersion in Types.hs to parse
the full numeric major component from the TextLike value, dispatching valid
majors to the appropriate VersionClassifier and sending any major >= 9 through
the v9 path; if the format is not a valid numeric major, fail explicitly. Remove
the current match-guard-based character checks in this classifier and keep the
logic centered on the parsed major version.
- Around line 242-286: The ProjectMap and PackageData parsers currently ignore
optionalDependencies, so those edges never reach the dependency graph; add
optional-dependency fields to the shared model and parse them in the FromJSON
instances alongside dependencies and devDependencies. Then thread the new data
through the graph-building path used by buildGraphCore so importer- and
package-level optional dependencies are retained in direct and transitive
construction, following the same treatment as production dependencies unless
there is a separate policy.
- Around line 111-115: Replace the match guard in the new Haskell code with a
case expression to comply with the repository rule against guards. Update the
logic in the Pnpm types handling, using the relevant function or pattern match
in Types.hs, so the same branching is expressed with case instead of a guard
while preserving behavior.
In `@src/Strategy/Node/Pnpm/V4_8.hs`:
- Around line 35-46: The parseAtKey function has a layout/parse issue because
the >>= continuation is aligned with the case alternatives instead of being part
of the Just txt branch. Update parseAtKey in the V4_8 strategy so the
Nothing/Just handling is expressed cleanly, with the slashRequired check using
if instead of a guard where needed, and place the bind inside the Just txt
branch using do notation so the parser can read the function correctly.
🪄 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: 344afd3e-573f-4206-88c7-da352808f958
📒 Files selected for processing (6)
CLAUDE.mdspectrometer.cabalsrc/Strategy/Node/Pnpm/PnpmLock.hssrc/Strategy/Node/Pnpm/Types.hssrc/Strategy/Node/Pnpm/V4_8.hssrc/Strategy/Node/Pnpm/V9.hs
| - **Tests**: Partial functions (`head`, `tail`, `!!`, etc.) are acceptable in test code. | ||
|
|
||
| ## Code Style Guidelines - Rust | ||
| - Format with `rustfmt` via `cargo fmt` | ||
| - Lint with Clippy via `cargo clippy` | ||
| - Follow standard Rust idioms including error handling | ||
| - Use the Rust embedded tools within the Haskell code via the extlib directory | ||
|
|
||
| For complete guidelines, see `docs/contributing/STYLE-GUIDE.md` No newline at end of file | ||
| For complete guidelines, see `docs/contributing/STYLE-GUIDE.md` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Policy references =="
rg -n --hidden -S 'partial functions|head|tail|!!|error|undefined|test code|Tests' \
CLAUDE.md docs/contributing/STYLE-GUIDE.md .hlint.yaml hlint.yaml cabal.project package.yaml \
2>/dev/null || true
echo "== Test-only lint exclusions =="
rg -n --hidden -S 'exclude.*test|test.*exclude|ignore.*test|test.*partial|partial.*test' \
.hlint.yaml hlint.yaml cabal.project package.yaml *.cabal \
2>/dev/null || trueRepository: fossas/fossa-cli
Length of output: 2445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CLAUDE.md context =="
sed -n '20,38p' CLAUDE.md
echo
echo "== HLint partial-function rules =="
sed -n '56,82p' .hlint.yaml
echo
echo "== STYLE-GUIDE partial-function section =="
sed -n '181,194p' docs/contributing/STYLE-GUIDE.mdRepository: fossas/fossa-cli
Length of output: 3463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CLAUDE.md context =="
sed -n '20,38p' CLAUDE.md
echo
echo "== HLint partial-function rules =="
sed -n '56,82p' .hlint.yaml
echo
echo "== STYLE-GUIDE partial-function section =="
sed -n '181,194p' docs/contributing/STYLE-GUIDE.mdRepository: fossas/fossa-cli
Length of output: 3463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CLAUDE.md context =="
sed -n '20,38p' CLAUDE.md
echo
echo "== HLint partial-function rules =="
sed -n '56,82p' .hlint.yaml
echo
echo "== STYLE-GUIDE partial-function section =="
sed -n '181,194p' docs/contributing/STYLE-GUIDE.mdRepository: fossas/fossa-cli
Length of output: 3463
Narrow the test-only partial-function exception. This is broader than the current policy: the HLint config only special-cases a few partial functions in specific **.*Spec/test modules, and the style guide still says to avoid partials and prove safety when they’re necessary. Match CLAUDE.md to that narrower exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` around lines 27 - 35, The test-only partial-function allowance in
CLAUDE.md is too broad and should be narrowed to match the actual policy. Update
the “Tests” guidance so it only permits the specific partial functions already
exempted in HLint for test/spec modules, and keep the rest of the style guidance
aligned with proving safety or avoiding partials elsewhere. Use the existing
“Tests” and Rust style sections in CLAUDE.md as the place to tighten the
wording.
| let strippedVersion = withoutPeerDepSuffix depVersion | ||
| let maybeNonRegistrySrcPackage = | ||
| Map.lookup strippedVersion pkgs | ||
| <|> Map.lookup depVersion pkgs | ||
| let maybeRegistrySrcPackage = | ||
| fmap (strippedVersion,) (Map.lookup (mkPkg depName strippedVersion) pkgs) | ||
| <|> fmap (depVersion,) (Map.lookup (mkPkg depName depVersion) pkgs) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the exact peer-suffixed package before the stripped fallback.
When both /pkg@1.0.0(peer@x) and /pkg@1.0.0 exist, trying the stripped key first can attach edges/envs to the wrong package instance. Prefer the exact depVersion, then fall back to withoutPeerDepSuffix.
Proposed fix
let strippedVersion = withoutPeerDepSuffix depVersion
let maybeNonRegistrySrcPackage =
- Map.lookup strippedVersion pkgs
- <|> Map.lookup depVersion pkgs
+ Map.lookup depVersion pkgs
+ <|> Map.lookup strippedVersion pkgs
let maybeRegistrySrcPackage =
- fmap (strippedVersion,) (Map.lookup (mkPkg depName strippedVersion) pkgs)
- <|> fmap (depVersion,) (Map.lookup (mkPkg depName depVersion) pkgs)
+ fmap (depVersion,) (Map.lookup (mkPkg depName depVersion) pkgs)
+ <|> fmap (strippedVersion,) (Map.lookup (mkPkg depName strippedVersion) pkgs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let strippedVersion = withoutPeerDepSuffix depVersion | |
| let maybeNonRegistrySrcPackage = | |
| Map.lookup strippedVersion pkgs | |
| <|> Map.lookup depVersion pkgs | |
| let maybeRegistrySrcPackage = | |
| fmap (strippedVersion,) (Map.lookup (mkPkg depName strippedVersion) pkgs) | |
| <|> fmap (depVersion,) (Map.lookup (mkPkg depName depVersion) pkgs) | |
| let strippedVersion = withoutPeerDepSuffix depVersion | |
| let maybeNonRegistrySrcPackage = | |
| Map.lookup depVersion pkgs | |
| <|> Map.lookup strippedVersion pkgs | |
| let maybeRegistrySrcPackage = | |
| fmap (depVersion,) (Map.lookup (mkPkg depName depVersion) pkgs) | |
| <|> fmap (strippedVersion,) (Map.lookup (mkPkg depName strippedVersion) pkgs) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 150 - 156, The lookup order
in the package resolution logic is too permissive: it checks the stripped
version before the exact peer-suffixed version, which can bind edges/envs to the
wrong package instance. Update the resolution in PnpmLock to prefer the exact
depVersion first and only fall back to withoutPeerDepSuffix for both
maybeNonRegistrySrcPackage and maybeRegistrySrcPackage, keeping the existing
Map.lookup flow but reversing the precedence around depVersion and
strippedVersion.
| LockfileV4Or5 (PnpmLockfileV4Or5 base) -> | ||
| case Text.uncons (lockfileRawVersion base) of | ||
| Just (c, _) | ||
| | c `elem` ['1', '2', '3'] -> | ||
| logWarn . pretty $ "pnpm-lock file is using older lockFileVersion: " <> lockfileRawVersion base <> ", which is not officially supported!" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid the match guard in the warning branch.
Use a nested if/case or a shared parsed-major helper here.
As per coding guidelines, Haskell normal code should avoid match guards.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Strategy/Node/Pnpm/PnpmLock.hs` around lines 236 - 240, The warning
branch in the pnpm lockfile version check uses a match guard on the result of
Text.uncons in the LockfileV4Or5 handling, which violates the Haskell style
guideline against guards in normal code. Refactor the logic in PnpmLock.hs
around the lockfileRawVersion check to use a nested case or a small shared
helper that parses the major version, then keep the same warning behavior in the
branch that detects versions starting with 1, 2, or 3.
Source: Coding guidelines
There was a problem hiding this comment.
This match guard is uncomplicated so it can stay.
There was a problem hiding this comment.
@csasarak Sounds good — keeping the simple match guard here is reasonable. No further action needed from my side.
| -- > dependencies: | ||
| -- > aws-sdk: | ||
| -- > specifier: 2.1148.0 | ||
| -- > version: 2.1148.0 | ||
| -- @ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the pattern guard with a case.
This is a small local cleanup to keep the new Haskell code within the repository rule against match guards.
Proposed refactor
resolveCatalogVersion :: PnpmCatalogs -> Text -> Text -> Text
resolveCatalogVersion (PnpmCatalogs cats) depName ver
- | Just catalogName <- Text.stripPrefix "catalog:" ver =
+ = case Text.stripPrefix "catalog:" ver of
+ Just catalogName ->
let name = if Text.null catalogName then "default" else catalogName
in fromMaybe ver $ Map.lookup name cats >>= Map.lookup depName
- | otherwise = ver
+ Nothing -> verAs per coding guidelines, Haskell normal code should avoid match guards.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- > dependencies: | |
| -- > aws-sdk: | |
| -- > specifier: 2.1148.0 | |
| -- > version: 2.1148.0 | |
| -- @ | |
| resolveCatalogVersion :: PnpmCatalogs -> Text -> Text -> Text | |
| resolveCatalogVersion (PnpmCatalogs cats) depName ver = case Text.stripPrefix "catalog:" ver of | |
| Just catalogName -> | |
| let name = if Text.null catalogName then "default" else catalogName | |
| in fromMaybe ver $ Map.lookup name cats >>= Map.lookup depName | |
| Nothing -> ver |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Strategy/Node/Pnpm/Types.hs` around lines 111 - 115, Replace the match
guard in the new Haskell code with a case expression to comply with the
repository rule against guards. Update the logic in the Pnpm types handling,
using the relevant function or pattern match in Types.hs, so the same branching
is expressed with case instead of a guard while preserving behavior.
Source: Coding guidelines
| data ProjectMap = ProjectMap | ||
| { directDependencies :: Map Text ProjectMapDepMetadata | ||
| , directDevDependencies :: Map Text ProjectMapDepMetadata | ||
| } | ||
| deriving (Show, Eq, Ord) | ||
|
|
||
| instance FromJSON ProjectMap where | ||
| parseJSON = Yaml.withObject "ProjectMap" $ \obj -> | ||
| ProjectMap | ||
| <$> obj .:? "dependencies" .!= mempty | ||
| <*> obj .:? "devDependencies" .!= mempty | ||
|
|
||
| newtype ProjectMapDepMetadata = ProjectMapDepMetadata | ||
| { version :: Text | ||
| } | ||
| deriving (Show, Eq, Ord) | ||
|
|
||
| instance FromJSON ProjectMapDepMetadata where | ||
| -- This is v5 lock format | ||
| parseJSON (Yaml.String r) = pure $ ProjectMapDepMetadata r | ||
| -- This is v6 lock format | ||
| parseJSON (Yaml.Object obj) = ProjectMapDepMetadata <$> obj .: "version" | ||
| parseJSON other = fail ("Invalid format; expected pure string or an object with a `version` field, got: " <> show other) | ||
|
|
||
| -- | ||
| -- Package data | ||
| -- | ||
|
|
||
| data PackageData = PackageData | ||
| { isDev :: Bool | ||
| , name :: Maybe Text -- only provided when non-registry resolver is used | ||
| , resolution :: Resolution | ||
| , dependencies :: Map Text Text | ||
| , peerDependencies :: Map Text Text | ||
| } | ||
| deriving (Show, Eq, Ord) | ||
|
|
||
| instance FromJSON PackageData where | ||
| parseJSON = Yaml.withObject "PackageData" $ \obj -> | ||
| PackageData | ||
| <$> (obj .:? "dev" .!= False) | ||
| <*> obj .:? "name" | ||
| <*> obj .: "resolution" | ||
| <*> (obj .:? "dependencies" .!= mempty) | ||
| <*> (obj .:? "peerDependencies" .!= mempty) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Parse optionalDependencies instead of dropping them.
The shared model never reads importer- or package-level optionalDependencies, so those edges are unavailable to buildGraphCore and can be omitted from analysis. Add fields here and wire them through direct/transitive graph construction, likely as production dependencies unless the product has a separate optional-dependency policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Strategy/Node/Pnpm/Types.hs` around lines 242 - 286, The ProjectMap and
PackageData parsers currently ignore optionalDependencies, so those edges never
reach the dependency graph; add optional-dependency fields to the shared model
and parse them in the FromJSON instances alongside dependencies and
devDependencies. Then thread the new data through the graph-building path used
by buildGraphCore so importer- and package-level optional dependencies are
retained in direct and transitive construction, following the same treatment as
production dependencies unless there is a separate policy.
| classifyVersion :: TextLike -> Parser VersionClassifier | ||
| classifyVersion (TextLike ver) = case listToMaybe (toString ver) of | ||
| Just '1' -> pure VersionV4Or5 | ||
| Just '2' -> pure VersionV4Or5 | ||
| Just '3' -> pure VersionV4Or5 | ||
| Just '4' -> pure VersionV4Or5 | ||
| Just '5' -> pure VersionV4Or5 | ||
| Just x | x `elem` ['6', '7', '8'] -> pure VersionV678 | ||
| Just '9' -> pure VersionV9 | ||
| _ -> fail $ "expected numeric lockfileVersion, got: " <> show ver |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse the full lockfile major version before dispatching.
listToMaybe only checks the first character, so 10.0 is treated like 1.x and routed through the v4/v5 path. Parse the numeric major component and either route >= 9 to v9 handling or reject it explicitly.
As per coding guidelines, this also removes the match guard in the classifier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Strategy/Node/Pnpm/Types.hs` around lines 335 - 344, The version
classifier in classifyVersion only inspects the first character of the lockfile
version, so multi-digit majors like 10 are misrouted through the v4/v5 path.
Update classifyVersion in Types.hs to parse the full numeric major component
from the TextLike value, dispatching valid majors to the appropriate
VersionClassifier and sending any major >= 9 through the v9 path; if the format
is not a valid numeric major, fail explicitly. Remove the current
match-guard-based character checks in this classifier and keep the logic
centered on the parsed major version.
Source: Coding guidelines
| parseAtKey :: Bool -> Text -> Maybe (Text, Text) | ||
| parseAtKey slashRequired pkgKey = | ||
| case Text.stripPrefix "/" pkgKey of | ||
| Nothing | slashRequired -> Nothing | ||
| Nothing -> Just pkgKey | ||
| Just txt -> Just txt | ||
| >>= \txt -> do | ||
| let (nameAndVersion, peerDepInfo) = Text.breakOn "(" txt | ||
| let (nameWithSlash, version) = Text.breakOnEnd "@" nameAndVersion | ||
| case (Text.stripSuffix "@" nameWithSlash, version) of | ||
| (Just name, v) -> Just (name, v <> peerDepInfo) | ||
| _ -> Nothing |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the surrounding lines with numbering.
git ls-files 'src/Strategy/Node/Pnpm/V4_8.hs'
echo '---'
wc -l src/Strategy/Node/Pnpm/V4_8.hs
echo '---'
sed -n '1,120p' src/Strategy/Node/Pnpm/V4_8.hs | cat -nRepository: fossas/fossa-cli
Length of output: 4607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the surrounding lines with numbering.
git ls-files 'src/Strategy/Node/Pnpm/V4_8.hs'
echo '---'
wc -l src/Strategy/Node/Pnpm/V4_8.hs
echo '---'
sed -n '1,120p' src/Strategy/Node/Pnpm/V4_8.hs | cat -nRepository: fossas/fossa-cli
Length of output: 4607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether a Haskell parser/compiler is available to validate the syntax claim.
command -v ghc >/dev/null && ghc --version || true
command -v runghc >/dev/null && runghc --version || true
command -v stack >/dev/null && stack --version || true
command -v cabal >/dev/null && cabal --version || trueRepository: fossas/fossa-cli
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether a Haskell parser/compiler is available to validate the syntax claim.
command -v ghc >/dev/null && ghc --version || true
command -v runghc >/dev/null && runghc --version || true
command -v stack >/dev/null && stack --version || true
command -v cabal >/dev/null && cabal --version || trueRepository: fossas/fossa-cli
Length of output: 154
🌐 Web query:
Haskell layout rule case alternative line break operator aligned with alternative parse error ">>=" after case alternative
💡 Result:
The parse errors you are encountering when using case expressions or operators like >>= across line breaks are due to Haskell's layout rule (or "off-side rule"), which dictates how indentation defines the structure of code [1]. When you omit explicit curly braces {} and semicolons ; in constructs like case, let, where, or do, Haskell uses the indentation of the first token following the keyword to determine the start of a layout block [1]. Key reasons for these errors include: 1. Insufficient Indentation: If a line within a case alternative or a do block is not indented strictly further than the start of the preceding expression, the parser assumes the current block has ended [2][3]. For example, if a case expression is inside a let block, the alternatives must be indented more than the start of the let binding; otherwise, the compiler may interpret the case alternatives as additional, incorrectly formatted bindings for the let statement [3]. 2. Misalignment of Alternatives: Alternatives within a case expression must be aligned at the same indentation level [2]. If an alternative is less indented, it terminates the case block; if it is indented inconsistently, it may be misparsed as a new, invalid entry in an enclosing block [3]. 3. Interaction with Operators: Operators like >>= (often used in do notation) require careful indentation management. If you break a line after an operator, the next line must be indented further than the start of the expression to be considered a continuation of that expression [2]. If it is not, the compiler may see it as the start of a new, separate statement, leading to a parse error [3]. To resolve these errors: * Always ensure that the body of a case or do block is indented more than the keyword that introduces it [3]. * When breaking lines in expressions (including those using >>=), ensure the continuation is indented further than the start of the expression [2]. * Use explicit braces {} and semicolons ; if you need to bypass layout rules or achieve specific formatting that the off-side rule does not naturally support [1][4]. * Check for "ghost" indentation issues by ensuring you are using spaces consistently, not mixing them with tabs [5]. For complex expressions, you can often work around these limitations by using parentheses to group terms, which can change how the parser interprets the indentation requirements [6].
Citations:
- 1: https://sources.debian.org/src/haskell98-report/20080907-9/report/layout.verb/
- 2: https://michaeldadams.org/papers/layout%5Fparsing%5F2/LayoutParsing2-2014-haskell-authors-copy.pdf
- 3: https://stackoverflow.com/questions/33004521/what-indentation-is-required-for-a-case-statement-within-a-let-statement
- 4: https://stackoverflow.com/questions/55278358/does-a-case-expression-in-a-let-expression-require-braces-and-semicolons
- 5: https://gitlab.haskell.org/ghc/ghc/-/issues/9581
- 6: https://stackoverflow.com/questions/40934464/why-is-it-a-syntax-error-to-linebreak-case-matches-without-extra-indentation-an
🌐 Web query:
Haskell layout rule case alternative line break operator aligned with alternative parse error ">>=" after case alternative
💡 Result:
In Haskell, the "layout rule" (or "off-side rule") automatically inserts braces and semicolons to define blocks of code based on indentation [1][2]. Parse errors related to "case" alternatives and line breaks almost always stem from incorrect indentation levels relative to the start of the layout block [3][4][5]. When you use keywords like "case... of", "let", "where", or "do", Haskell expects the subsequent items (e.g., the alternatives in a case expression) to be indented further to the right than the keyword itself or the surrounding context [1][2]. Key causes for these errors include: 1. Misaligned Alternatives: If a case alternative is indented at the same level or to the left of the start of the case expression (or its preceding let/do block), the compiler terminates the layout block prematurely [1][4][5]. This often leads to confusing parse errors, such as a "parse error on input '->'" or unexpected token errors, because the compiler believes the block has already ended [3][6][7][4]. 2. Let/Do Block Interference: When a "case" expression is nested inside a "let" or "do" block, its indentation must be strictly greater than the indentation of the "let" or "do" expression, or its components, to be parsed as part of that block [3][5]. If the indentation is insufficient, the compiler may incorrectly attempt to parse the "case" as a sibling statement or part of an outer scope, leading to syntax failures [3][5]. 3. Resolution: To fix these errors, ensure that all alternatives within a "case" expression are aligned at the same indentation level, and that this level is greater than the indentation of the "case" keyword itself [4][5]. Alternatively, you can use explicit braces and semicolons (e.g., case x of { A ->...; B ->... }), which completely bypasses the layout rule and is immune to these indentation-related parse errors [8][9][5]. If you are receiving a "parse error on input '>>='" or similar after a case alternative, it often indicates that the previous case branch was not properly terminated, causing the parser to see the next line as an extension of the previous expression rather than a new statement in the "do" or "let" block [6][10]. Ensuring proper indentation alignment is the standard solution [3][4].
Citations:
- 1: https://www.haskell.org/onlinereport/haskell2010/haskellch2.html
- 2: https://sources.debian.org/src/haskell98-report/20080907-9/report/layout.verb/
- 3: https://stackoverflow.com/questions/58905982/having-parse-error-on-input-in-a-case-of
- 4: https://stackoverflow.com/questions/31088057/parse-error-in-a-case-statement
- 5: https://stackoverflow.com/questions/55278358/does-a-case-expression-in-a-let-expression-require-braces-and-semicolons
- 6: https://gitlab.haskell.org/ghc/ghc/-/issues/9581
- 7: https://gitlab.haskell.org/ghc/ghc/-/issues/15705
- 8: https://www.haskell.org/onlinereport/haskell2010/haskellch10.html
- 9: https://www.haskell.org/onlinereport/syntax-iso.html
- 10: https://stackoverflow.com/questions/50489871/parse-error-on-input-with-case-analysis
🌐 Web query:
Haskell layout rule case alternative line break operator aligned with alternative parse error ">>=" after case alternative
💡 Result:
In Haskell, the parse error occurring after a case alternative—often manifesting as a failure on the arrow (->) or subsequent lines—is typically caused by the interaction of the layout rule (the "off-side" rule) with nested syntactic blocks [1][2]. When a case expression is nested inside another block, such as a let or do expression, the parser uses the indentation level of the first lexeme following the keyword (like case, let, or where) to define the boundaries of that block [1][3][2]. If the alternatives of a case expression are not indented sufficiently beyond the indentation level of the enclosing block, the layout rule may incorrectly interpret those alternatives as new, separate items belonging to the parent block rather than as part of the case expression itself [2][4][5]. For example, in the following code, if the case alternatives are at the same indentation level as the variable defined in the let block, the compiler attempts to parse them as additional bindings for that let block, leading to a parse error when it encounters the case syntax (such as ->) instead of a valid binding [2][4]: -- Incorrect: alternatives are not indented enough let x = case y of A -> 1 -- Misinterpreted as part of the let block B -> 2 To resolve this, the alternatives must be indented further to the right than the starting column of the expression they belong to [1][4]. -- Correct: alternatives indented beyond the let binding start let x = case y of A -> 1 B -> 2 The Haskell layout rule is technically specified in terms of inserting implicit curly braces and semicolons [6][7]. When the compiler encounters an "invalid" token at a point where a closing brace could legally be inserted, it invokes the parse-error rule to potentially close the current layout context [8][3]. If your indentation is ambiguous, the parser may prematurely close the case block or fail to open one, resulting in the observed parse errors [2][4]. Always ensure that content inside a case block is clearly indented relative to the keyword or the start of the expression to avoid these layout ambiguities [1][2].
Citations:
- 1: https://stackoverflow.com/questions/55278358/does-a-case-expression-in-a-let-expression-require-braces-and-semicolons
- 2: https://stackoverflow.com/questions/33004521/what-indentation-is-required-for-a-case-statement-within-a-let-statement
- 3: https://sources.debian.org/src/haskell98-report/20080907-9/report/layout.verb/
- 4: https://stackoverflow.com/questions/58905982/having-parse-error-on-input-in-a-case-of
- 5: https://stackoverflow.com/questions/35472760/haskells-lambda-case-doesnt-work-in-prelude-as-expected-parse-error
- 6: https://www.haskell.org/onlinereport/haskell2010/haskellch10.html
- 7: https://www.haskell.org/definition/haskell98-report.pdf
- 8: https://www.haskell.org/onlinereport/syntax-iso.html
Fix the case/>>= layout in parseAtKey.
>>= is aligned with the case alternatives instead of the Just txt branch, so this is a parse error. Fold the bind into the branch with do notation; the guard should become if.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Strategy/Node/Pnpm/V4_8.hs` around lines 35 - 46, The parseAtKey function
has a layout/parse issue because the >>= continuation is aligned with the case
alternatives instead of being part of the Just txt branch. Update parseAtKey in
the V4_8 strategy so the Nothing/Just handling is expressed cleanly, with the
slashRequired check using if instead of a guard where needed, and place the bind
inside the Just txt branch using do notation so the parser can read the function
correctly.
Source: Coding guidelines
1 participant
Overview
The PNPM lockfile support is hard to handle because instead of supporting concrete versions of the config we kind of have a bunch of conditionals supporting different aspects of it in an ad-hoc way. This was an attempt to split the Pnpm lock processor into separate pieces that are only active when relevant. I'm hoping that it will make it easier for us to do work on Pnpm and know that the work is more narrowly scoped to one lockfile format.
Acceptance criteria
The Pnpm lockfile code is easier to read/understand and likely to be easier to work with than what we have today.
Testing plan
I'm relying entirely on our test suite. I don't have a super deep knowledge of all the vagaries of different pnpm lockfile formats. I had a look at our tests and the coverage looks fairly decent though I don't know how or if it captures more esoteric features.
Risks
The main risk is I broke something and have not realized. The other is that this isn't really an improvement to anyone but me.
Metrics
References
There isn't a ticket for this. It's just something I've been doing in the background since I find this area of the CLI to be really hard to grok.
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.