Skip to content

Handle Scalac variance unsoundness with regards to constructors. #926

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

Merged
merged 9 commits into from
Nov 9, 2015

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 5, 2015

See SI-9549 for a summary. Review by @retronym

The idea is that whenever Dotty detects a migration problem under -language:Scala2,
it should issue a migration warning, so we know what needs to be rewritten.
The included test

     pos-special/variances-constr.scala

demonstrates an unsoundness in the variance checking of scalac.
Scalac excludes symbols owned by constructors from the
checking. This is unsound, as can be demonstrated by compiling the test
and observing output of the program run:

Exception in thread "main" java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
	at Test$.main(variances-constr.scala:17)
	at Test.main(variances-constr.scala)

Dotty allows this code only under -language:Scala2 and issues a migration warning.
@@ -500,7 +500,9 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling {
projection.name == tpnme.hkApply &&
!other.isHKApply &&
other.testLifted(projection.prefix.LambdaClass(forcing = true).typeParams,
if (inOrder) isSubType(projection.prefix, _) else isSubType(_, projection.prefix),
{ xx => println(i"test lifted with $xx")
Copy link
Contributor

Choose a reason for hiding this comment

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

println left in code.

@retronym
Copy link
Member

retronym commented Nov 6, 2015

LGTM.

I checked out this branch to test that:

class C[+A] { class Inner { def this(a: A) = this() } } // only allowed under the migration

I do see an seemingly spurious warning about procedure syntax, is that a known issue?

⚡ ./bin/dotc -migration -language:Scala2 sandbox/test.scala
sandbox/test.scala:1: warning: Procedure syntax no longer supported; `=' should be inserted here
class C[+A] { class Inner { def this(a: A) = this() } }
                                           ^
sandbox/test.scala:1: warning: According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:
covariant type A occurs in contravariant position in type A of value a
class C[+A] { class Inner { def this(a: A) = this() } }
                                       ^
two warnings found

This version is correctly allowed:

class C[+A] { def this(a: A) = this() }

During my testing, I also noticed another difference wrt scalac:

class C[A] { class A } // sandbox/test.scala:1: error: A is already defined as type A

@odersky
Copy link
Contributor Author

odersky commented Nov 6, 2015

The procedure syntax warning seems to be a bug. I'll follow up on this.

On Fri, Nov 6, 2015 at 10:07 PM, Jason Zaugg [email protected]
wrote:

LGTM.

I checked out this branch to test that:

class C[+A] { class Inner { def this(a: A) = this() } } // only allowed under the migration

I do see an seemingly spurious warning about procedure syntax, is that a
known issue?

⚡ ./bin/dotc -migration -language:Scala2 sandbox/test.scala
sandbox/test.scala:1: warning: Procedure syntax no longer supported; `=' should be inserted here
class C[+A] { class Inner { def this(a: A) = this() } }
^
sandbox/test.scala:1: warning: According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:
covariant type A occurs in contravariant position in type A of value a
class C[+A] { class Inner { def this(a: A) = this() } }
^
two warnings found

This version is allowed:

class C[+A] { def this(a: A) = this() }

During my testing, I also noticed another difference wrt scalac:

class C[A] { class A } // sandbox/test.scala:1: error: A is already defined as type A


Reply to this email directly or view it on GitHub
#926 (comment).

Martin Odersky
EPFL

odersky added a commit that referenced this pull request Nov 9, 2015
Handle Scalac variance unsoundness with regards to constructors.
@odersky odersky merged commit 9cbe994 into scala:master Nov 9, 2015
@allanrenucci allanrenucci deleted the stdlib-variance branch December 14, 2017 19:24
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.

4 participants