Make -Xexperimental features available separately #5376

Merged
merged 2 commits into from Dec 1, 2016

Projects

None yet

6 participants

@milessabin
Contributor
milessabin commented Sep 5, 2016 edited

I've removed all the code which is guarded by a bare -Xexperimental flag setting. In the case of the things that were covered by enableTypeVarExperimentals the status of all tests remains the same, so I think that the functionality it was intended to enable has become part of the default case.

I have also disabled the code supporting match virtualization ... I'd be happy to put it back under a -Yvirtpatmat flag.

And I've removed the disabling of the handling of SAMs where 2.11.x source code compatibility has been requested. Again, I would be happy to put this back under a -Ysam flag, but I'm not convinced that partial downgrades make a lot of sense, so I would prefer to just remove this functionality.

@scala-jenkins scala-jenkins added this to the 2.12.1 milestone Sep 5, 2016
@adriaanm
Member
adriaanm commented Sep 5, 2016

But what about the users of the improved type inference for existentials, say? We'd also like to preserve the escape hatch to opt out of SAM conversion for older compilers. We can discuss refinements of this PR during 2.12.x, as we would like to have a good experience for -Xexperimental. The 2.12.0 release is not the right time for this.

We have one more tweak to make that was discovered by the community build (scala-js/scala-js#2580 (comment)). This is the only kind of change we will make during the whole 2.12.0 RC cycle: fixing bugs that prevent use of non-experimental language features and that do not have a reasonable workaround. We look forward to getting back to the regular swing of subsequent 2.12.1 and up releases, but we have to get 2.12.0 out first!

@milessabin
Contributor

On improved type inference for existentials: no tests have been affected by this change ... see my commit message. Do you have any particular test in mind which would fail if this was no longer supported? Either this is now covered by the default case or there is no test coverage for it at all at present.

On the Scala 2.11.x compatibility front, yes, I guess this should go back. I will need to make similar changes in TLS for 2.11.x, and it's really there that an explicit flag for SAMs becomes an issue.

You didn't mention the match virtualization issue ... what are your thoughts on that for > 2.12.0?

I understand your desire to get 2.12.0 out the door, however, if -Xexperimental continues to be a "mystery meat" option, then we should not recommend that people use it in preference to -Ypartial-unification and that should be reflected in the release notes.

@adriaanm
Member
adriaanm commented Sep 6, 2016

Ok, I'm happy with that compromise. I'll add this PR to the release notes label to remind ourselves.

@milessabin
Contributor

I'm not quite sure what the compromise is? :-D

@adriaanm
Member
adriaanm commented Sep 6, 2016

I prefer recommending -Xexperimental. You prefer merging this PR for RC1. We'll do neither for 2.12.0

@milessabin
Contributor

Gotcha ... yes, that's a good compromise :-)

@milessabin
Contributor

Rebased.

@milessabin
Contributor

Squashed and rebased.

@milessabin
Contributor

Rebased.

@adriaanm ... are there any changes you'd like to see before merging?

@adriaanm
Member

I'm sad to see my various experimental features go. I know of at least some users of the improved type inference for existentials, and virtualized pattern matching is kind of neat too, wouldn't you say? I don't think we can just drop these and rely on the test suite to catch this, sadly -- our test coverage is far from 100%.

@milessabin
Contributor

Understood, but this did result in problems for someone enabling -Xexperimental in preference to -Ypartial-unification. Do you consider these things mature enough to make non-experimental? If not, do you (or anyone else who's invested in them) have time to get them there?

The virtualization stuff is for @TiarkRompf and @namin mainly, isn't it? We can ask them if they still need it.

@milessabin
Contributor

Here's the "mystery meat" issue we had with -Xexperimental in TLS: typelevel#113. I'm not sure exactly what feature is being enabled by -Xexperimental which is responsible for that, so it's not clear to me if the failure is a bug or an intended change. Either way, it would be unfortunate to pull in something with an unclear status as a side effect of encouraging people to use -Xexperimental to enable features that they definitely do want.

At a minimum we need to be able to turn off undesirable features. For that we would need explicit -Y options for each feature which is currently guarded by bare Xexperimental. I'm happy to do that if you agree it's the right way to go.

@milessabin
Contributor

Another option might be to separate out a -Xearly-access which is a strict subset of -Xexperimental.

It occurs to me that the Scala virtualized infrastructure will probably never be enabled by default, so I think it probably warrants a flag of its own, independent from -Xexperimental.

@adriaanm
Member

I propose we add Y flags for the esoteric stuff like virtpatmat. The type
inference changes should be safe under experimental. Do you have reports of
it breaking something?
On Thu, Nov 17, 2016 at 01:47 Miles Sabin notifications@github.com wrote:

Another option might be to separate out a -Xearly-access which is a
strict subset of -Xexperimental.

It occurs to me that the Scala virtualized infrastructure will probably
never be enabled by default, so I think it probably warrants a flag of its
own, independent from -Xexperimental.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5376 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFjy9mjj2iy2Pysw9gwar5PlqCX39SKks5q_CLIgaJpZM4J1FeO
.

@milessabin
Contributor
milessabin commented Nov 17, 2016 edited

@adriaanm it turns out the issue I linked to earlier only fails with -Xexperimental on 2.11.x.

Can you point to an explanation of what the type inference changes actually are?

@adriaanm
Member
adriaanm commented Nov 17, 2016 edited

The code had gotten a bit overfactored, but generally I think the comment explained it well:

When comparing two types containing skolems, remember the highest level
of skolemization. If that highest level is higher than our initial
skolemizationLevel, we can't re-use those skolems as the solution of this
typevar, which means we'll need to repack our inst into a fresh existential.

Apparently we'll need to fix https://issues.scala-lang.org/browse/SI-5729 before it can be enabled by default, but it should eventually become default behavior.

@milessabin
Contributor

Do you have small example which illustrates the difference with the flag enabled vs. not?

@milessabin
Contributor
milessabin commented Nov 17, 2016 edited

I have to be honest, I don't think I could recommend anyone to use -Xexperimental if we know up front that it enables a bug which looks like it could have an impact in not particularly improbable code.

If we know that we have to fix SI-5729 before it can be enabled by default, can we either fix SI-5729 now, or defer the inclusion of this in -Xexperimental until it is fixed?

@milessabin
Contributor
milessabin commented Nov 17, 2016 edited

@dwijnand suggested (as a way to find out about real-world impact),

One way would be if you created a PR to the community build enabling it by default and requested a Jenkins build of your PR. Might find you some real world cases.

Which I think is a great idea. Unfortunately it won't work because -Xexperimental enables all the features it covers, not just this one. If nothing else, I think this is an argument in favour of having a flag to enable the feature individually even if we only expect and encourage people to enable it via -Xexperimental.

@dwijnand
Member

That's true.

Given this PR is about -Xexperimental in general, perhaps a community build run with -Xexperimental would be informative.

@milessabin
Contributor

We know that will fail because the SI-2712 fix will break projects which already have unapply-like workarounds for SI-2712.

@retronym
Member

Perhaps try -Xexperimental -Ypartial-unification:false ...

⚡ printf ":settings\n" | scala -Xexperimental -Ypartial-unification:false
Welcome to Scala 2.12.0 (OpenJDK 64-Bit Server VM, Java 1.8.0_112-release).
Type in expressions for evaluation. Or try :help.

scala> :settings
-Xexperimental = true
-Ypartial-unification = false
-d = .
-deprecation = true
-encoding = UTF-8
-feature = true
-nowarn = false

scala> :quit
@milessabin
Contributor

@retronym true. Good suggestion.

But hey, just look at that ... isn't it helpful to be able to use flags to isolate the effects of individual features ... just sayin' ...

@milessabin
Contributor

Can someone point me to instructions for how to setup/request a community build with a set of flags enforced globally?

@SethTisue
Member

also possible to run locally with scalac_opts=whatever ./run.sh

@milessabin
Contributor

I've now managed to complete a community build with -Xexperimental -Ypartial-unification:false (yes, it really did take me this long) and it's successful, meaning that @adriaanm's type inference improvements are safe, at least to the extent that the community build is representative.

However, I'm firmly in favour of all features under -Xexperimental either being individually controllable, or becoming default. Given that the type inference changes appear to be safe, I'd like to see them made default as part of this -Xexperimental cleanup.

If that's deemed too risky, then I think it's unreasonable to deny prospective users of -Xexperimental the option of turning it off individually (other than by continuing to not use -Xexperimental and only enabling the features they can enable individually ... which is the opposite of what we're trying to achieve here).

I'll also reinstate the virtpatmat code under a -Y flag as @adriaanm suggested.

milessabin added some commits Nov 28, 2016
@milessabin milessabin Pattern matching virtualization now guarded by -Yvirtpatmat.
6ec56ab
@milessabin milessabin Typevar experimentals now default; t5729 pos -> neg.
1870f1a
@milessabin
Contributor
milessabin commented Nov 28, 2016 edited

I've taken a close look at SI-5729, SLS 6.26.3 and concluded that the implementation of asSpecificAs matches the spec, hence that the ambiguity reported in SI-5729 is the correct behaviour, not a bug: the spec provides no justification for preferring the polymorphic join[S] over the monomorphic (albeit existential) join.

So I've enabled the improved inference by default and moved t5729.scala from pos to neg.

@milessabin
Contributor

I've also left the 2.11.x source code compatibility handling of SAMs alone.

@milessabin
Contributor

It seems that Dotty agrees that the SI-5729 case should be ambiguous.

@adriaanm
Member
adriaanm commented Nov 29, 2016 edited

I ran out of time (for today) while convincing myself that that is true. Here's my sketch of a counter-example, where I've tried to reduce method application to assignment:

trait C[+X]

object Test {
  def join(in: C[_]): Int = ???
  def join[S](in: C[S]): String = ???
  trait Equiv { 
    type S 

    val alt1: C[S] = alt2 // can't supply a C[_] when a C[S] was expected for universally quantified S
    val alt2: C[_] = alt1 // a C[S] with unknown S can pass as the existentially quantified C[_]
  }

  join(null: C[_]) // should pick the polymorphic one, since it's more specific
}

Abbreviating the spec to the parts we need:

Overloading Resolution

An alternative A is more specific than an alternative B if
A is as specific as B and B is not as specific as A:

  • A parameterized method m of type (p_1:T_1, ... , p_n:T_n)U is as specific as some other member m'
    if m' is applicable to arguments (p_1 , ... , p_n) of types T_1 , ... , T_n.
  • A polymorphic method of type [a_1 >: L_1 <: U_1 , ... , a_n >: L_n <: U_n]T is as specific as some other member of type S
    if T is as specific as S under the assumption that each a_i is an abstract type name bounded from below by L_i and from above by U_i for i = 1 , ... , n.

Function Applications

An application f(e_1 , ... , e_m) applies the function f to the argument expressions e_1, ... , e_m. For this expression to be well-typed, the function must be applicable to its arguments, which is defined next by case analysis on f's type.

If f has a method type (p_1:T_1 , ... , p_n:T_n)U, each argument expression e_i is typed with the corresponding parameter type T_i as expected type. Let S_i be the type of argument e_i (i = 1 , ... , m). The function f must be applicable to its arguments e_1, ... , e_n of types S_1 , ... , S_n. We say that an argument expression e_i is a named argument if it has the form x_i=e'_i and x_i is one of the parameter names p_1, ..., p_n.

Once the types S_i have been determined, the function f of the above method type is said to be applicable if all of the following conditions hold:

  • for every named argument p_j=e_i' the type S_i is compatible with the parameter type T_j;
  • for every positional argument e_i the type S_i is compatible with T_i;
  • if the expected type is defined, the result type U is compatible to it.

If f is a polymorphic method, local type inference is used to instantiate f's type parameters.
The polymorphic method is applicable if type inference can determine type arguments so that the instantiated method is applicable.

@adriaanm adriaanm self-assigned this Nov 30, 2016
@milessabin
Contributor
milessabin commented Nov 30, 2016 edited

@adriaanm you've missed out,

A member of any other type is always as specific as a parameterized method or a polymorphic method.

Which applied to this case I read as saying that an applicable monomorphic method is as specific as a polymorphic method. And the implementation seems to directly reflect that ... matches on MethodType appear before matches on PolyType in the ranking function.

@milessabin
Contributor
milessabin commented Nov 30, 2016 edited

For comparison,

Welcome to Scala 2.12.0 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_112).
Type in expressions for evaluation. Or try :help.

scala> object C {
     |   def join(in: Seq[List[Any]]): Int = 23
     |   def join[S](in: Seq[List[S]]): String = "foo"
     | }
defined object C

scala> C.join(Seq[List[Int]]())
<console>:13: error: ambiguous reference to overloaded definition,
both method join in object C of type [S](in: Seq[List[S]])String
and  method join in object C of type (in: Seq[List[Any]])Int
match argument types (Seq[List[Int]])
       C.join(Seq[List[Int]]())
         ^

I believe that the case reported in SI-5729 should be handled exactly the same as this one (which it is after this PR).

@adriaanm
Member

A member of any other type is always as specific as a parameterized method or a polymorphic method.

this case does not apply here, since we're comparing only PolyType and MethodType. The quoted case above is a base case for when non-method members are thrown in the mix.

@milessabin
Contributor

How about my example where the existential is replaced by Any?

@milessabin
Contributor
milessabin commented Nov 30, 2016 edited

I think that the first clause is enough to argue that the monomorphic method is as specific as the polymorphic method (and hence that these two should be ambiguous), if your improved type inference for existentials is in play. That rule says that a method m is as specific as a method m' if m' is applicable to an argument list typed as the parameters of m.

So consider,

scala> object C {
     |   def join(in: Seq[List[_]]): Int = 23
     |   def join[S](in: Seq[List[S]]): String = "foo"
     |   def join2[S](in: Seq[List[S]]): String = "foo"
     | }
defined object C

Without -Xexperimental this condition is (unintuitively, incorrectly IMO) not met because of a failure of type inference. We can see this without being distracted by ambiguity by attempting this with join2,

scala> C.join2(Seq[List[Int]](): Seq[List[_]]): String
<console>:13: error: no type parameters for method join2: (in: Seq[List[S]])String exist so that it can be applied to arguments (Seq[List[_]])
 --- because ---
argument expression's type is not compatible with formal parameter type;
 found   : Seq[List[_]]
 required: Seq[List[?S]]
       C.join2(Seq[List[Int]](): Seq[List[_]]): String
         ^
<console>:13: error: type mismatch;
 found   : Seq[List[_]]
 required: Seq[List[S]]
       C.join2(Seq[List[Int]](): Seq[List[_]]): String

On the other hand, with -Xexperimental the same application of join2 is valid,

scala> C.join2(Seq[List[Int]](): Seq[List[_]]): String
res0: String = foo

which I assume is the main objective of your change.

But from this it immediately follows that the first clause of the rule is met: join[S] is applicable to argument lists which are typed as the parameters of join, so join is at least as specific as join[S].

So the ambiguity due to enabling -Xexperimental is, correctly, due to the polymorphic method becoming more widely applicable due to improved type inference.

@adriaanm
Member
adriaanm commented Nov 30, 2016 edited

Ok, you convinced me. Here's how I see it:

class Test {
trait T[x] // OR `class T[+x]` OR `class T[-x]`

// if these two definitions compile...
def joinX(in: T[_]): String = joinP(in)
def joinP[S](in: T[S]): String = joinX(in)

def joinO(in: T[_]): Int = 1
def joinO[S](in: T[S]): String = "a"

// ... this overloaded call is ambiguous, since the definitions above 
// imply neither is more specific (since they are "inter-applicable")
// (if joinX/joinP do not compile, over overload is more specific than 
// the other, and the call below should type check)
joinO(null)
}
@adriaanm
Member
adriaanm commented Dec 1, 2016

Thanks for pushing this through, @milessabin!

@adriaanm adriaanm merged commit 038c15e into scala:2.12.x Dec 1, 2016

6 checks passed

cla @milessabin signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [3813] SUCCESS. Took 18 s.
Details
validate-main [4318] SUCCESS. Took 145 min.
Details
validate-publish-core [4187] SUCCESS. Took 31 min.
Details
validate-test [3688] SUCCESS. Took 94 min.
Details
@milessabin
Contributor

Awesome ... thanks!

@larsrh larsrh referenced this pull request Dec 1, 2016
Merged

Remove deprecated -Y flags #5558

@SethTisue
Member

thanks Miles, this is a really nice cleanup.

@retronym
Member
retronym commented Dec 2, 2016

Regarding discussions about isAsSpecific, I'd like to cross reference some problem I found with the implementation that I tried to fix, but wasn't able to without causing too much breakage. In the end, I've punted that work to scala/scala-dev#207.

trait M1[A]; trait M2[A] extends M1[A]

trait N1[A[_]]; trait N2[A[_]] extends N1[A]

object Test {
  type D = DummyImplicit
  def m[A](implicit d: D): M1[A] = ???
  def m[A](implicit d1: D, d2: D): M2[A] = ???
  m // not ambiguous, M1[_] <:< M2[_] (type params of the methods are existentially abstracted in `isAsSpecificValueType`)

  def n[A[_]](implicit d: D): N1[A] = ???
  def n[A[_]](implicit d1: D, d2: D): N2[A] = ???
  n // was not ambiguous in 2.11:
    // existentialAbtraction(A :: Nil, N2[A]) did not notice that `N2[A]` contained
    // `A` (because it was eta expanded to [_]A[_] before matching for `case TypeRef(_, ASymbol, _)`)
}

I'm not sure if whether it is relevant to this discussion or not.

@adriaanm adriaanm added a commit to adriaanm/scala that referenced this pull request Dec 2, 2016
@adriaanm adriaanm Revert existential infer part of #5376
It wasn't a good idea after all.

Also removed some tracing code that I cannot imagine
was ever used in a production compiler. It's still just
a recompile away.

Fixes scala/scala-dev#280
891ecbf
@milessabin
Contributor

@adriaanm @retronym can you elaborate on "too much breakage"? I was able to successfully run a community build with this enabled, and it pretty clearly is in line with the spec.

@milessabin
Contributor

Also I see no difference in behaviour on @retronym's new example in 2.12.x with the inference improvements enabled or disabled.

@SethTisue SethTisue changed the title from Clean up of code guarded by bare -Xexperimental to Make all -Xexperimental features available separately Dec 5, 2016
@SethTisue SethTisue changed the title from Make all -Xexperimental features available separately to Make -Xexperimental features available separately Dec 5, 2016
@fhalde fhalde added a commit to fhalde/scala that referenced this pull request Dec 22, 2016
@adriaanm @fhalde adriaanm + fhalde Revert existential infer part of #5376
It wasn't a good idea after all.

Also removed some tracing code that I cannot imagine
was ever used in a production compiler. It's still just
a recompile away.

Fixes scala/scala-dev#280
20bf651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment