In PSA output label warnings and errors as such (#1277)

Author: fsoikin

CHANGELOG.md

@@ -27,6 +27,8 @@ Other improvements:
   cached locally.
 - `spago publish` now allows to publish a package with some test (but only
   test!) dependencies not present in the registry.
+- errors and warnings are now explicitly labeled as "ERROR" and "WARNING" in
+  Spago build output.
 - always using forward slash as path separator in lockfile, regardless of the
   platform, so that the lockfile doesn't keep changing when team members run
   Spago on different platforms.

src/Spago/Psa/Printer.purs

@@ -5,9 +5,7 @@
 --   Copyright © Nathan Faubion
 --   https://opensource.org/license/mit/
 module Spago.Psa.Printer
-  ( renderWarning
-  , renderError
-  , renderStats
+  ( renderStats
   , renderVerboseStats
   , printDefaultOutputToErr
   , printJsonOutputToOut
@@ -50,11 +48,11 @@ printJsonOutputToOut output = do
 printDefaultOutputToErr :: PsaArgs -> Output -> Effect Unit
 printDefaultOutputToErr options output = do
   forWithIndex_ output.warnings \i warning -> do
-    Console.error $ printDoc (renderWarning lenWarnings (i + 1) warning)
+    Console.error $ printDoc (renderCitation Warn lenWarnings (i + 1) warning)
     Console.error ""
 
   forWithIndex_ output.errors \i error -> do
-    Console.error $ printDoc (renderError lenErrors (i + 1) error)
+    Console.error $ printDoc (renderCitation Err lenErrors (i + 1) error)
     Console.error ""
 
   Console.error $ printDoc (renderStats' output.stats)
@@ -70,17 +68,21 @@ printDefaultOutputToErr options output = do
     Core.CompactStats -> renderStats
     Core.VerboseStats -> renderVerboseStats
 
-renderWarning :: Int -> Int -> PsaAnnotedError -> D.Doc Ansi.GraphicsParam
-renderWarning = renderWrapper Ansi.BrightYellow
+data Citation = Warn | Err
 
-renderError :: Int -> Int -> PsaAnnotedError -> D.Doc Ansi.GraphicsParam
-renderError = renderWrapper Ansi.BrightRed
+citationColor :: Citation -> Ansi.Color
+citationColor Warn = Ansi.BrightYellow
+citationColor Err = Ansi.BrightRed
 
-renderWrapper :: Ansi.Color -> Int -> Int -> PsaAnnotedError -> D.Doc Ansi.GraphicsParam
-renderWrapper color total index { error, path, position, source, message } =
+citationLabel :: Citation -> String
+citationLabel Warn = "WARNING"
+citationLabel Err = "ERROR"
+
+renderCitation :: Citation -> Int -> Int -> PsaAnnotedError -> D.Doc Ansi.GraphicsParam
+renderCitation cit total index { error, path, position, source, message } =
   D.foldWithSeparator (D.break <> D.break)
     [ D.words $
-        [ renderStatus color total index error.errorCode
+        [ renderStatus cit total index error.errorCode
         , renderPath path error.moduleName <> foldMap renderPosition position
         ]
     , D.indent $ D.lines
@@ -92,9 +94,11 @@ renderWrapper color total index { error, path, position, source, message } =
 toLines :: String -> D.Doc Ansi.GraphicsParam
 toLines = D.lines <<< map D.text <<< Str.split (Str.Pattern "\n")
 
-renderStatus :: Ansi.Color -> Int -> Int -> String -> D.Doc Ansi.GraphicsParam
-renderStatus color total index code =
-  DA.foreground color $ D.text $ "[" <> show index <> "/" <> show total <> " " <> code <> "]"
+renderStatus :: Citation -> Int -> Int -> String -> D.Doc Ansi.GraphicsParam
+renderStatus cit total index code =
+  DA.foreground (citationColor cit)
+    $ D.text
+    $ "[" <> citationLabel cit <> " " <> show index <> "/" <> show total <> " " <> code <> "]"
 
 renderModuleName :: Maybe String -> D.Doc Ansi.GraphicsParam
 renderModuleName Nothing = DA.dim $ D.text "(unknown module)"

test-fixtures/build/1148-warnings-diff-errors/errors/expected-stderr.txt

@@ -0,0 +1,43 @@
+Reading Spago workspace configuration...
+
+✓ Selecting package to build: foo
+
+Downloading dependencies...
+Building...
+[1 of 2] Compiling Foo
+[2 of 2] Compiling Main
+[ERROR 1/2 TypesDoNotUnify] src/Foo.purs:4:5
+
+  4  x = "nope"
+         ^^^^^^
+
+  Could not match type
+    String
+  with type
+    Int
+  while checking that type String
+    is at least as general as type Int
+  while checking that expression "nope"
+    has type Int
+  in value declaration x
+
+[ERROR 2/2 TypesDoNotUnify] src/Main.purs:10:7
+
+  10    log 42
+            ^^
+
+  Could not match type
+    Int
+  with type
+    String
+  while checking that type Int
+    is at least as general as type String
+  while checking that expression 42
+    has type String
+  in value declaration main
+
+           Src   Lib   All
+Warnings     0     0     0
+Errors       2     0     2
+
+✘ Failed to build.

test-fixtures/build/1148-warnings-diff-errors/errors/spago.yaml

@@ -0,0 +1,10 @@
+package:
+  name: foo
+  dependencies:
+    - console
+    - effect
+    - prelude
+
+workspace:
+  packageSet:
+    registry: 41.5.0

test-fixtures/build/1148-warnings-diff-errors/errors/src/Foo.purs

@@ -0,0 +1,4 @@
+module Foo where
+
+x :: Int
+x = "nope"

test-fixtures/build/1148-warnings-diff-errors/errors/src/Main.purs

@@ -0,0 +1,11 @@
+module Main where
+
+import Prelude
+
+import Effect
+import Effect.Console (log)
+
+main :: Effect Unit
+main = do
+  log 42
+  pure unit

test-fixtures/build/1148-warnings-diff-errors/warnings/expected-stderr.txt

@@ -0,0 +1,43 @@
+Reading Spago workspace configuration...
+
+✓ Selecting package to build: foo
+
+Downloading dependencies...
+Building...
+[1 of 1] Compiling Main
+[WARNING 1/4 ImplicitImport] src/Main.purs:3:1
+
+  3  import Prelude
+     ^^^^^^^^^^^^^^
+
+  Module Prelude has unspecified imports, consider using the explicit form:
+    import Prelude (Unit, pure, unit)
+
+[WARNING 2/4 ImplicitImport] src/Main.purs:5:1
+
+  5  import Effect
+     ^^^^^^^^^^^^^
+
+  Module Effect has unspecified imports, consider using the explicit form:
+    import Effect (Effect)
+
+[WARNING 3/4 UnusedImport] src/Main.purs:6:1
+
+  6  import Effect.Console (log)
+     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+  The import of Effect.Console is redundant
+
+[WARNING 4/4 UnusedName] src/Main.purs:10:7
+
+  10    let unusedVar = 1
+            ^^^^^^^^^^^^^
+
+  Name unusedVar was introduced but not used.
+  in value declaration main
+
+           Src   Lib   All
+Warnings     4     0     4
+Errors       0     0     0
+
+✓ Build succeeded.

test-fixtures/build/1148-warnings-diff-errors/warnings/spago.yaml

@@ -0,0 +1,10 @@
+package:
+  name: foo
+  dependencies:
+    - console
+    - effect
+    - prelude
+
+workspace:
+  packageSet:
+    registry: 41.5.0

test-fixtures/build/1148-warnings-diff-errors/warnings/src/Main.purs

@@ -0,0 +1,11 @@
+module Main where
+
+import Prelude
+
+import Effect
+import Effect.Console (log)
+
+main :: Effect Unit
+main = do
+  let unusedVar = 1
+  pure unit

test/Prelude.purs

@@ -396,5 +396,11 @@ escapePathInErrMsg = case Process.platform of
 assertWarning :: forall m. MonadThrow Error m => Array String -> Boolean -> String -> m Unit
 assertWarning paths shouldHave stdErr  = do
   when (not $ Array.all (\exp -> shouldHave == (String.contains (Pattern exp) stdErr)) paths) do
-    Assert.fail $ "STDERR contained one or more texts:\n" <> show paths <> "\n\nStderr was:\n" <> stdErr
+    Assert.fail
+      $ "STDERR "
+      <> (if shouldHave then "did not contain" else "contained")
+      <> " one or more texts:\n"
+      <> show paths
+      <> "\n\nStderr was:\n"
+      <> stdErr
 

test/Spago/Build.purs

@@ -3,6 +3,7 @@ module Test.Spago.Build where
 import Test.Prelude
 
 import Data.Foldable (fold)
+import Data.String as String
 import Node.FS.Aff as FSA
 import Node.Path as Path
 import Node.Platform as Platform
@@ -211,6 +212,32 @@ spec = Spec.around withTempDir do
       spago [ "build" ] >>= shouldBeSuccessErr (fixture "build/migrate-config/migrated-output.txt")
       checkFixture "spago.yaml" (fixture "build/migrate-config/migrated-spago.yaml")
 
+    Spec.it "#1148: outputs errors and warnings after build" \{ spago, fixture } -> do
+      let
+        shouldBeSuccessErr' = checkOutputsWithPathSeparatorPatchErr isRight
+        shouldBeFailureErr' = checkOutputsWithPathSeparatorPatchErr isLeft
+
+        checkOutputsWithPathSeparatorPatchErr result expectedFixture =
+          checkOutputs'
+            { stdoutFile: Nothing
+            , stderrFile: Just $ fixture expectedFixture
+            , result
+            , sanitize:
+                String.trim
+                >>> String.replaceAll (String.Pattern $ "src\\") (String.Replacement "src/")
+                >>> String.replaceAll (String.Pattern $ "\r\n") (String.Replacement "\n")
+            }
+
+      FS.copyTree { src: fixture "build/1148-warnings-diff-errors", dst: "." }
+
+      liftEffect $ Process.chdir "errors"
+      spago [ "install" ] >>= shouldBeSuccess
+      spago [ "build" ] >>= shouldBeFailureErr' "build/1148-warnings-diff-errors/errors/expected-stderr.txt"
+
+      liftEffect $ Process.chdir "../warnings"
+      spago [ "install" ] >>= shouldBeSuccess
+      spago [ "build" ] >>= shouldBeSuccessErr' "build/1148-warnings-diff-errors/warnings/expected-stderr.txt"
+
     Pedantic.spec
 
     Monorepo.spec

test/Spago/Build/Monorepo.purs

@@ -142,8 +142,8 @@ spec = Spec.describe "monorepo" do
       FS.copyTree { src: fixture "monorepo/strict-true-uncensored-warnings", dst: "." }
       let
         errs =
-          [ "[1/2 UnusedName] " <> escapePathInErrMsg [ "package-b", "src", "Main.purs:6:13" ]
-          , "[2/2 UnusedName] " <> escapePathInErrMsg [ "package-b", "test", "Main.purs:6:13" ]
+          [ "[ERROR 1/2 UnusedName] " <> escapePathInErrMsg [ "package-b", "src", "Main.purs:6:13" ]
+          , "[ERROR 2/2 UnusedName] " <> escapePathInErrMsg [ "package-b", "test", "Main.purs:6:13" ]
           ]
         hasUnusedWarningError = assertWarning errs true
       spago [ "build" ] >>= check { stdout: mempty, stderr: hasUnusedWarningError, result: isLeft }

test/Spago/Test.purs

@@ -60,7 +60,7 @@ spec = Spec.around withTempDir do
       FS.mkdirp "subpackage/test"
       FS.writeTextFile "subpackage/src/Main.purs" (Init.srcMainTemplate "Subpackage.Main")
 
-      -- We write a file into the current working directory. 
+      -- We write a file into the current working directory.
       -- The subpackage test will read the given file without changing its directory
       -- and log its content as its output.
       let textFilePath = "foo.txt"
@@ -130,8 +130,8 @@ spec = Spec.around withTempDir do
       let
         exp =
           case Process.platform of
-            Just Platform.Win32 -> "[1/1 UnusedName] test\\Test\\Main.purs:10:5"
-            _ -> "[1/1 UnusedName] test/Test/Main.purs:10:5"
+            Just Platform.Win32 -> "[WARNING 1/1 UnusedName] test\\Test\\Main.purs:10:5"
+            _ -> "[WARNING 1/1 UnusedName] test/Test/Main.purs:10:5"
         hasUnusedNameWarningError stdErr = do
 
           unless (String.contains (String.Pattern exp) stdErr) do