Merge pull request #1110 from alexarchambault/bsp-processing-errors

Report wrong import $file-s via diagnostics in BSP

amm/interp/src/main/scala/ammonite/interp/Parsers.scala

@@ -60,7 +60,7 @@ object Parsers {
   // After each statement, there must either be `Semis`, a "}" marking the
   // end of the block, or the `End` of the input
   def StatementBlock[_: P](blockSep: => P0) =
-    P( Semis.? ~ (!blockSep ~ TmplStat ~~ WS ~~ (Semis | &("}") | End)).!.repX)
+    P( Semis.? ~ (Index ~ (!blockSep ~ TmplStat ~~ WS ~~ (Semis | &("}") | End)).!).repX)
 
   def Splitter0[_: P] = P( StatementBlock(Fail) )
   def Splitter[_: P] = P( ("{" ~ Splitter0 ~ "}" | Splitter0) ~ End )
@@ -85,7 +85,8 @@ object Parsers {
 
     parse(code, Splitter(_), instrument = instrument) match{
       case Parsed.Failure(_, index, extra) if furthest == code.length => None
-      case x => Some(x)
+      case f @ Parsed.Failure(_, _, _) => Some(f)
+      case Parsed.Success(value, index) => Some(Parsed.Success(value.map(_._2), index))
     }
   }
 
@@ -110,10 +111,12 @@ object Parsers {
       case f: Parsed.Failure => stringWrap(s)
     }
   }
-  def parseImportHooks(source: CodeSource, stmts: Seq[String]) = synchronized{
+  def parseImportHooks(source: CodeSource, stmts: Seq[String]) =
+    parseImportHooksWithIndices(source, stmts.map((0, _)))
+  def parseImportHooksWithIndices(source: CodeSource, stmts: Seq[(Int, String)]) = synchronized{
     val hookedStmts = mutable.Buffer.empty[String]
     val importTrees = mutable.Buffer.empty[ImportTree]
-    for(stmt <- stmts) {
+    for((startIdx, stmt) <- stmts) {
       // Call `fastparse.ParserInput.fromString` explicitly, to avoid generating a
       // lambda in the class body and making the we-do-not-load-fastparse-on-cached-scripts
       // test fail
@@ -127,7 +130,11 @@ object Parsers {
               currentStmt = currentStmt.patch(
                 importTree.start, (importTree.prefix(0) + ".$").padTo(length, ' '), length
               )
-              importTrees.append(importTree)
+              val importTree0 = importTree.copy(
+                start = startIdx + importTree.start,
+                end = startIdx + importTree.end
+              )
+              importTrees.append(importTree0)
             }
           }
           hookedStmts.append(currentStmt)

amm/interp/src/main/scala/ammonite/interp/Preprocessor.scala

@@ -74,13 +74,14 @@ object Preprocessor{
       case f: Parsed.Failure =>
         Left(formatFastparseError(fileName, rawCode, f))
 
-      case s: Parsed.Success[Seq[(String, Seq[String])]] =>
+      case s: Parsed.Success[Seq[(String, Seq[(Int, String)])]] =>
 
         var offset = 0
         val blocks = mutable.ArrayBuffer[(String, Seq[String])]()
 
         // comment holds comments or empty lines above the code which is not caught along with code
-        for( (comment, code) <- s.value){
+        for( (comment, codeWithStartIdx) <- s.value){
+          val code = codeWithStartIdx.map(_._2)
 
           //ncomment has required number of newLines appended based on OS and offset
           //since fastparse has hardcoded `\n`s, while parsing strings with `\r\n`s it
@@ -107,10 +108,10 @@ object Preprocessor{
   def splitScriptWithStart(
     rawCode: String,
     fileName: String
-  ): Either[String, IndexedSeq[(Int, String, Seq[String])]] = {
+  ): Either[Parsed.Failure, IndexedSeq[(Int, String, Seq[(Int, String)])]] = {
     Parsers.splitScriptWithStart(rawCode) match {
       case f: Parsed.Failure =>
-        Left(formatFastparseError(fileName, rawCode, f))
+        Left(f)
 
       case Parsed.Success(value, _) =>
         val blocks = value.toVector.map {

amm/interp/src/main/scala/ammonite/interp/script/AmmoniteBuildServer.scala

@@ -269,22 +269,24 @@ class AmmoniteBuildServer(
     diagnostics: Seq[Diagnostic]
   ): Unit = {
 
-    val bspDiagnostics = diagnostics.map {
-      case Diagnostic(severity, start, end, msg) =>
-        val start0 = new BPosition(start.line, start.char)
-        val end0 = new BPosition(end.line, end.char)
-        val diagnostic = new BDiagnostic(new Range(start0, end0), msg)
-        diagnostic.setSeverity(
-          severity match {
-            case "INFO" => DiagnosticSeverity.INFORMATION
-            case "WARNING" => DiagnosticSeverity.WARNING
-            case "ERROR" => DiagnosticSeverity.ERROR
-            case _ => sys.error(s"Unrecognized severity: $severity")
-          }
-        )
-        diagnostic
+    def bspDiagnostic(diagnostic: Diagnostic): BDiagnostic = {
+      val start0 = new BPosition(diagnostic.start.line, diagnostic.start.char)
+      val end0 = new BPosition(diagnostic.end.line, diagnostic.end.char)
+      val bspDiagnostic = new BDiagnostic(new Range(start0, end0), diagnostic.message)
+      bspDiagnostic.setSeverity(
+        diagnostic.severity match {
+          case "INFO" => DiagnosticSeverity.INFORMATION
+          case "WARNING" => DiagnosticSeverity.WARNING
+          case "ERROR" => DiagnosticSeverity.ERROR
+          case _ => sys.error(s"Unrecognized severity: ${diagnostic.severity}")
+        }
+      )
+      bspDiagnostic
     }
 
+    val processorDiagnostics = mod0.processorDiagnostics.map(bspDiagnostic)
+    val compileDiagnostics = diagnostics.map(bspDiagnostic)
+
     val diagnosticsParams = new PublishDiagnosticsParams(
       new TextDocumentIdentifier(
         mod0
@@ -295,7 +297,7 @@ class AmmoniteBuildServer(
           .toASCIIString
       ),
       target,
-      bspDiagnostics.asJava,
+      (processorDiagnostics ++ compileDiagnostics).asJava,
       true // ???
     )
 
@@ -384,14 +386,12 @@ class AmmoniteBuildServer(
   }
 
   private def compileScript(script: Script, target: BuildTargetIdentifier): Boolean = {
-
     val (_, res) = compiler.compile(
       script,
       proc,
       doCompile = compileScript(_, _)
     )
-
-    res.isRight
+    script.processorDiagnostics.isEmpty && res.isRight
   }
 
   def buildTargetCompile(params: CompileParams): CompletableFuture[CompileResult] =

amm/interp/src/main/scala/ammonite/interp/script/Script.scala

@@ -8,7 +8,8 @@ import ammonite.util.Name
 final case class Script(
   code: String,
   codeSource: CodeSource,
-  blocks: Seq[Script.Block]
+  blocks: Seq[Script.Block],
+  processorDiagnostics: Seq[Diagnostic]
 ) {
 
   lazy val dependencyImports: Imports = {

amm/interp/src/main/scala/ammonite/interp/script/ScriptCache.scala

@@ -43,13 +43,8 @@ final class ScriptCache(
       .filter(os.isFile(_))
       .flatMap { p =>
         // FIXME Blocking
-        proc.load(p) match {
-          case Left(err) =>
-            // TODO Log error
-            Nil
-          case Right(script) =>
-            Seq(script)
-        }
+        val script = proc.load(p)
+        Seq(script)
       }
 
     val dependencies = scripts0

amm/interp/src/main/scala/ammonite/interp/script/ScriptProcessor.scala

@@ -24,58 +24,91 @@ final case class ScriptProcessor(
   def load(
     code: String,
     codeSource: CodeSource
-  ): Either[String, Script] = {
+  ): Script = {
 
+    val rawCode = Interpreter.skipSheBangLine(code)
+    lazy val offsetToPos = PositionOffsetConversion.offsetToPos(rawCode)
     val splittedScript = Preprocessor.splitScriptWithStart(
-      Interpreter.skipSheBangLine(code),
+      rawCode,
       codeSource.fileName
-    )
+    ).left.map { f =>
+      val startPos = offsetToPos(f.index).copy(char = 0)
+      val endPos = {
+        val endIdx = rawCode.indexOf('\n', f.index)
+        val actualEndIdx = if (endIdx < 0) rawCode.length else endIdx
+        offsetToPos(actualEndIdx)
+      }
+      val expected = f.trace().failure.label
+      Seq(Diagnostic("ERROR", startPos, endPos, s"Expected $expected"))
+    }
 
-    def hookFor(tree: ImportTree) = {
+    def hookFor(tree: ImportTree): Either[Diagnostic, (Seq[String], ImportHook)] = {
       val hookOpt = importHooks.find { case (k, v) => tree.strippedPrefix.startsWith(k) }
-      hookOpt match {
-        case None => Left(s"Invalid import hook '${tree.strippedPrefix.mkString(".")}'")
-        case Some(hook) => Right((tree, hook))
+      hookOpt.toRight {
+        Diagnostic(
+          "ERROR",
+          offsetToPos(tree.start),
+          offsetToPos(tree.end),
+          s"Invalid import hook '${tree.strippedPrefix.mkString(".")}'"
+        )
       }
     }
 
-    def hookResults(hookPrefix: Seq[String], hook: ImportHook, tree: ImportTree) =
-      hook.handle(
+    def hookResults(
+      hookPrefix: Seq[String],
+      hook: ImportHook,
+      tree: ImportTree
+    ): Either[Diagnostic, Seq[ImportHook.Result]] = {
+      val r = hook.handle(
         codeSource,
         tree.copy(prefix = tree.prefix.drop(hookPrefix.length)),
         ScriptProcessor.dummyInterpreterInterface,
         codeWrapper.wrapperPath
       )
+      r.left.map { error =>
+        Diagnostic(
+          "ERROR",
+          offsetToPos(tree.start),
+          offsetToPos(tree.end),
+          error
+        )
+      }
+    }
 
-    for {
-      elems <- splittedScript
-      withImportHooks <- elems
-        .traverse {
-          case (startIdx, leadingSpaces, statements) =>
-            val (statements0, importTrees) = Parsers.parseImportHooks(codeSource, statements)
-            importTrees.traverse(hookFor).map((startIdx, leadingSpaces, statements0, _))
-        }
-        .left.map(_.mkString(", "))
-      r <-  withImportHooks
-        .traverse {
-          case (startIdx, leadingSpaces, statements0, imports) =>
-            imports
-              .traverse {
-                case (tree, (hookPrefix, hook)) =>
-                  hookResults(hookPrefix, hook, tree)
-              }
-              .map(l => Script.Block(startIdx, leadingSpaces, statements0, l.flatten))
-        }
-        .left.map(_.mkString(", "))
-    } yield Script(code, codeSource, r)
+    val diagnostics = new mutable.ListBuffer[Diagnostic]
+
+    for (l <- splittedScript.left)
+      diagnostics ++= l
+
+    val blocks = for {
+      elems <- splittedScript.right.toSeq
+      (startIdx, leadingSpaces, statements) <- elems
+    } yield {
+      val (statements0, importTrees) = Parsers.parseImportHooksWithIndices(codeSource, statements)
+      val importResults =
+        for {
+          tree <- importTrees
+          (hookPrefix, hook) <- hookFor(tree) match {
+            case Left(diag) => diagnostics += diag; Nil
+            case Right(imports0) => Seq(imports0)
+          }
+          res <- hookResults(hookPrefix, hook, tree) match {
+            case Left(diag) => diagnostics += diag; Nil
+            case Right(res) => res
+          }
+        } yield res
+      Script.Block(startIdx, leadingSpaces, statements0, importResults)
+    }
+
+    Script(code, codeSource, blocks, diagnostics.toVector)
   }
 
-  def load(path: os.Path, codeSource: CodeSource): Either[String, Script] = {
+  def load(path: os.Path, codeSource: CodeSource): Script = {
     val code = os.read(path)
     load(code, codeSource)
   }
 
-  def load(path: os.Path): Either[String, Script] = {
+  def load(path: os.Path): Script = {
     val (pkg, wrapper) = Util.pathToPackageWrapper(Nil, path.relativeTo(wd))
     val codeSource = CodeSource(
       wrapper,
@@ -96,25 +129,20 @@ final case class ScriptProcessor(
         case h :: t if alreadySeen(h) =>
           helper(t, alreadySeen)
         case h :: t =>
-          val maybeDeps = h.dependencies.scriptDependencies
-            .traverse { imp =>
-              imp.code match {
-                case Left(code) => load(code, imp.codeSource)
-                case Right(path) => load(path, imp.codeSource)
-              }
-            }
-
-            maybeDeps match {
-              case Left(errors) => Left(errors.mkString(", "))
-              case Right(deps) =>
-                val filteredDeps = deps.filterNot(alreadySeen)
-                if (filteredDeps.isEmpty) {
-                  if (h != module)
-                    b += h
-                  helper(t, alreadySeen + h)
-                } else
-                  helper(filteredDeps.toList ::: toAdd, alreadySeen)
+          val deps = h.dependencies.scriptDependencies.map { imp =>
+            imp.code match {
+              case Left(code) => load(code, imp.codeSource)
+              case Right(path) => load(path, imp.codeSource)
             }
+          }
+
+          val filteredDeps = deps.filterNot(alreadySeen)
+          if (filteredDeps.isEmpty) {
+            if (h != module)
+              b += h
+            helper(t, alreadySeen + h)
+          } else
+            helper(filteredDeps.toList ::: toAdd, alreadySeen)
       }
 
     helper(module :: Nil, Set.empty)

amm/interp/src/main/scala/ammonite/interp/script/SingleScriptCompiler.scala

@@ -144,18 +144,21 @@ class SingleScriptCompiler(
     blocksOffsetAndCode: Vector[(Int, String)]
   ): Unit = {
 
-    def adjust(blockIdx: Int): (Int, Int) => Option[(Int, Int)] = {
-      val startOffsetInSc = module.blocks(blockIdx - 1).startIdx
-      val startPosInSc = offsetToPosSc(startOffsetInSc)
-
-      PositionOffsetConversion.scalaPosToScPos(
-        module.code,
-        startPosInSc.line,
-        startPosInSc.char,
-        blocksOffsetAndCode(blockIdx - 1)._2,
-        blocksOffsetAndCode(blockIdx - 1)._1
-      )
-    }
+    def adjust(blockIdx: Int): (Int, Int) => Option[(Int, Int)] =
+      if (module.blocks.isEmpty) // can happen if there were errors during preprocessing
+        (_, _) => None
+      else {
+        val startOffsetInSc = module.blocks(blockIdx - 1).startIdx
+        val startPosInSc = offsetToPosSc(startOffsetInSc)
+
+        PositionOffsetConversion.scalaPosToScPos(
+          module.code,
+          startPosInSc.line,
+          startPosInSc.char,
+          blocksOffsetAndCode(blockIdx - 1)._2,
+          blocksOffsetAndCode(blockIdx - 1)._1
+        )
+      }
 
     for {
       target <- moduleTarget

amm/repl/src/main/scala/ammonite/repl/AmmoniteFrontEnd.scala

@@ -28,7 +28,7 @@ case class AmmoniteFrontEnd(extraFilters: Filter = Filter.empty) extends FrontEn
         addHistory(code)
         fastparse.parse(code, Parsers.Splitter(_)) match{
           case Parsed.Success(value, idx) =>
-            Res.Success((code, value))
+            Res.Success((code, value.map(_._2)))
           case f @ Parsed.Failure(_, index, extra) =>
             Res.Failure(
               Preprocessor.formatFastparseError("(console)", code, f)

amm/src/test/resources/bsp/error/error-and-invalid-import-file.sc

@@ -0,0 +1,3 @@
+import $file.nope.nope.nope
+
+zz

amm/src/test/resources/bsp/error/invalid-import-file.sc

@@ -0,0 +1,4 @@
+import scala.collection.mutable._
+import $file.nope.nope.nope
+
+val n = 2