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

added to type mismatch reporting output of tree, which can't be typed #12717

Merged
merged 1 commit into from
Jul 27, 2021

Conversation

rssh
Copy link
Contributor

@rssh rssh commented Jun 5, 2021

added to type mismatch reporting output of tree, which can't be typed in -explain mode.
Because in macros, tree in sourcecode in position and a processed tree can be very different.

@SethTisue
Copy link
Member

SethTisue commented Jul 7, 2021

Looks plausible, but needs test coverage, yes? (In part because a test or two would help illustrate the purpose of the PR. Perhaps that's why it hasn't attracted reviewer attention so far.)

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the intent seems reasonable to me, to help understand what went wrong in the expanded macro code. But we'd want to see some examples as tests to review and maintain this enhancement.

@rssh
Copy link
Contributor Author

rssh commented Jul 11, 2021

Just added tests. will rebase and squash now.
Short description of test-case here:

package t12717

import scala.quoted._

object A:

   def foo(x:Int): Int = ???

   def bar(x:String): String = ???


object X:

   inline def doSomething[T](inline x:T):Any = ${
      doSomethingImpl('x)
   }

   def doSomethingImpl[T:Type](x:Expr[T])(using Quotes):Expr[Any] =
     import quotes.reflect._
     val aTerm = '{A}.asTerm
     val xBar = Apply(Select.unique(aTerm,"bar"),List(x.asTerm))
     Apply(Select.unique(aTerm,"foo"), List(xBar)).asExpr

And usage:

package t12717

object Test:

   val x = X.doSomething("XXX") // error

The output is:

-- [E007] Type Mismatch Error: tests/neg-custom-args/hidden-type-errors/Test.scala:6:24 --------------------------------
6 |   val x = X.doSomething("XXX") // error
  |           ^^^^^^^^^^^^^^^^^^^^
  |           Found:    String
  |           Required: Int
  | This location contains code that was inlined from Test.scala:6

Explanation
===========

Tree: t12717.A.bar("XXX")

I tried to show that
  String
conforms to
  Int
but the comparison trace ended with `false`:


  ==> String  <:  Int
    ==> String  <:  Int (recurring)
      ==> String  <:  Int (recurring)
      <== String  <:  Int (recurring) = false
    <== String  <:  Int (recurring) = false
  <== String  <:  Int = false

The tests were made under the empty constraint

1 error found

Without

Tree: t12717.A.bar("XXX")

it's hard to understand, why the type checking of macro result has failed.

@rssh
Copy link
Contributor Author

rssh commented Jul 11, 2021

btw, also I'm not sure that I added the test in the right place: looks like a neg-custom-args check only the existence of error, not an exact output of the compiler. Maybe we need a new category for such test types (?)

…in macros pos can point to complete other thing.

 added tests for hidden-compiler

Co-authored-by: Dale Wijnand <dale.wijnand@gmail.com>
@dwijnand dwijnand self-assigned this Jul 12, 2021
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. What I see missing is that Int is expected because that tree is being passed to A.foo, which is still only understandable from reading the macro. So it almost seems like a half improvement, but that's still an improvement over the status quo.

I wouldn't mind another maintainer weighing in with or against me, before merging.

@dwijnand dwijnand removed their assignment Jul 12, 2021
@dwijnand dwijnand merged commit b902516 into scala:master Jul 27, 2021
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants