Skip to content
This repository was archived by the owner on Sep 1, 2020. It is now read-only.

42.type #18

Closed
wants to merge 16 commits into from
Closed

42.type #18

wants to merge 16 commits into from

Conversation

puffnfresh
Copy link

This @folone's branch, cherry-picked onto 2.11.x - opening a PR so that we can play around with it.

@non
Copy link

non commented Sep 5, 2014

👍

I think everyone agrees we want this feature, so maybe we should just merge this work and merge later fixes as needed?

@puffnfresh
Copy link
Author

I'd like to hear from @folone before merging.

@folone
Copy link

folone commented Sep 5, 2014

Hey folks! This branch has ~8 failing tests. IIRC, those just need updates to the .check files, but I'm in a land of almost no internet atm, and cannot verify. I'll be back on Saturday evening, promise to look into this during the weekend.

aloiscochard pushed a commit to aloiscochard/scala that referenced this pull request Sep 5, 2014
Properly quote regex replacement string in path canonicalization
@milessabin
Copy link
Member

Yeah, we should definitely wait for @folone.

I have one comment: given that this is a patch against the compiler, why does it include a macro? Surely we could just make SingleInhabitant an intrinsic?

@milessabin
Copy link
Member

See also #27.

@folone
Copy link

folone commented Sep 8, 2014

Sorry for the wait. So, here's the diff that needs to be applied to the branch so that it builds cleanly: https://gist.github.com/folone/c1755c105504da26c298. Those changes are – changing things like Int(42) to 42.type, removing tests that did not compile previously (because of Any vs AnyRef issue), fixing bincompat.

There are more global issues that @milessabin mentioned. Namely:

  • Using Singleton type bound instead of SingleInhabitant typeclass
  • Reimplementing inhabitant function without a macro

There are other known issues that are described in the 42.type.scala test.

@milessabin
Copy link
Member

How would you feel about splitting the SingletonInhabitant part out of this for now?

I would very much like to be able to make shapeless's Witness and WitnessWith go away, but before I'm completely confident of the best way to do that I'd like to have a chance to play with the combination of this and a fix for #27.

@folone
Copy link

folone commented Sep 8, 2014

So, just remove SingletonInhabitant and def inhabitant for now (before we have a good solution to materialize values of singleton types)? Sure, sounds like a plan.

@milessabin
Copy link
Member

@folone sounds good.

@puffnfresh
Copy link
Author

See #45.

@puffnfresh puffnfresh closed this Sep 12, 2014
@puffnfresh puffnfresh removed the ready label Sep 12, 2014
puffnfresh pushed a commit to puffnfresh/scala that referenced this pull request Jul 30, 2015
In Scala 2.8.2, an optimization was added to create a static
cache for Symbol literals (ie, the results of `Symbol.apply("foo"))`.
This saves the map lookup on the second pass through code.

This actually was broken somewhere during the Scala 2.10 series,
after the addition of an overloaded `apply` method to `Symbol`.

The cache synthesis code was made aware of the overload and brought
back to working condition recently, in scala#3149.

However, this has uncovered a latent bug when the Symbol literals are
defined with traits.

One of the enclosed tests failed with:

	  jvm > t8933b-run.log
	java.lang.IllegalAccessError: tried to access field MotherClass.symbol$1 from class MixinWithSymbol$class
	        at MixinWithSymbol$class.symbolFromTrait(A.scala:3)
	        at MotherClass.symbolFromTrait(Test.scala:1)

This commit simply disables the optimization if we are in a trait.
Alternative fixes might be: a) make the static Symbol cache field
public / b) "mixin" the static symbol cache. Neither of these
seem worth the effort and risk for an already fairly situational
optimization.

Here's how the optimization looks in a class:

	% cat sandbox/test.scala; qscalac sandbox/test.scala && echo ":javap C" | qscala;
	class C {
	  'a; 'b
	}
	Welcome to Scala version 2.11.5-20141106-145558-aa558dce6d (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_20).
	Type in expressions to have them evaluated.
	Type :help for more information.

	scala> :javap C
	  Size 722 bytes
	  MD5 checksum 6bb00189166917254e8d40499ee7c887
	  Compiled from "test.scala"
	public class C

	{
	  public static {};
	    descriptor: ()V
	    flags: ACC_PUBLIC, ACC_STATIC
	    Code:
	      stack=2, locals=0, args_size=0
	         0: getstatic     typelevel#16                 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
	         3: ldc           typelevel#18                 // String a
	         5: invokevirtual typelevel#22                 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
	         8: putstatic     typelevel#26                 // Field symbol$1:Lscala/Symbol;
	        11: getstatic     typelevel#16                 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
	        14: ldc           typelevel#28                 // String b
	        16: invokevirtual typelevel#22                 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
	        19: putstatic     typelevel#31                 // Field symbol$2:Lscala/Symbol;
	        22: return

	  public C();
	    descriptor: ()V
	    flags: ACC_PUBLIC
	    Code:
	      stack=1, locals=1, args_size=1
	         0: aload_0
	         1: invokespecial typelevel#34                 // Method java/lang/Object."<init>":()V
	         4: getstatic     typelevel#26                 // Field symbol$1:Lscala/Symbol;
	         7: pop
	         8: getstatic     typelevel#31                 // Field symbol$2:Lscala/Symbol;
	        11: pop
	        12: return
	}

fixup
puffnfresh pushed a commit to puffnfresh/scala that referenced this pull request Jul 30, 2015
These methods are "signature polymorphic", which means that compiler
should not:
  1. adapt the arguments to `Object`
  2. wrap the repeated parameters in an array
  3. adapt the result type to `Object`, but instead treat it as it
     it already conforms to the expected type.

Dispiritingly, my initial attempt to implement this touched the type
checker, uncurry, erasure, and the backend.

However, I realized we could centralize handling of this in the typer
if at each application we substituted the signature polymorphic
symbol with a clone that carried its implied signature, which is
derived from the types of the arguments (typechecked without an
expected type) and position within and enclosing cast or block.

The test case requires Java 7+ to compile so is currently embedded
in a conditionally compiled block of code in a run test.

We ought to create a partest category for modern JVMs so we can
write such tests in a more natural style.

Here's how this looks in bytecode. Note the `bipush` / `istore`
before/after the invocation of `invokeExact`, and the descriptor
`(LO$;I)I`.

```
% cat sandbox/poly-sig.scala && qscala Test && echo ':javap Test$#main' | qscala
import java.lang.invoke._

object O {
  def bar(x: Int): Int = -x
}

object Test {
  def main(args: Array[String]): Unit = {
    def lookup(name: String, params: Array[Class[_]], ret: Class[_]) = {
      val lookup = java.lang.invoke.MethodHandles.lookup
      val mt = MethodType.methodType(ret, params)
      lookup.findVirtual(O.getClass, name, mt)
    }
    def lookupBar = lookup("bar", Array(classOf[Int]), classOf[Int])

    val barResult: Int = lookupBar.invokeExact(O, 42)
    ()
  }
}

scala> :javap Test$#main
  public void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: ACC_PUBLIC
    Code:
      stack=3, locals=3, args_size=2
         0: aload_0
         1: invokespecial typelevel#18                 // Method lookupBar$1:()Ljava/lang/invoke/MethodHandle;
         4: getstatic     typelevel#23                 // Field O$.MODULE$:LO$;
         7: bipush        42
         9: invokevirtual typelevel#29                 // Method java/lang/invoke/MethodHandle.invokeExact:(LO$;I)I
        12: istore_2
        13: return
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      14     0  this   LTest$;
            0      14     1  args   [Ljava/lang/String;
           13       0     2 barResult   I
      LineNumberTable:
        line 16: 0
}
```

I've run this test across our active JVMs:

```
% for v in 1.6 1.7 1.8; do java_use $v; pt --terse test/files/run/t7965.scala || break; done
java version "1.6.0_65"
Java(TM) SE Runtime Environment (build 1.6.0_65-b14-466.1-11M4716)
Java HotSpot(TM) 64-Bit Server VM (build 20.65-b04-466.1, mixed mode)
Selected 1 tests drawn from specified tests

.

1/1 passed (elapsed time: 00:00:02)
Test Run PASSED
java version "1.7.0_71"
Java(TM) SE Runtime Environment (build 1.7.0_71-b14)
Java HotSpot(TM) 64-Bit Server VM (build 24.71-b01, mixed mode)
Selected 1 tests drawn from specified tests

.

1/1 passed (elapsed time: 00:00:07)
Test Run PASSED
java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
Selected 1 tests drawn from specified tests

.

1/1 passed (elapsed time: 00:00:05)
Test Run PASSED
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants