search

scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.89k stars 1.06k forks source link

Erroneous source position when warning triggered in macro code (mismatched file/span) #10384

Open griggt opened 4 years ago

griggt commented 4 years ago

This issue is the root cause of the problem that prompted #10250 (which only fixes the crash in sbt-bridge).

For illustrative purposes, assume the following patch has been applied to the compiler:

diff --git a/compiler/src/dotty/tools/dotc/report.scala b/compiler/src/dotty/tools/dotc/report.scala
index 61e5dde75a..fabb5168eb 100644
--- a/compiler/src/dotty/tools/dotc/report.scala
+++ b/compiler/src/dotty/tools/dotc/report.scala
@@ -64,7 +64,9 @@ object report:
   }

   def warning(msg: Message, pos: SrcPos = NoSourcePosition)(using Context): Unit =
-    issueWarning(new Warning(msg, addInlineds(pos)))
+    val fullPos = addInlineds(pos)
+    println(s"DEBUG Position: $fullPos")
+    issueWarning(new Warning(msg, fullPos))

   def error(msg: Message, pos: SrcPos = NoSourcePosition, sticky: Boolean = false)(using Context): Unit =
     val fullPos = addInlineds(pos)

Minimized Code

(leading comment lines are included in the source file)

// Assert.scala
import scala.quoted._

class Assert(expression: Boolean)

object Assert:
  inline def assert(inline condition: Boolean): Assert =
    ${ assertMacro('{condition}) }

  def assertMacro(condition: Expr[Boolean])(implicit qctx: QuoteContext): Expr[Assert] =
    import qctx.tasty._

    condition.unseal.underlyingArgument match
      case TypeApply(sel @ Select(lhs, "isInstanceOf"), targs) =>
        let(lhs) { l =>
          val res = l.select(sel.symbol).appliedToTypeTrees(targs).seal.cast[Boolean]
          '{ Assert($res) }.unseal
        }.seal.cast[Assert]

      case _ =>
        '{ Assert($condition) }
// Test.scala
object Test:
  def test() = Assert.assert(0.isInstanceOf[Int])

The above code is a standalone, extremely simplified adaptation of:

import org.scalatest.funsuite._

class FooSuite extends AnyFunSuite {
  test("0 is an Int") {
    assert(0.isInstanceOf[Int])
  }
}

Output

Compiling with 3.0.0-M1:

$ dotc -version
Dotty compiler version 3.0.0-M1-bin-SNAPSHOT-git-818d85a -- Copyright 2002-2020, LAMP/EPFL

$ dotc Assert.scala Test.scala
DEBUG Position: Test.scala:[538..542]
-- [E123] Syntax Warning: Test.scala:3:28 --------------------------------------
3 |  def test() = Assert.assert(0.isInstanceOf[Int])
  |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |The highlighted type test will always succeed since the scrutinee type (TermRef(NoPrefix,val x).show) is a subtype of Int
  | This location contains code that was inlined from Test.scala:4

The DEBUG Position output seems weird, where does Test.scala:[538..542] come from? Test.scala contains only 77 characters, so how can it have a span [538..542]? Also, code that was inlined from Test.scala:4 doesn't make much sense, the file only contains 3 lines and nothing to inline from.

Further Investigation

Let's look at the behavior of a slightly older version of Dotty, specifically commit 371d8f575637ac4f59cfd78b40227bf303976313 just before #9900 was merged.

$ dotc -version
Dotty compiler version 0.28.0-bin-SNAPSHOT-git-371d8f5 -- Copyright 2002-2020, LAMP/EPFL

$ dotc Assert.scala Test.scala
DEBUG Position: Assert.scala:[538..542]
-- [E123] Syntax Warning: Test.scala:3:28 --------------------------------------
3 |  def test() = Assert.assert(0.isInstanceOf[Int])
  |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |The highlighted type test will always succeed since the scrutinee type (TermRef(NoPrefix,val x).show) is a subtype of Int
  | This location contains code that was inlined from Assert.scala:17

Aha, the output here makes much more sense! And now we see where the span of [538..542] comes from: the file Assert.scala.

The source code at position Assert.scala:[538..542] is:

         '{ Assert($res) }.unseal
                   ^^^^^

Further, testing the merge commit (e2ade80) for #9900 shows it to be that PR that is the culprit for the discontinuity between the source file and the span in the source position. Those results omitted here for brevity.

Note that for the issue to occur, it is important that isInstanceOf is handled inside the macro. In other words, if we make this change:

     condition.unseal.underlyingArgument match
-      case TypeApply(sel @ Select(lhs, "isInstanceOf"), targs) =>
-        let(lhs) { l =>
-          val res = l.select(sel.symbol).appliedToTypeTrees(targs).seal.cast[Boolean]
-          '{ Assert($res) }.unseal
-        }.seal.cast[Assert]
-
       case _ =>
         '{ Assert($condition) }

it results in:

$ dotc -version
Dotty compiler version 3.0.0-M1-bin-SNAPSHOT-git-818d85a -- Copyright 2002-2020, LAMP/EPFL

$ $ dotc Assert.scala Test.scala
DEBUG Position: Test.scala:[56..70..75]
-- [E123] Syntax Warning: Test.scala:3:43 --------------------------------------
3 |  def test() = Assert.assert(0.isInstanceOf[Int])
  |                             ^^^^^^^^^^^^^^^^^^^
  |The highlighted type test will always succeed since the scrutinee type (ConstantType(Constant(0)).show) is a subtype of Int

which seems okay.

Updated Syntax

For convenience, here is the test case updated for the syntax used in the 3.0.0-RC1 nightlies:

// Assert.scala
import scala.quoted._

class Assert(expression: Boolean)

object Assert:
  inline def assert(inline condition: Boolean): Assert =
    ${ assertMacro('{condition}) }

  def assertMacro(condition: Expr[Boolean])(using Quotes): Expr[Assert] =
    import quotes.reflect._
    import ValDef.let

    condition.asTerm.underlyingArgument match
      case TypeApply(sel @ Select(lhs, "isInstanceOf"), targs) =>
        let(Symbol.spliceOwner, lhs) { l =>
          val res = l.select(sel.symbol).appliedToTypeTrees(targs).asExprOf[Boolean]
          '{ Assert($res) }.asTerm
        }.asExprOf[Assert]

      case _ =>
        '{ Assert($condition) }

Expectation

Source position should have internally consistent source file and span, and reported diagnostics should make sense.

smarter commented 4 years ago

Many thanks for getting to the bottom of this! @nicolasstucki / @odersky : could one of you take a look at this? It's a regression from https://github.com/lampepfl/dotty/pull/9900

smarter commented 3 years ago

According to @nicolasstucki the problem here is the use of underlyingArgument in the macro which strips the Inlined nodes which keep track of positions: the output of underlyingArgument should be inspected but not used to generate trees. This should at least be documented but maybe we need a different API to steer people in the right direction (mapUnderlyingArgument ?)

smarter commented 3 years ago

Trying the same example adapted for Scala 3.1:

// Assert.scala
import scala.quoted._

class Assert(expression: Boolean)

object Assert:
  inline def assert(inline condition: Boolean): Assert =
    ${ assertMacro('{condition}) }

  def assertMacro(condition: Expr[Boolean])(implicit qctx: Quotes): Expr[Assert] =
    import qctx.reflect._

    condition.asTerm.underlyingArgument match
      case TypeApply(sel @ Select(lhs, "isInstanceOf"), targs) =>
        ValDef.let(Symbol.spliceOwner, lhs) { l =>
          val res = l.select(sel.symbol).appliedToTypeTrees(targs).asExprOf[Boolean]
          '{ Assert($res) }.asTerm
        }.asExprOf[Assert]

      case _ =>
        '{ Assert($condition) }
// Test.scala
object Test:
  def test() = Assert.assert(0.isInstanceOf[Int])
DEBUG Position: try/Assert.scala:[560..564]
-- [E123] Syntax Warning: try/Test.scala:2:28 -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
2 |  def test() = Assert.assert(0.isInstanceOf[Int])
  |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |               The highlighted type test will always succeed since the scrutinee type (x : Int) is a subtype of Int
  | This location contains code that was inlined from Assert.scala:17

It looks like everything works correctly now, even though the Inlined node is stripped! On the other hand, the position was wrong in Scala 3.0.2. I believe this was fixed by @mbovel in https://github.com/lampepfl/dotty/pull/13627. @nicolasstucki do you still think there might be some issue with stripping Inlined node?

nicolasstucki commented 3 years ago

Not sure, the fix in #13627 might have compensated for the missing Inlined node.