Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
griggt opened this issue Nov 19, 2020 · 4 comments
Assignees
Labels
area:metaprogramming:quotes Issues related to quotes and splices itype:bug

Comments

@griggt
Copy link
Contributor

griggt commented Nov 19, 2020

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 371d8f5 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
Copy link
Member

smarter commented Nov 20, 2020

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 #9900

@smarter smarter added this to the 3.0.0 milestone Nov 23, 2020
@odersky odersky modified the milestones: 3.0.0, 3.0.0-M3 Dec 14, 2020
@anatoliykmetyuk anatoliykmetyuk removed this from the 3.0.0-M3 milestone Dec 14, 2020
@smarter
Copy link
Member

smarter commented Dec 14, 2020

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 smarter added this to the 3.0.0-RC1 milestone Dec 14, 2020
@nicolasstucki nicolasstucki removed this from the 3.0.0-RC1 milestone Feb 3, 2021
@smarter
Copy link
Member

smarter commented Nov 12, 2021

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 #13627. @nicolasstucki do you still think there might be some issue with stripping Inlined node?

@nicolasstucki
Copy link
Contributor

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

@nicolasstucki nicolasstucki added area:metaprogramming:quotes Issues related to quotes and splices and removed area:metaprogramming labels Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metaprogramming:quotes Issues related to quotes and splices itype:bug
Projects
None yet
Development

No branches or pull requests

5 participants