Merge pull request #2396 from som-snytt/issue/unmoored-doc

SI-7376 Scaladoc warns when discarding local doc comments with API tags

8 files changed, 158 insertions, 53 deletions

diff --git a/src/compiler/scala/reflect/macros/runtime/Typers.scala b/src/compiler/scala/reflect/macros/runtime/Typers.scala
index 7e268247dd..070b288c3a 100644
--- a/src/compiler/scala/reflect/macros/runtime/Typers.scala
+++ b/src/compiler/scala/reflect/macros/runtime/Typers.scala
@@ -11,7 +11,7 @@ trait Typers {
   def openImplicits: List[(Type, Tree)] = callsiteTyper.context.openImplicits
 
   /**
-   * @see [[scala.tools.reflect.Toolbox.typeCheck]]
+   * @see [[scala.tools.reflect.ToolBox.typeCheck]]
    */
   def typeCheck(tree: Tree, pt: Type = universe.WildcardType, silent: Boolean = false, withImplicitViewsDisabled: Boolean = false, withMacrosDisabled: Boolean = false): Tree = {
     macroLogVerbose("typechecking %s with expected type %s, implicit views = %s, macros = %s".format(tree, pt, !withImplicitViewsDisabled, !withMacrosDisabled))
diff --git a/src/compiler/scala/reflect/reify/Reifier.scala b/src/compiler/scala/reflect/reify/Reifier.scala
index 11857e2172..74949fce6d 100644
--- a/src/compiler/scala/reflect/reify/Reifier.scala
+++ b/src/compiler/scala/reflect/reify/Reifier.scala
@@ -8,8 +8,9 @@ import scala.reflect.reify.utils.Utils
 /** Given a tree or a type, generate a tree that when executed at runtime produces the original tree or type.
  *  See more info in the comments to `reify` in scala.reflect.api.Universe.
  *
- * @author Martin Odersky
- *  @version 2.10
+ *  @author   Martin Odersky
+ *  @version  2.10
+ *  @since    2.10
  */
 abstract class Reifier extends States
                           with Phases
@@ -31,20 +32,20 @@ abstract class Reifier extends States
     this.asInstanceOf[Reifier { val global: Reifier.this.global.type }]
   override def hasReifier = true
 
-  /**
-   *  For `reifee` and other reification parameters, generate a tree of the form
-   *
+  /** For `reifee` and other reification parameters, generate a tree of the form
+   *  {{{
    *    {
-   *      val $u: universe.type = <[ universe ]>
-   *      val $m: $u.Mirror = <[ mirror ]>
-   *      $u.Expr[T](rtree)       // if data is a Tree
-   *      $u.TypeTag[T](rtree)    // if data is a Type
+   *      val \$u: universe.type = <[ universe ]>
+   *      val \$m: \$u.Mirror = <[ mirror ]>
+   *      \$u.Expr[T](rtree)       // if data is a Tree
+   *      \$u.TypeTag[T](rtree)    // if data is a Type
    *    }
+   *  }}}
    *
    *  where
    *
-   *    - `universe` is the tree that represents the universe the result will be bound to
-   *    - `mirror` is the tree that represents the mirror the result will be initially bound to
+   *    - `universe` is the tree that represents the universe the result will be bound to.
+   *    - `mirror` is the tree that represents the mirror the result will be initially bound to.
    *    - `rtree` is code that generates `reifee` at runtime.
    *    - `T` is the type that corresponds to `data`.
    *
diff --git a/src/compiler/scala/reflect/reify/phases/Metalevels.scala b/src/compiler/scala/reflect/reify/phases/Metalevels.scala
index 18ea908cdf..ea6b5886cc 100644
--- a/src/compiler/scala/reflect/reify/phases/Metalevels.scala
+++ b/src/compiler/scala/reflect/reify/phases/Metalevels.scala
@@ -11,7 +11,7 @@ trait Metalevels {
   /**
    *  Makes sense of cross-stage bindings.
    *
-   *  ================
+   *  ----------------
    *
    *  Analysis of cross-stage bindings becomes convenient if we introduce the notion of metalevels.
    *  Metalevel of a tree is a number that gets incremented every time you reify something and gets decremented when you splice something.
@@ -53,7 +53,7 @@ trait Metalevels {
    *  1) Runtime eval that services dynamic splices requires scala-compiler.jar, which might not be on library classpath
    *  2) Runtime eval incurs a severe performance penalty, so it'd better to be explicit about it
    *
-   *  ================
+   *  ----------------
    *
    *  As we can see, the only problem is the fact that lhs'es of `splice` can be code blocks that can capture variables from the outside.
    *  Code inside the lhs of an `splice` is not reified, while the code from the enclosing reify is.
diff --git a/src/compiler/scala/tools/nsc/ast/DocComments.scala b/src/compiler/scala/tools/nsc/ast/DocComments.scala
index 5ad494177c..f2e5c9b1eb 100755
--- a/src/compiler/scala/tools/nsc/ast/DocComments.scala
+++ b/src/compiler/scala/tools/nsc/ast/DocComments.scala
@@ -366,7 +366,10 @@ trait DocComments { self: Global =>
             case vname  =>
               lookupVariable(vname, site) match {
                 case Some(replacement) => replaceWith(replacement)
-                case None              => reporter.warning(sym.pos, "Variable " + vname + " undefined in comment for " + sym + " in " + site)
+                case None              =>
+                  val pos = docCommentPos(sym)
+                  val loc = pos withPoint (pos.start + vstart + 1)
+                  reporter.warning(loc, s"Variable $vname undefined in comment for $sym in $site")
               }
             }
         }
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenJVMASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenJVMASM.scala
index 2ad474cf3f..59df8e56c1 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenJVMASM.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenJVMASM.scala
@@ -8,11 +8,11 @@ package backend.jvm
 import scala.tools.nsc.io.AbstractFile
 import scala.tools.nsc.symtab._
 
-/** Code shared between the legagy backend [[scala.tools.nsc.backend.jvm.GenJVM]]
-  * and the new backend [[scala.tools.nsc.backend.jvm.GenASM]]. There should be
-  * more here, but for now I'm starting with the refactorings that are either
-  * straightforward to review or necessary for maintenance.
-  */
+/** Code shared between the erstwhile legacy backend (aka GenJVM)
+ *  and the new backend [[scala.tools.nsc.backend.jvm.GenASM]]. There should be
+ *  more here, but for now I'm starting with the refactorings that are either
+ *  straightforward to review or necessary for maintenance.
+ */
 trait GenJVMASM {
   val global: Global
   import global._
diff --git a/src/scaladoc/scala/tools/nsc/doc/ScaladocAnalyzer.scala b/src/scaladoc/scala/tools/nsc/doc/ScaladocAnalyzer.scala
index bf6d6ffed7..003c439f65 100644
--- a/src/scaladoc/scala/tools/nsc/doc/ScaladocAnalyzer.scala
+++ b/src/scaladoc/scala/tools/nsc/doc/ScaladocAnalyzer.scala
@@ -9,7 +9,6 @@ package doc
 import scala.tools.nsc.ast.parser.{ SyntaxAnalyzer, BracePatch }
 import scala.reflect.internal.Chars._
 import symtab._
-import reporters.Reporter
 import typechecker.Analyzer
 import scala.reflect.internal.util.{ BatchSourceFile, RangePosition }
 
@@ -150,20 +149,54 @@ abstract class ScaladocSyntaxAnalyzer[G <: Global](val global: G) extends Syntax
 
   class ScaladocUnitScanner(unit0: CompilationUnit, patches0: List[BracePatch]) extends UnitScanner(unit0, patches0) {
 
-    private var docBuffer: StringBuilder = null        // buffer for comments
-    private var docPos: Position         = NoPosition  // last doc comment position
-    private var inDocComment             = false
+    private var docBuffer: StringBuilder = null        // buffer for comments (non-null while scanning)
+    private var inDocComment             = false       // if buffer contains double-star doc comment
+    private var lastDoc: DocComment      = null        // last comment if it was double-star doc 
 
+    private lazy val unmooredParser = {                // minimalist comment parser
+      import scala.tools.nsc.doc.base.{comment => _, _}
+      new {
+        val global: Global = ScaladocSyntaxAnalyzer.this.global
+      } with CommentFactoryBase with MemberLookupBase {
+        import global.{ settings, Symbol }
+        def parseComment(comment: DocComment) = {
+          val nowarnings = settings.nowarn.value
+          settings.nowarn.value = true
+          try parseAtSymbol(comment.raw, comment.raw, comment.pos)
+          finally settings.nowarn.value = nowarnings
+        }
+
+        override def internalLink(sym: Symbol, site: Symbol): Option[LinkTo] = None
+        override def chooseLink(links: List[LinkTo]): LinkTo = links.headOption orNull
+        override def toString(link: LinkTo): String = "No link"
+        override def findExternalLink(sym: Symbol, name: String): Option[LinkToExternal] = None
+        override def warnNoLink: Boolean = false
+      }
+    }
+
+    /**
+     * Warn when discarding buffered doc at the end of a block.
+     * This mechanism doesn't warn about arbitrary unmoored doc.
+     * Also warn under -Xlint, but otherwise only warn in the presence of suspicious
+     * tags that appear to be documenting API.  Warnings are suppressed while parsing
+     * the local comment so that comments of the form `[at] Martin` will not trigger a warning.
+     * By omission, tags for `see`, `todo`, `note` and `example` are ignored.
+     */
     override def discardDocBuffer() = {
+      import scala.tools.nsc.doc.base.comment.Comment
       val doc = flushDoc
-      if (doc ne null)
-        unit.warning(docPos, "discarding unmoored doc comment")
+      // tags that make a local double-star comment look unclean, as though it were API
+      def unclean(comment: Comment): Boolean = {
+        import comment._
+        authors.nonEmpty || result.nonEmpty || throws.nonEmpty || valueParams.nonEmpty ||
+        typeParams.nonEmpty || version.nonEmpty || since.nonEmpty
+      }
+      def isDirty = unclean(unmooredParser parseComment doc)
+      if ((doc ne null) && (settings.lint || isDirty))
+        unit.warning(doc.pos, "discarding unmoored doc comment")
     }
 
-    override def flushDoc(): DocComment = {
-      if (docBuffer eq null) null
-      else try DocComment(docBuffer.toString, docPos) finally docBuffer = null
-    }
+    override def flushDoc(): DocComment = (try lastDoc finally lastDoc = null)
 
     override protected def putCommentChar() {
       if (inDocComment)
@@ -182,23 +215,19 @@ abstract class ScaladocSyntaxAnalyzer[G <: Global](val global: G) extends Syntax
       super.skipBlockComment()
     }
     override def skipComment(): Boolean = {
-      super.skipComment() && {
-        if (docBuffer ne null) {
-          if (inDocComment)
-            foundDocComment(docBuffer.toString, offset, charOffset - 2)
-          else
-            try foundComment(docBuffer.toString, offset, charOffset - 2) finally docBuffer = null
-        }
+      // emit a block comment; if it's double-star, make Doc at this pos
+      def foundStarComment(start: Int, end: Int) = try {
+        val str = docBuffer.toString
+        val pos = new RangePosition(unit.source, start, start, end)
+        unit.comment(pos, str)
+        if (inDocComment)
+          lastDoc = DocComment(str, pos)
         true
+      } finally {
+        docBuffer    = null
+        inDocComment = false
       }
-    }
-    def foundComment(value: String, start: Int, end: Int) {
-      val pos = new RangePosition(unit.source, start, start, end)
-      unit.comment(pos, value)
-    }
-    def foundDocComment(value: String, start: Int, end: Int) {
-      docPos = new RangePosition(unit.source, start, start, end)
-      unit.comment(docPos, value)
+      super.skipComment() && ((docBuffer eq null) || foundStarComment(offset, charOffset - 2))
     }
   }
   class ScaladocUnitParser(unit: CompilationUnit, patches: List[BracePatch]) extends UnitParser(unit, patches) {
diff --git a/test/scaladoc/run/t5527.check b/test/scaladoc/run/t5527.check
index ab2aeb2d67..235e73b5c8 100644
--- a/test/scaladoc/run/t5527.check
+++ b/test/scaladoc/run/t5527.check
@@ -1,12 +1,12 @@
-newSource1:17: warning: discarding unmoored doc comment
-          /** Testing 123 */
-          ^
-newSource1:27: warning: discarding unmoored doc comment
-        /** Calculate this result. */
+newSource1:47: warning: discarding unmoored doc comment
+        /** Document this crucial constant for posterity.
         ^
-newSource1:34: warning: discarding unmoored doc comment
-        /** Another digit is a giveaway. */
+newSource1:64: warning: discarding unmoored doc comment
+        /*************************\
         ^
+newSource1:73: warning: discarding unmoored doc comment
+        val i = 10 */** Important!
+                   ^
 [[syntax trees at end of                    parser]] // newSource1
 package <empty> {
   object UselessComments extends scala.AnyRef {
@@ -48,6 +48,26 @@ package <empty> {
     val test4 = 'a' match {
       case ('0'| '1'| '2'| '3'| '4'| '5'| '6'| '7'| '8'| '9') => true
       case _ => false
+    };
+    def test5: scala.Unit = if (true)
+      $qmark$qmark$qmark
+    else
+      ();
+    def test6 = {
+      val u = 4;
+      0.to(u).foreach(((i) => println(i)))
+    };
+    def test7 = {
+      val u = 4;
+      0.to(u).foreach(((i) => println(i)))
+    };
+    def test8 = {
+      val z = "fancy";
+      z.replace("fanc", "arts")
+    };
+    def test9 = {
+      val i = 10.$times(10);
+      assert(i.$eq$eq(100))
     }
   };
   /** comments that we should keep */
@@ -102,7 +122,11 @@ package <empty> {
         super.<init>();
         ()
       }
-    }
+    };
+    /** Get the simple value.
+       *  @return the default value
+       */
+    def value: Int = 7
   }
 }
 
diff --git a/test/scaladoc/run/t5527.scala b/test/scaladoc/run/t5527.scala
index 2449ff60c3..60ae23c1a7 100644
--- a/test/scaladoc/run/t5527.scala
+++ b/test/scaladoc/run/t5527.scala
@@ -49,6 +49,47 @@ object Test extends DirectTest {
         case _ =>
           false
       }
+
+      def test5 {
+        /** @martin is this right? It shouldn't flag me as scaladoc. */
+        if (true) ???
+      }
+
+      def test6 = {
+        /** Document this crucial constant for posterity.
+         *  Don't forget to dedoc this comment if you refactor to a local.
+         *  @author Paul Phillips
+         */
+        val u = 4
+        for (i <- 0 to u)
+          println(i)
+      }
+      def test7 = {
+        /** Some standard tags are tolerated locally and shouldn't trigger a warning.
+         *  @note Don't change this unless you know what you're doing. This means you.
+         */
+        val u = 4
+        for (i <- 0 to u)
+          println(i)
+      }
+      def test8 = {
+        /*************************\
+         * Fancy ASCII Art Block *
+         *   @author som-snytt   *
+        \*************************/
+        // this is just a local
+        val z = "fancy"
+        z replace ("fanc", "arts")
+      }
+      def test9 = {
+        val i = 10 */** Important!
+                     *  We have to multiply here!
+                     *  @author community
+                     *  @see SI-1234
+                     */
+                10
+        assert(i == 100)
+      }
     }
 
     /** comments that we should keep */
@@ -85,6 +126,13 @@ object Test extends DirectTest {
       /** class D */
       @deprecated("use ... instead", "2.10.0")
       class D
+
+      /** Get the simple value.
+       *  @return the default value
+       */
+      // an intervening line comment
+      /* I had more to say, but didn't want to pollute the scaladoc. */
+      def value: Int = 7
     }
   """.trim