Remove src deps from test deps on --pedantic-packages report

src/Spago/Purs/Graph.purs

@@ -183,68 +183,86 @@ checkImports graph = do
   packageGraph <- runSpago (Record.union { selected: NEA.singleton selected } env) $ getModuleGraphWithPackage graph
 
   let
-    packageName = selected.package.name
-    testPackageName = unsafeCoerce (PackageName.print packageName <> ":test")
-
-    declaredDependencies = unwrap selected.package.dependencies
-    declaredTestDependencies = maybe Map.empty (unwrap <<< _.dependencies) selected.package.test
-
-    -- The direct dependencies specified in the config
-    dependencyPackages = Map.mapMaybe (const (Just Map.empty)) declaredDependencies
-    dependencyTestPackages = Map.mapMaybe (const (Just Map.empty)) $ Map.union declaredDependencies declaredTestDependencies
-
-  -- Compile the globs for the project, we get the set of source files in the project
-  projectGlob :: Set FilePath <- map Set.fromFoldable do
-    map Array.fold $ traverse compileGlob (Config.sourceGlob NoTestGlobs packageName (WorkspacePackage selected))
-
-  -- Same but for tests
-  projectTestsGlob :: Set FilePath <- map Set.fromFoldable do
-    map Array.fold $ traverse compileGlob (Config.sourceGlob OnlyTestGlobs packageName (WorkspacePackage selected))
-
-  let
-    -- Filter this improved graph to only have the project modules
-    projectGraph = Map.filterWithKey (\_ { path } -> Set.member path projectGlob) packageGraph
-    projectTestsGraph = Map.filterWithKey (\_ { path } -> Set.member path projectTestsGlob) packageGraph
-
-    -- Go through all the modules in the project graph, figure out which packages each module depends on,
-    -- accumulate all of that in a single place
-    accumulateImported importedPkgs' (Tuple moduleName { depends }) =
-      let
-        accumulateDep importedPkgs importedModule = case Map.lookup importedModule packageGraph of
-          Nothing -> importedPkgs
-          -- Skip dependencies on modules in the same package, we are not interested in that
-          Just { package } | package == packageName -> importedPkgs
-          Just { package } | package == testPackageName -> importedPkgs
-          Just { package: importedPackage } -> Map.alter
-            ( case _ of
-                Nothing -> Just $ Map.singleton moduleName (Set.singleton importedModule)
-                Just p -> Just $ Map.alter
-                  ( case _ of
-                      Nothing -> Just $ Set.singleton importedModule
-                      Just set -> Just $ Set.insert importedModule set
-                  )
-                  moduleName
-                  p
-            )
-            importedPackage
-            importedPkgs
-      in
-        foldl accumulateDep importedPkgs' depends
-
-    importedPackages :: ImportedPackages
-    importedPackages = foldl accumulateImported Map.empty (Map.toUnfoldable projectGraph :: Array _)
-    importedTestPackages = foldl accumulateImported Map.empty (Map.toUnfoldable projectTestsGraph :: Array _)
-
-    unused = Map.keys $ Map.difference dependencyPackages importedPackages
-    transitive = Map.difference importedPackages dependencyPackages
-    unusedTest =
-      if Set.isEmpty projectTestsGlob then Set.empty
-      else Map.keys $ Map.difference (Map.difference dependencyTestPackages dependencyPackages) importedTestPackages
-    transitiveTest =
-      if Set.isEmpty projectTestsGlob then Map.empty
-      else Map.difference importedTestPackages dependencyTestPackages
-
-  pure { unused, transitive, unusedTest, transitiveTest }
+    dropValues = Map.mapMaybe (const (Just Map.empty))
+    srcDeps = unwrap selected.package.dependencies
+  srcResult <- getUsedUnusedTransitiveFor
+    { selected
+    , packageName: selected.package.name
+    , dependencyPackages: dropValues srcDeps
+    , isSrc: true
+    , packageGraph
+    }
+  let srcDepsUsed = Map.filterKeys (flip Map.member srcResult.used) srcDeps
+  testResult <- getUsedUnusedTransitiveFor
+    { selected
+    , packageName: selected.package.name
+    , dependencyPackages:
+        dropValues
+          -- add the used source dependencies, not the declared ones.
+          $ Map.unionWith const srcDepsUsed
+          -- get test deps
+          $ maybe Map.empty (unwrap <<< _.dependencies) selected.package.test
+    , isSrc: false
+    , packageGraph
+    }
+
+  pure
+    { unused: srcResult.unused
+    , transitive: srcResult.transitive
+    , unusedTest: Set.difference testResult.unused $ Map.keys srcResult.used
+    , transitiveTest: differenceAll testResult.transitive [ srcResult.used, srcResult.transitive ]
+    }
+  where
+  differenceAll sourceMap removalsArray = foldl Map.difference sourceMap removalsArray
+  getUsedUnusedTransitiveFor { selected, packageName, dependencyPackages, isSrc, packageGraph } = do
+    let
+      testPackageName = unsafeCoerce (PackageName.print packageName <> ":test")
+
+      testGlobOption
+        | isSrc = NoTestGlobs
+        | otherwise = OnlyTestGlobs
+
+    -- Compile the globs for the project, we get the set of source files in the project
+    glob :: Set FilePath <- map Set.fromFoldable do
+      map Array.fold $ traverse compileGlob (Config.sourceGlob testGlobOption packageName (WorkspacePackage selected))
+
+    let
+      -- Filter this improved graph to only have the project modules
+      projectGraph = Map.filterWithKey (\_ { path } -> Set.member path glob) packageGraph
+
+      -- Go through all the modules in the project graph, figure out which packages each module depends on,
+      -- accumulate all of that in a single place
+      accumulateImported importedPkgs' (Tuple moduleName { depends }) =
+        let
+          accumulateDep importedPkgs importedModule = case Map.lookup importedModule packageGraph of
+            Nothing -> importedPkgs
+            -- Skip dependencies on modules in the same package, we are not interested in that
+            Just { package } | package == packageName -> importedPkgs
+            Just { package } | package == testPackageName -> importedPkgs
+            Just { package: importedPackage } -> Map.alter
+              ( case _ of
+                  Nothing -> Just $ Map.singleton moduleName (Set.singleton importedModule)
+                  Just p -> Just $ Map.alter
+                    ( case _ of
+                        Nothing -> Just $ Set.singleton importedModule
+                        Just set -> Just $ Set.insert importedModule set
+                    )
+                    moduleName
+                    p
+              )
+              importedPackage
+              importedPkgs
+        in
+          foldl accumulateDep importedPkgs' depends
+
+      importedPackages :: ImportedPackages
+      importedPackages = foldl accumulateImported Map.empty (Map.toUnfoldable projectGraph :: Array _)
+
+    pure
+      { used: if isSrc then Map.intersection dependencyPackages importedPackages else Map.empty
+      , unused: Map.keys $ Map.difference dependencyPackages importedPackages
+      , transitive: Map.difference importedPackages dependencyPackages
+      }
 
 --------------------------------------------------------------------------------
 -- Errors

test-fixtures/check-pedantic-packages.txt

@@ -0,0 +1,41 @@
+Reading Spago workspace configuration...
+Read the package set from the registry
+
+✅ Selecting package to build: foo
+
+Downloading dependencies...
+Building...
+           Src   Lib   All
+Warnings     0     0     0
+Errors       0     0     0
+
+✅ Build succeeded.
+
+Looking for unused and undeclared transitive dependencies...
+
+❌ Sources for package 'foo' declares unused dependencies - please remove them from the project config:
+  - either
+
+
+❌ Sources for package 'foo' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:
+  newtype
+    from `Main`, which imports:
+      Data.Newtype
+
+
+Run the following command to install them all:
+  spago install -p foo newtype
+
+
+❌ Tests for package 'foo' declares unused dependencies - please remove them from the project config:
+  - tuples
+
+
+❌ Tests for package 'foo' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:
+  either
+    from `Test.Main`, which imports:
+      Data.Either
+
+
+Run the following command to install them all:
+  spago install --test-deps -p foo either

test/Spago/Build.purs

@@ -2,6 +2,8 @@ module Test.Spago.Build where
 
 import Test.Prelude
 
+import Data.Array as Array
+import Data.Map as Map
 import Node.FS.Aff as FSA
 import Node.Path as Path
 import Node.Process as Process
@@ -102,36 +104,40 @@ spec = Spec.around withTempDir do
 
       Spec.describe "fails when there are imports from transitive dependencies and" do
         let
-          setupSrcTransitiveTests spago = do
+          setupSrcTransitiveTests spago installConsoleAndEffect = do
             spago [ "init", "--name", "7368613235362d34312f4e59746b7869335477336d33414d72" ] >>= shouldBeSuccess
             spago [ "install", "maybe" ] >>= shouldBeSuccess
+            when installConsoleAndEffect do
+              spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess
             FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nimport Data.Maybe\nimport Control.Alt\nmain = unit"
             -- get rid of "Compiling ..." messages and other compiler warnings
             spago [ "build" ] >>= shouldBeSuccess
 
         Spec.it "passed --pedantic-packages CLI flag" \{ spago, fixture } -> do
-          setupSrcTransitiveTests spago
+          setupSrcTransitiveTests spago true
           spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt")
 
         Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do
-          setupSrcTransitiveTests spago
+          setupSrcTransitiveTests spago false
           addPedanticPackagesToSrc
           spago [ "build" ] >>= shouldBeFailureErr (fixture "check-direct-import-transitive-dependency.txt")
 
       Spec.describe "warns about unused dependencies when" do
         let
-          setupSrcUnusedDeps spago = do
+          setupSrcUnusedDeps spago installConsoleAndEffect = do
             spago [ "init", "--name", "7368613235362d2f444a2b4f56375435646a59726b53586548" ] >>= shouldBeSuccess
+            when installConsoleAndEffect do
+              spago [ "install", "--test-deps", "console", "effect" ] >>= shouldBeSuccess
             FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) "module Main where\nimport Prelude\nmain = unit"
             -- get rid of "Compiling ..." messages and other compiler warnings
             spago [ "build" ] >>= shouldBeSuccess
 
         Spec.it "passing --pedantic-packages CLI flag" \{ spago, fixture } -> do
-          setupSrcUnusedDeps spago
+          setupSrcUnusedDeps spago true
           spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt")
 
         Spec.it "package config has 'pedantic_packages: true'" \{ spago, fixture } -> do
-          setupSrcUnusedDeps spago
+          setupSrcUnusedDeps spago false
           addPedanticPackagesToSrc
           spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-dependency.txt")
 
@@ -159,6 +165,86 @@ spec = Spec.around withTempDir do
           modifyPkgTestConfig true
           spago [ "build" ] >>= shouldBeFailureErr (fixture "check-unused-test-dependency.txt")
 
+      {-
+      Goal:
+        Src
+          - remove either - unused
+          - install newtype - (transitive dep of either) as it will be needed once either is removed
+        Test
+          - remove tuples - unused
+          - install either - (implicit dep of source package, but need to add it here once removed from source)
+      -}
+      Spec.describe "fails to build and reports deduplicated src and test unused/transitive dependenciess when" do
+        let
+          addPedanticPackages = do
+            modifyPackageConfig \config ->
+              config
+                { package = config.package <#> \r -> r
+                    { build = Just
+                        { pedantic_packages: Just true
+                        , strict: r.build >>= _.strict
+                        , censor_project_warnings: r.build >>= _.censor_project_warnings
+                        }
+                    , test = Just
+                        { main: "Test.Main"
+                        , pedantic_packages: Just true
+                        , strict: r.test >>= _.strict
+                        , censor_test_warnings: r.test >>= _.censor_test_warnings
+                        , dependencies: maybe (Config.Dependencies Map.empty) _.dependencies r.test
+                        , execArgs: r.test >>= _.execArgs
+                        }
+                    }
+                }
+
+          setupUnusedAndTransitiveDeps spago = do
+            -- Since we need to edit `spago.yaml` and other files generated by `spago init`,
+            -- may as well just create those files by hand.
+            FS.writeYamlFile configCodec "spago.yaml" $ mkPackageAndWorkspaceConfig
+              { package: { packageName: "foo", srcDependencies: [ "prelude", "control", "either" ] }
+              , workspace: { setVersion: Just $ mkVersion "0.0.1" }
+              }
+              [ configAddTestMain
+              , configAddTestDependencies [ "tuples" ]
+              ]
+
+            FS.mkdirp "src"
+            FS.writeTextFile (Path.concat [ "src", "Main.purs" ]) $ Array.intercalate "\n"
+              [ "module Main where "
+              , ""
+              , "import Prelude"
+              , "import Data.Newtype as Newtype"
+              , "import Control.Alt as Alt"
+              , ""
+              , "foo :: Int"
+              , "foo = 1"
+              ]
+
+            FS.mkdirp $ Path.concat [ "test", "Test" ]
+            FS.writeTextFile (Path.concat [ "test", "Test", "Main.purs" ]) $ Array.intercalate "\n"
+              [ "module Test.Main where "
+              , ""
+              , "import Prelude"
+              , "import Data.Newtype (class Newtype)"
+              , "import Data.Either (Either(..))"
+              , ""
+              , "newtype Bar = Bar Int"
+              , "derive instance Newtype Bar _"
+              , ""
+              , "foo :: Either Bar Int"
+              , "foo = Right 1"
+              ]
+            -- get rid of "Compiling ..." messages and other compiler warnings
+            spago [ "build" ] >>= shouldBeSuccess
+
+        Spec.it "passing --pedantic-packages" \{ spago, fixture } -> do
+          setupUnusedAndTransitiveDeps spago
+          spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt")
+
+        Spec.it "package config has `pedantic_packages: true` in both `build` and `test`" \{ spago, fixture } -> do
+          setupUnusedAndTransitiveDeps spago
+          addPedanticPackages
+          spago [ "build" ] >>= shouldBeFailureErr (fixture "check-pedantic-packages.txt")
+
     Spec.it "--strict causes build to fail if there are warnings" \{ spago, fixture } -> do
       spago [ "init" ] >>= shouldBeSuccess
       let srcMain = Path.concat [ "src", "Main.purs" ]

test/Spago/Build/Polyrepo.purs

@@ -364,7 +364,7 @@ spec = Spec.describe "polyrepo" do
             { modName: mkTestModuleName packageName
             , imports:
                 [ "import Control.Alt as Control"
-                , "import Data.Newtype as Newtype"
+                , "import Safe.Coerce as Coerce"
                 ]
             , body:
                 [ ""
@@ -383,17 +383,21 @@ spec = Spec.describe "polyrepo" do
           [ toMsgPrefix isSrc <> " for package '" <> package <> "' declares unused dependencies - please remove them from the project config:"
           , "  - " <> (if isSrc then "tuples" else "either")
           ]
-      mkTransitiveDepErr isSrc package =
+      mkTransitiveDepErr isSrc package = do
+        let
+          { pkg, mkModName, pkgModName } =
+            if isSrc then { pkg: "newtype", mkModName: mkSrcModuleName, pkgModName: "Data.Newtype" }
+            else { pkg: "safe-coerce", mkModName: mkTestModuleName, pkgModName: "Safe.Coerce" }
         Array.intercalate "\n"
           [ Array.fold
               [ toMsgPrefix isSrc
               , " for package '"
               , package
               , "' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:"
               ]
-          , "  newtype"
-          , "    from `" <> (if isSrc then mkSrcModuleName else mkTestModuleName) package <> "`, which imports:"
-          , "      Data.Newtype"
+          , "  " <> pkg
+          , "    from `" <> mkModName package <> "`, which imports:"
+          , "      " <> pkgModName
           ]
     Spec.it "when package config has 'pedantic_packages: true', build fails with expected errors" \{ spago } -> do
       writeWorkspaceOnlySpagoYamlFile