Add links to Awsome Error Messages

* Improved timeout handling for URL checking
* Check unique URLs only once
* Changed notation for URLs

Author: ennru

compiler/src/dotty/tools/dotc/reporting/diagnostic/Message.scala

@@ -18,8 +18,8 @@ object Message {
 }
 
 sealed trait DocumentationLink {
-  def text: String
   def url: String
+  def text: String
 }
 
 /** A documentation link can be rendered by tooling to direct the programmer to
@@ -32,19 +32,19 @@ sealed trait DocumentationLink {
   * if the error class is tested there.
   */
 object DocumentationLink {
-  case class LanguageSpec(text: String = "Scala Language Specification", suffix: String) extends DocumentationLink {
+  case class LanguageSpec(suffix: String, text: String = "Scala Language Specification") extends DocumentationLink {
     val url = s"https://www.scala-lang.org/files/archive/spec/2.13/$suffix"
   }
-  case class TourUrl(text: String, suffix: String) extends DocumentationLink {
+  case class TourUrl(suffix: String, text: String) extends DocumentationLink {
     val url = s"http://docs.scala-lang.org/overviews/$suffix"
   }
-  case class Sip(text: String, suffix: String) extends DocumentationLink {
+  case class Sip(suffix: String, text: String) extends DocumentationLink {
     val url = s"http://docs.scala-lang.org/sips/$suffix"
   }
-  case class DottyDocs(text: String = "Dotty documentation", suffix: String) extends DocumentationLink {
+  case class DottyDocs(suffix: String, text: String = "Dotty documentation") extends DocumentationLink {
     val url = s"http://dotty.epfl.ch/docs/$suffix"
   }
-  case class FullUrl(text: String, url: String) extends DocumentationLink
+  case class FullUrl(url: String, text: String) extends DocumentationLink
 }
 
 /** A `Message` contains all semantic information necessary to easily

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -23,7 +23,7 @@ import dotty.tools.dotc.ast.Trees
 import dotty.tools.dotc.config.ScalaVersion
 import dotty.tools.dotc.core.Flags.{FlagSet, Mutable}
 import dotty.tools.dotc.core.SymDenotations.SymDenotation
-import diagnostic.DocumentationLink.{DottyDocs, LanguageSpec, TourUrl}
+import diagnostic.DocumentationLink.{DottyDocs, FullUrl, LanguageSpec, TourUrl}
 
 import scala.util.control.NonFatal
 
@@ -99,18 +99,13 @@ object messages {
   import ast.tpd
 
   /** Helper methods for messages */
-  private def tryExpressionLinks = List(
-    LanguageSpec(suffix = "06-expressions.html#try-expressions")
-  )
+  private def tryExpressionLinks = LanguageSpec("06-expressions.html#try-expressions") :: Nil
 
-  private def implicitClassLinks = List(
-    TourUrl("More information on implicit classes", "core/implicit-classes.html")
-  )
+  private def implicitClassLinks = TourUrl("core/implicit-classes.html", "More information on implicit classes") :: Nil
 
-  private def patternMatchingLinks = List(
-    LanguageSpec(suffix = "08-pattern-matching.html"),
-    DottyDocs(suffix = "reference/changed/pattern-matching.html")
-  )
+  private def patternMatchingLinks =
+    LanguageSpec("08-pattern-matching.html") ::
+    DottyDocs("reference/changed/pattern-matching.html") :: Nil
 
 
   // Syntax Errors ---------------------------------------------------------- //
@@ -182,9 +177,7 @@ object messages {
            |use of the ${"with"} keyword. There are a few differences in
            |semantics between intersection types and using `${"with"}'."""
 
-    override val links = List(
-      DottyDocs(suffix = "reference/intersection-types.html")
-    )
+    override val links = DottyDocs("reference/intersection-types.html") :: Nil
   }
 
   case class CaseClassMissingParamList(cdef: untpd.TypeDef)(implicit ctx: Context)
@@ -198,9 +191,7 @@ object messages {
            |have a singleton representation of ${cdef.name}, use a "${"case object"}".
            |Or, add an explicit `()' as a parameter list to ${cdef.name}."""
 
-    override val links = List(
-      LanguageSpec(suffix = "05-classes-and-objects.html#case-classes")
-    )
+    override val links = LanguageSpec("05-classes-and-objects.html#case-classes") :: Nil
   }
 
   case class AnonymousFunctionMissingParamType(param: untpd.ValDef,
@@ -234,9 +225,7 @@ object messages {
            |
            |${"val f: Seq[Int] => Option[List[Int]] = { case xs @ List(1, 2, 3) => Some(xs) }"} """
 
-    override val links = List(
-      LanguageSpec(suffix = "06-expressions.html#anonymous-functions")
-    )
+    override val links = LanguageSpec("06-expressions.html#anonymous-functions") :: Nil
   }
 
   case class WildcardOnTypeArgumentNotAllowedOnNew()(implicit ctx: Context)
@@ -441,9 +430,7 @@ object messages {
 
     }
 
-    override val links = List(
-      DottyDocs(suffix = "reference/trait-parameters.html")
-    )
+    override val links = DottyDocs("reference/trait-parameters.html") :: Nil
   }
 
   case class TopLevelImplicitClass(cdef: untpd.TypeDef)(implicit ctx: Context)
@@ -578,9 +565,7 @@ object messages {
            |"""
     }
 
-    override val links = List(
-      TourUrl("More on String Interpolation", "core/string-interpolation.html")
-    )
+    override val links = TourUrl("core/string-interpolation.html", "More on String Interpolation") :: Nil
   }
 
   case class UnboundPlaceholderParameter()(implicit ctx:Context)
@@ -915,9 +900,7 @@ object messages {
            | - null
            |"""
 
-    override val links = List(
-      LanguageSpec(suffix = "01-lexical-syntax.html#literals")
-    )
+    override val links = LanguageSpec(suffix = "01-lexical-syntax.html#literals") :: Nil
   }
 
   case class PatternMatchExhaustivity(uncovered: String)(implicit ctx: Context)
@@ -1092,9 +1075,7 @@ object messages {
            |
            |${"List[_]"}
            |"""
-    override val links = List(
-      DottyDocs(suffix = "reference/dropped/existential-types.html")
-    )
+    override val links = DottyDocs("reference/dropped/existential-types.html") :: Nil
   }
 
   case class UnboundWildcardType()(implicit ctx: Context)
@@ -1281,9 +1262,7 @@ object messages {
           |  (all other special characters)
           |Operators starting with a letter have lowest precedence, followed by operators starting with `|`, etc.
           |""".stripMargin
-    override val links = List(
-      LanguageSpec(suffix = "06-expressions.html#infix-operations")
-    )
+    override val links = LanguageSpec("06-expressions.html#infix-operations") :: Nil
   }
 
   case class CantInstantiateAbstractClassOrTrait(cls: Symbol, isTrait: Boolean)(implicit ctx: Context)
@@ -1758,9 +1737,7 @@ object messages {
          |In this instance, the modifier combination is not supported"""
     }
 
-    override val links = List(
-      LanguageSpec("Please see the official Scala Language Specification section on modifiers",
-        "05-classes-and-objects.html#modifiers"))
+    override val links = LanguageSpec("05-classes-and-objects.html#modifiers") :: Nil
   }
 
   case class ImplicitFunctionTypeNeedsNonEmptyParameterList()(implicit ctx: Context)
@@ -1779,9 +1756,7 @@ object messages {
          |
          |$code2""".stripMargin
     }
-    override val links = List(
-      DottyDocs(suffix = "reference/implicit-function-types.html")
-    )
+    override val links = DottyDocs("reference/implicit-function-types.html") :: Nil
   }
 
   case class WrongNumberOfParameters(expected: Int)(implicit ctx: Context)
@@ -1805,9 +1780,7 @@ object messages {
     val msg = "expected start of definition"
     val explanation =
       hl"you have to provide either ${"class"}, ${"trait"}, ${"object"}, or ${"enum"} definitions after qualifiers"
-    override val links = List(
-      LanguageSpec(suffix = "09-top-level-definitions.html")
-    )
+    override val links = LanguageSpec(suffix = "09-top-level-definitions.html") :: Nil
   }
 
   case class NoReturnFromInline(owner: Symbol)(implicit ctx: Context)

compiler/test/dotty/tools/dotc/reporting/UrlExistenceChecker.scala

@@ -6,6 +6,7 @@ import java.util.concurrent.atomic.AtomicBoolean
 import org.junit.Assert.fail
 
 import scala.concurrent.{Await, Future}
+import scala.concurrent.duration.DurationInt
 import scala.concurrent.ExecutionContext.Implicits.global
 import scala.util.control.NonFatal
 
@@ -21,52 +22,65 @@ trait UrlExistenceChecker {
   private[this] val runChecks = new AtomicBoolean(true)
   private[this] val httpOk = 200
 
+  private[this] var checkingUrls: Set[String] = Set.empty
   private[this] var checks: List[Future[String]] = Nil
-  private[this] val urlTimeout = 2000 //ms
-  private[this] val systemProperty = "linkChecking" //ms
+  private[this] val urlTimeout = 10.seconds
+  private[this] val urlTimeoutInt: Int = urlTimeout.toMillis.toInt
+  private[this] val systemProperty = "linkChecking"
 
   /** Tries to connect to "https://www.github.com/" and cancels URL testing if
     * unavailable.
-    * Checks system property `linkChecking` to contain `true`.
+    * Checks system property `linkChecking` be undefined or to contain `true`.
     */
-  def checkForConnectivity(): Unit =
+  def checkForConnectivity(): Unit = {
+    def noChecking(text: String =
+        "Warning: URL checking has been switched off as github.com can't be reached."): Unit = {
+      runChecks.set(false)
+      println(text)
+    }
+
     if (sys.props.getOrElse(systemProperty, "true") == "true")
       Future {
         try {
-          if (connectTo("https://www.github.com/") != httpOk) noChecking()
+          if (connectTo("https://www.github.com/", timeout = 2000) != httpOk)
+            noChecking()
         } catch {
           case NonFatal(_) =>
             noChecking()
         }
       }
-    else {
-      runChecks.set(false)
-      println(s"Warning: URL checking has been switched off via system property `$systemProperty`.")
-    }
+    else
+      noChecking(
+        s"Warning: URL checking has been switched off via system property `$systemProperty`.")
+  }
 
   def checkLinks(message: String, links: List[String]): Unit =
     if (runChecks.get())
-      links.foreach(url =>
-        checks = checkUrl(message, url) :: checks)
+      links.foreach { url =>
+        if (!checkingUrls.contains(url)) {
+          checkingUrls += url
+          checks = checkUrl(message, url) :: checks
+        }
+      }
 
+  /** Awaits completion of all URL checks and fails the test if any has a non empty
+    * String as a result.
+    */
   def checkUrlResponses(): Unit =
     if (runChecks.get()) {
-      import scala.concurrent.duration.DurationInt
-      val futureSeq = Future.sequence(checks)
-      val res = Await.result(futureSeq, 5.seconds)
-      val resStr = res.filter(_.nonEmpty)
-      if (resStr.nonEmpty) {
-        fail(
-          s"""Some URLs did not get response OK (200):
-             |  ${resStr.mkString("\n  ")}
-             |(Switch URL checking off in sbt via `set javaOptions in Test += "-DlinkChecking=false"`)""".stripMargin)
-      }
+      val allFutures = Future.sequence(checks)
+      val results = Await.result(allFutures, urlTimeout + 2.seconds).filter(_.nonEmpty)
+      if (results.nonEmpty)
+        fail(s"""Some URLs did not get response OK (200):
+           |  ${results.mkString("\n  ")}
+           |(Switch URL checking off in sbt via `set javaOptions in Test += "-DlinkChecking=false"`)""".stripMargin)
     }
 
   private def checkUrl(message: String, url: String) = Future {
     try {
-      if (connectTo(url) != httpOk)
-        s"$message: No OK response from $url"
+      val response = connectTo(url)
+      if (response != httpOk)
+        s"$message: Got HTTP $response response from $url"
       else ""
     } catch {
       case _: SocketTimeoutException =>
@@ -78,16 +92,12 @@ trait UrlExistenceChecker {
     }
   }
 
-  private def connectTo(url: String): Int = {
+  private def connectTo(url: String, timeout: Int = urlTimeoutInt): Int = {
     val conn = new URL(url).openConnection().asInstanceOf[HttpURLConnection]
     conn.setRequestMethod("HEAD")
-    conn.setConnectTimeout(urlTimeout)
-    conn.setReadTimeout(urlTimeout)
+    conn.setConnectTimeout(timeout)
+    conn.setReadTimeout(timeout)
     conn.getResponseCode
   }
 
-  private def noChecking(): Unit = {
-    runChecks.set(false)
-    println("Warning: URL checking has been switched off as github.com can't be reached.")
-  }
 }