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

Predef.implicitly TODO comment #11124

Open
xuwei-k opened this issue Aug 30, 2018 · 16 comments
Open

Predef.implicitly TODO comment #11124

xuwei-k opened this issue Aug 30, 2018 · 16 comments

Comments

@xuwei-k
Copy link

xuwei-k commented Aug 30, 2018

https://github.com/scala/scala/blob/v2.13.0-M5/src/library/scala/Predef.scala#L214

// TODO: when dependent method types are on by default, give this result type e.type, so that inliner has better chance of knowing which method to inline in calls like implicitly[MatchingStrategy[Option]].zero

Should we just add e.type? or remove TODO comment if incorrect?

@Jasper-M
Copy link
Member

Jasper-M commented Aug 30, 2018

I am very much in favor of this change. It also prevents loss of type information.

@xuwei-k
Copy link
Author

xuwei-k commented Aug 30, 2018

🤔

object A {
  def x1[A](implicit a: A): A = a
  @inline def x2[A](implicit a: A): A = a
  def x3[A](implicit a: A): a.type = a
  @inline def x4[A](implicit a: A): a.type = a

  implicit val i = 42

  def main(): Unit = {
    x1[Int]
  
    x2[Int]
  
    x3[Int]
  
    x4[Int]
  }
}
$ scala -version
Scala code runner version 2.13.0-M5 -- Copyright 2002-2018, LAMP/EPFL and Lightbend, Inc.
$ java -version
java version "1.8.0_181"
Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)
$ scalac -opt:l:inline -opt-inline-from:** A.scala 

javap -v A\$.class

  public void main();
    descriptor: ()V
    flags: ACC_PUBLIC
    Code:
      stack=2, locals=1, args_size=1
         0: aload_0
         1: aload_0
         2: invokevirtual #29                 // Method i:()I
         5: invokestatic  #35                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
         8: invokevirtual #37                 // Method x1:(Ljava/lang/Object;)Ljava/lang/Object;
        11: pop
        12: aload_0
        13: invokevirtual #29                 // Method i:()I
        16: pop
        17: aload_0
        18: aload_0
        19: invokevirtual #29                 // Method i:()I
        22: invokestatic  #35                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
        25: invokevirtual #39                 // Method x3:(Ljava/lang/Object;)Ljava/lang/Object;
        28: pop
        29: aload_0
        30: invokevirtual #29                 // Method i:()I
        33: pop
        34: return
      LineNumberTable:
        line 10: 0
        line 12: 12
        line 14: 17
        line 16: 29
        line 16: 34
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      35     0  this   LA$;
}

@lrytz
Copy link
Member

lrytz commented Aug 30, 2018

Maybe your shell expanded the **, try with '-opt-inline-from:**'?

@smarter
Copy link
Member

smarter commented Aug 30, 2018

Would be interesting to do this change and see what happens to the community build. Same thing with identity and locally .

@lrytz
Copy link
Member

lrytz commented Aug 30, 2018

(Wait, I misunderstood something)

@lrytz
Copy link
Member

lrytz commented Aug 30, 2018

What is the problem with the bytecode you posted above?

@milessabin
Copy link

FYI shapeless has a the method with these semantics, backed by a macro to ensure inlining, and there is also @non's Imp which is basically the same.

I'm sure Erik won't mind me saying that we'd both like to see this change so we can retire our workarounds :-)

@xuwei-k
Copy link
Author

xuwei-k commented Aug 30, 2018

The @inline annotation provide meaningful effects regardless of whether add : a.type = or not.

@lrytz
Copy link
Member

lrytz commented Aug 30, 2018

The inliner works on the bytecode level, on erased types, so the change doesn't matter there.

It can make a difference in constant folding, maybe elsewhere

scala> def id[T](a: T): T = a
id: [T](a: T)T

scala> def ida[T](a: T): a.type = a
ida: [T](a: T)a.type

scala> id(1) + 2 : 3
             ^
       error: type mismatch;
        found   : Int
        required: 3

scala> ida(1) + 2 : 3
res5: Int = 3

@SethTisue SethTisue added this to the 2.13.0-RC1 milestone Aug 30, 2018
@xuwei-k
Copy link
Author

xuwei-k commented Sep 1, 2018

Maybe we should also change following similar methods.

scala/math/Equiv.scala:  @inline def apply[T: Equiv]: Equiv[T] = implicitly[Equiv[T]]
scala/math/Fractional.scala:  @inline def apply[T](implicit frac: Fractional[T]): Fractional[T] = frac
scala/math/Integral.scala:  @inline def apply[T](implicit int: Integral[T]): Integral[T] = int
scala/math/Numeric.scala:  @inline def apply[T](implicit num: Numeric[T]): Numeric[T] = num
scala/math/Ordering.scala:  @inline def apply[T](implicit ord: Ordering[T]) = ord

@milessabin
Copy link

@xuwei-k what would that gain us in those cases?

The rationale for making the change in the case of implicitly is that, depending on the type argument, there are refinements which are lost. But in the case of those type classes there aren't any useful refinements to lose.

@smarter
Copy link
Member

smarter commented Oct 1, 2018

Implementing this in Scala 2 will require fixing the compiler to be a bit more tolerant when using a dependent method as a function (it should just widen the dependent part), from https://github.com/smarter/scala/tree/precise-id:

[error] /home/smarter/opt/scala/src/library/scala/StringContext.scala:114:54: polymorphic expression cannot be instantiated to expected type;
[error]  found   : [A](x: A)x.type (with underlying type [A](x: A)x.type)
[error]  required: String => String
[error]   def raw(args: Any*): String = standardInterpolator(identity, args, parts)
[error]                                                      ^
[error] /home/smarter/opt/scala/src/library/scala/collection/ArrayOps.scala:1209:39: method with dependent type (x: A)x.type cannot be converted to function value
[error]   def distinct: Array[A] = distinctBy(identity)
[error]                                       ^

smarter added a commit to smarter/scala that referenced this issue Oct 1, 2018
Fixes scala/bug#11124

Breaks because scalac doesn't like the dependent result type.
@adriaanm
Copy link
Contributor

adriaanm commented Jan 8, 2019

Let's consider this during 2.14 dev, since it has knock-on effects as mentioned by smarter.

@adriaanm adriaanm modified the milestones: 2.13.0-RC1, 2.14.0-M1 Jan 8, 2019
@xuwei-k
Copy link
Author

xuwei-k commented Mar 17, 2019

scala/scala#7874

@adriaanm
Copy link
Contributor

Here's what I tried to improve type inference, but this broke a massive number of quasiquote-related tests, and we're out of time for RC1:

  object ApproximateDependentMap extends TypeMap {
    def apply(tp: Type): Type =
      if (tp.isImmediatelyDependent) {
        // Don't give up this easily -- we may be able to provide a bit more information by using the widened type.
        // This pattern is used by Predef.implicitly to propagate the type var in the argument type to the result,
        // while also being as precise as possible in the result type.
        val w = tp.widen
        if (tp ne w) apply(w) else WildcardType
      }
      else tp.mapOver(this)
  }

I also tried not changing the compiler, but a slightly more sneaky signature for implicitly:

  @inline def implicitly[T](implicit e: T): e.type with T = e

this runs into other compiler bugs (the evidence parameter may be private, and the result type is not properly widened to avoid depending on it) 😓

@adriaanm
Copy link
Contributor

For the record, I tried one more (perhaps too hacky) compromise, but still too many macro tests break (and I have no idea how to even approach fixing them)

  object ApproximateDependentMap extends TypeMap {
    def apply(tp: Type): Type =
      if (tp.isImmediatelyDependent) {
        // Don't give up just yet -- we may be able to propagate a bit more information for type inference if we can widen to a type parameter.
        // This pattern is used by Predef.implicitly to propagate the type var in the argument type to the result,
        // while also being as precise as possible in the result type.
        val w = tp.widen
        if ((tp ne w) && w.typeSymbol.isTypeParameterOrSkolem) apply(w) else WildcardType
      }
      else tp.mapOver(this)
  }

Test failures:

  • run/macro-reify-chained1
  • run/macro-reify-chained2
  • run/macro-reify-nested-a1
  • run/macro-reify-nested-a2
  • run/macro-reify-nested-b1
  • run/macro-reify-nested-b2

wip: scala/scala@2.13.x...adriaanm:implicitly-precise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants