[ANE-2852] pnpm v9 lockfile and dev deps

Changelog.md

@@ -1,5 +1,13 @@
 # FOSSA CLI Changelog
 
+## 3.16.5
+
+- PNPM: Fix pnpm v9 lockfile transitive devDependency classification. Dependencies of devDependencies were incorrectly reported as production dependencies in pnpm v9 projects. ([#1668](https://github.com/fossas/fossa-cli/pull/1668))
+
+## 3.16.4
+
+- Bugfix: revert caching changes as they caused a problem with missing libs on macos ([#1675](https://github.com/fossas/fossa-cli/pull/1675))
+
 ## 3.16.3
 
 - Elixir: Use `MIX_ENV=prod` for accurate production dependency resolution, with fallback to `--only prod` for projects lacking `config/prod.exs` ([#1662](https://github.com/fossas/fossa-cli/pull/1662))

src/Strategy/Node/Pnpm/PnpmLock.hs

@@ -10,6 +10,7 @@ where
 
 import Control.Applicative ((<|>))
 import Control.Effect.Diagnostics (Diagnostics, Has, context)
+import Control.Monad (when)
 import Data.Aeson (FromJSON (..), withObject)
 import Data.Aeson.Extra (TextLike (..))
 import Data.Aeson.KeyMap (toHashMapText)
@@ -30,17 +31,23 @@ import DepTypes (
   DepType (GitType, NodeJSType, URLType, UserType),
   Dependency (Dependency, dependencyType),
   VerConstraint (CEq),
+  hydrateDepEnvs,
+  insertEnvironment,
  )
-import Effect.Grapher (deep, direct, edge, evalGrapher, run)
+import Effect.Grapher (deep, direct, edge, label, run, withLabeling)
 import Effect.Logger (
   Logger,
   logWarn,
   pretty,
  )
 import Effect.ReadFS (ReadFS, readContentsYaml)
-import Graphing (Graphing, shrink)
+import Graphing (Graphing)
+import Graphing qualified
 import Path (Abs, File, Path)
 
+newtype PnpmLabel = PnpmEnv DepEnvironment
+  deriving (Eq, Ord)
+
 -- | Pnpm Lockfile
 --
 -- Pnpm lockfile (v5) (in yaml) has the following shape (irrelevant fields omitted):
@@ -300,16 +307,22 @@ analyze file = context "Analyzing Npm Lockfile (v3)" $ do
 
   context "Building dependency graph" $ pure $ buildGraph pnpmLockFile
 
+-- Build the dependency graph, labeling direct deps with their environment
+-- (prod/dev). hydrateDepEnvs then propagates those environments to all
+-- transitive successors.
 buildGraph :: PnpmLockfile -> Graphing Dependency
-buildGraph lockFile = withoutLocalPackages $
-  run . evalGrapher $ do
+buildGraph lockFile = withoutLocalPackages . hydrateDepEnvs $
+  run . withLabeling applyLabels $ do
     for_ (toList lockFile.importers) $ \(_, projectImporters) -> do
-      let allDirectDependencies =
-            toList (directDependencies projectImporters)
-              <> toList (directDevDependencies projectImporters)
+      for_ (Map.toList $ directDependencies projectImporters) $ \(depName, ProjectMapDepMetadata depVersion) ->
+        for_ (toResolvedDependency depName depVersion) $ \dep -> do
+          direct dep
+          when isV9 $ label dep (PnpmEnv EnvProduction)
 
-      for_ allDirectDependencies $ \(depName, (ProjectMapDepMetadata depVersion)) ->
-        maybe (pure ()) direct $ toResolvedDependency (isPnpm9Dev depName) depName depVersion
+      for_ (Map.toList $ directDevDependencies projectImporters) $ \(depName, ProjectMapDepMetadata depVersion) ->
+        for_ (toResolvedDependency depName depVersion) $ \dep -> do
+          direct dep
+          when isV9 $ label dep (PnpmEnv EnvDevelopment)
 
     -- Add edges and deep dependencies by iterating over all packages.
     --
@@ -338,14 +351,14 @@ buildGraph lockFile = withoutLocalPackages $
       let (depName, depVersion) = case getPkgNameVersion pkgKey of
             Nothing -> (pkgKey, Nothing)
             Just (name, version) -> (name, Just version)
-      let parentDep = toDependency (isPnpm9Dev depName) depName depVersion pkgMeta
+      let parentDep = toDependency depName depVersion pkgMeta
 
       -- It is ok, if this dependency was already graphed as direct
       -- @direct 1 <> deep 1 = direct 1@
       deep parentDep
 
       for_ deepDependencies $ \(deepName, deepVersion) -> do
-        maybe (pure ()) (edge parentDep) (toResolvedDependency (isPnpm9Dev deepName) deepName deepVersion)
+        maybe (pure ()) (edge parentDep) (toResolvedDependency deepName deepVersion)
   where
     getPkgNameVersion :: Text -> Maybe (Text, Text)
     getPkgNameVersion = case lockFileVersion lockFile of
@@ -418,8 +431,8 @@ buildGraph lockFile = withoutLocalPackages $
     --    e.g.
     --      file:../local-package
     --
-    toResolvedDependency :: Bool -> Text -> Text -> Maybe Dependency
-    toResolvedDependency devOverride depName depVersion = do
+    toResolvedDependency :: Text -> Text -> Maybe Dependency
+    toResolvedDependency depName depVersion = do
       -- Some versions of the lockfile remove the peer dep suffix.
       -- Others do not which is why it tries both.
       let strippedVersion = withoutPeerDepSuffix depVersion
@@ -431,25 +444,8 @@ buildGraph lockFile = withoutLocalPackages $
               <|> fmap (depVersion,) (Map.lookup (mkPkgKey depName depVersion) (packages lockFile))
       case (maybeNonRegistrySrcPackage, maybeRegistrySrcPackage) of
         (Nothing, Nothing) -> Nothing
-        (Just nonRegistryPkg, _) -> Just $ toDependency devOverride depName Nothing nonRegistryPkg
-        (Nothing, Just (version, registryPkg)) -> Just $ toDependency devOverride depName (Just version) registryPkg
-
-    -- PNPM lockfile 9 doesn't store information about dev deps directly on package data.
-    -- Use a project map to determine if a package should be marked dev.
-    isPnpm9ProjectDev :: Text -> ProjectMap -> Bool
-    isPnpm9ProjectDev name ProjectMap{directDevDependencies} = Map.member name directDevDependencies
-
-    isPnpm9ProjectDep :: Text -> ProjectMap -> Bool
-    isPnpm9ProjectDep name ProjectMap{directDependencies} = Map.member name directDependencies
-
-    -- Returns true if a dependency is either absent or a dev dependency in each workspace. If it is a
-    -- non-dev dependency in any workspace, then this function returns false. Also returns false if the
-    -- lockfile is not PNPM v9
-    isPnpm9Dev :: Text -> Bool
-    isPnpm9Dev depName =
-      (lockFile.lockFileVersion == PnpmLockV9)
-        && (any (isPnpm9ProjectDev depName) lockFile.importers)
-        && not (any (isPnpm9ProjectDep depName) lockFile.importers)
+        (Just nonRegistryPkg, _) -> Just $ toDependency depName Nothing nonRegistryPkg
+        (Nothing, Just (version, registryPkg)) -> Just $ toDependency depName (Just version) registryPkg
 
     -- Makes representative key if the package was
     -- resolved via registry resolver.
@@ -466,17 +462,17 @@ buildGraph lockFile = withoutLocalPackages $
       PnpmLockV678 _ -> "/" <> name <> "@" <> version
       PnpmLockV9 -> name <> "@" <> version
 
-    toDependency :: Bool -> Text -> Maybe Text -> PackageData -> Dependency
-    toDependency devOverride name maybeVersion (PackageData isDev _ (RegistryResolve _) _ _) =
-      toDep NodeJSType name (withoutPeerDepSuffix . withoutSymConstraint <$> maybeVersion) (isDev || devOverride)
-    toDependency devOverride _ _ (PackageData isDev _ (GitResolve (GitResolution url rev)) _ _) =
-      toDep GitType url (Just rev) (isDev || devOverride)
-    toDependency devOverride _ _ (PackageData isDev _ (TarballResolve (TarballResolution url)) _ _) =
-      toDep URLType url Nothing (isDev || devOverride)
-    toDependency devOverride _ _ (PackageData isDev (Just name) (DirectoryResolve _) _ _) =
-      toDep UserType name Nothing (isDev || devOverride)
-    toDependency devOverride name _ (PackageData isDev Nothing (DirectoryResolve _) _ _) =
-      toDep UserType name Nothing (isDev || devOverride)
+    toDependency :: Text -> Maybe Text -> PackageData -> Dependency
+    toDependency name maybeVersion (PackageData isDev _ (RegistryResolve _) _ _) =
+      toDep NodeJSType name (withoutPeerDepSuffix . withoutSymConstraint <$> maybeVersion) isDev
+    toDependency _ _ (PackageData isDev _ (GitResolve (GitResolution url rev)) _ _) =
+      toDep GitType url (Just rev) isDev
+    toDependency _ _ (PackageData isDev _ (TarballResolve (TarballResolution url)) _ _) =
+      toDep URLType url Nothing isDev
+    toDependency _ _ (PackageData isDev (Just name) (DirectoryResolve _) _ _) =
+      toDep UserType name Nothing isDev
+    toDependency name _ (PackageData isDev Nothing (DirectoryResolve _) _ _) =
+      toDep UserType name Nothing isDev
 
     -- Sometimes package versions include symlinked paths
     -- of sibling dependencies used for resolution.
@@ -489,8 +485,23 @@ buildGraph lockFile = withoutLocalPackages $
     toDep :: DepType -> Text -> Maybe Text -> Bool -> Dependency
     toDep depType name version isDev = Dependency depType name (CEq <$> version) mempty (toEnv isDev) mempty
 
+    -- For v9, environments start empty and are set later via labels
+    -- during graph construction, then propagated by hydrateDepEnvs.
+    -- For older versions, isDev from PackageData is reliable and
+    -- environments are set inline.
     toEnv :: Bool -> Set.Set DepEnvironment
-    toEnv isNotRequired = Set.singleton $ if isNotRequired then EnvDevelopment else EnvProduction
+    toEnv isNotRequired =
+      if isV9
+        then mempty
+        else Set.singleton (if isNotRequired then EnvDevelopment else EnvProduction)
+
+    isV9 :: Bool
+    isV9 = lockFile.lockFileVersion == PnpmLockV9
+
+    applyLabels :: Dependency -> Set.Set PnpmLabel -> Dependency
+    applyLabels = foldr applyLabel
+      where
+        applyLabel (PnpmEnv env) = insertEnvironment env
 
     withoutLocalPackages :: Graphing Dependency -> Graphing Dependency
     withoutLocalPackages = Graphing.shrink (\dep -> dependencyType dep /= UserType)

test/Pnpm/PnpmLockSpec.hs

@@ -16,6 +16,7 @@ import DepTypes (
   VerConstraint (CEq),
  )
 import GraphUtil (
+  expectDep,
   expectDirect,
   expectEdge,
  )
@@ -31,6 +32,19 @@ mkProdDep nameAtVersion = mkDep nameAtVersion (Just EnvProduction)
 mkDevDep :: Text -> Dependency
 mkDevDep nameAtVersion = mkDep nameAtVersion (Just EnvDevelopment)
 
+mkBothEnvDep :: Text -> Dependency
+mkBothEnvDep nameAtVersion = do
+  let nameAndVersionSplit = Text.splitOn "@" nameAtVersion
+      name = head nameAndVersionSplit
+      version = last nameAndVersionSplit
+  Dependency
+    NodeJSType
+    name
+    (CEq <$> Just version)
+    mempty
+    (Set.fromList [EnvProduction, EnvDevelopment])
+    mempty
+
 mkDep :: Text -> Maybe DepEnvironment -> Dependency
 mkDep nameAtVersion env = do
   let nameAndVersionSplit = Text.splitOn "@" nameAtVersion
@@ -110,9 +124,16 @@ spec = do
   -- With the advent of lockfile v9, pnpm now has its own pnpm-workspace.yaml file.
   let pnpmLockV9Workspace = currentDir </> $(mkRelFile "test/Pnpm/testdata/pnpm-9-workspace-project/pnpm-lock.yaml")
 
+  let pnpmLockV9SharedDep = currentDir </> $(mkRelFile "test/Pnpm/testdata/pnpm-9-shared-dep/pnpm-lock.yaml")
+  let pnpmLockV9LocalDep = currentDir </> $(mkRelFile "test/Pnpm/testdata/pnpm-9-local-dep/pnpm-lock.yaml")
+  let pnpmLockV9MultiVersion = currentDir </> $(mkRelFile "test/Pnpm/testdata/pnpm-9-multi-version/pnpm-lock.yaml")
+
   describe "works with v9 format" $ do
     checkGraph pnpmLockV9 pnpmLockV9GraphSpec
     describe "workspace" $ checkGraph pnpmLockV9Workspace pnpmLockV9GraphSpec
+    describe "shared deps" $ checkGraph pnpmLockV9SharedDep pnpmLockV9SharedDepSpec
+    describe "local dep env propagation" $ checkGraph pnpmLockV9LocalDep pnpmLockV9LocalDepSpec
+    describe "multi-version env labeling" $ checkGraph pnpmLockV9MultiVersion pnpmLockV9MultiVersionSpec
 
 pnpmLockGraphSpec :: Graphing Dependency -> Spec
 pnpmLockGraphSpec graph = do
@@ -407,5 +428,62 @@ pnpmLockV9GraphSpec graph = do
       -- ├── sax 1.4.4
       -- └── xmlbuilder 11.0.1
       hasEdge (mkProdDep "uri-js@4.4.1") (mkProdDep "punycode@2.3.1")
-      hasEdge (mkDevDep "xml2js@0.6.2") (mkProdDep "sax@1.4.4")
-      hasEdge (mkDevDep "xml2js@0.6.2") (mkProdDep "xmlbuilder@11.0.1")
+      hasEdge (mkDevDep "xml2js@0.6.2") (mkDevDep "sax@1.4.4")
+      hasEdge (mkDevDep "xml2js@0.6.2") (mkDevDep "xmlbuilder@11.0.1")
+
+pnpmLockV9SharedDepSpec :: Graphing Dependency -> Spec
+pnpmLockV9SharedDepSpec graph = do
+  let hasEdge :: Dependency -> Dependency -> Expectation
+      hasEdge = expectEdge graph
+
+  describe "buildGraph with shared deps" $ do
+    it "should mark direct dependencies of project as direct" $ do
+      expectDirect
+        [ mkProdDep "uri-js@4.4.1"
+        , mkDevDep "xml2js@0.6.2"
+        ]
+        graph
+
+    it "should build edges with correct environments" $ do
+      -- sax is reachable from both prod (uri-js) and dev (xml2js) -- gets both
+      hasEdge (mkProdDep "uri-js@4.4.1") (mkBothEnvDep "sax@1.4.4")
+      hasEdge (mkDevDep "xml2js@0.6.2") (mkBothEnvDep "sax@1.4.4")
+      -- punycode is prod-only (via uri-js)
+      hasEdge (mkProdDep "uri-js@4.4.1") (mkProdDep "punycode@2.3.1")
+      -- xmlbuilder is dev-only (via xml2js)
+      hasEdge (mkDevDep "xml2js@0.6.2") (mkDevDep "xmlbuilder@11.0.1")
+
+-- Bug 2 regression: transitive deps of a local (file:) package must inherit
+-- the environment from the importer that depends on it, even though the
+-- local package node itself is removed from the final graph.
+pnpmLockV9LocalDepSpec :: Graphing Dependency -> Spec
+pnpmLockV9LocalDepSpec graph = do
+  let hasEdge :: Dependency -> Dependency -> Expectation
+      hasEdge = expectEdge graph
+
+  describe "buildGraph with local dep" $ do
+    it "should not include the local package in the final graph" $ do
+      -- local-pkg is UserType and should be removed by withoutLocalPackages
+      expectDirect
+        [ mkProdDep "express@4.18.2"
+        ]
+        graph
+
+    it "should propagate prod environment through local package to transitive deps" $ do
+      -- express is a transitive dep of local-pkg (prod) — must be prod
+      expectDep (mkProdDep "express@4.18.2") graph
+      -- body-parser is a transitive dep of express — must also be prod
+      expectDep (mkProdDep "body-parser@1.20.1") graph
+      hasEdge (mkProdDep "express@4.18.2") (mkProdDep "body-parser@1.20.1")
+
+-- Bug 3 regression: when two workspace packages depend on different versions
+-- of the same package (one prod, one dev), each version must receive only its
+-- own environment label — not both.
+pnpmLockV9MultiVersionSpec :: Graphing Dependency -> Spec
+pnpmLockV9MultiVersionSpec graph = do
+  describe "buildGraph with multi-version deps" $ do
+    it "should label each version with only its own environment" $ do
+      -- sax@1.2.1 is prod-only (from app-a)
+      expectDep (mkProdDep "sax@1.2.1") graph
+      -- sax@1.4.4 is dev-only (from app-b)
+      expectDep (mkDevDep "sax@1.4.4") graph