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

Fix 14682: fix overriding check in mergeSingleDenot #14755

Merged
merged 3 commits into from Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/core/Denotations.scala
Expand Up @@ -469,11 +469,12 @@ object Denotations {
else if sym1.is(Method) && !sym2.is(Method) then 1
else 0

val relaxedOverriding = ctx.explicitNulls && (sym1.is(JavaDefined) || sym2.is(JavaDefined))
val matchLoosely = sym1.matchNullaryLoosely || sym2.matchNullaryLoosely

if symScore <= 0 && info2.overrides(info1, matchLoosely, checkClassInfo = false) then
if symScore <= 0 && info2.overrides(info1, relaxedOverriding, matchLoosely, checkClassInfo = false) then
denot2
else if symScore >= 0 && info1.overrides(info2, matchLoosely, checkClassInfo = false) then
else if symScore >= 0 && info1.overrides(info2, relaxedOverriding, matchLoosely, checkClassInfo = false) then
denot1
else
val jointInfo = infoMeet(info1, info2, safeIntersection)
Expand Down
20 changes: 12 additions & 8 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Expand Up @@ -1071,21 +1071,25 @@ object Types {

/** Is this type a legal type for member `sym1` that overrides another
* member `sym2` of type `that`? This is the same as `<:<`, except that
* @param relaxedCheck if true type `Null` becomes a subtype of non-primitive value types in TypeComparer.
* @param matchLoosely if true the types `=> T` and `()T` are seen as overriding each other.
* @param checkClassInfo if true we check that ClassInfos are within bounds of abstract types
*/
final def overrides(that: Type, matchLoosely: => Boolean, checkClassInfo: Boolean = true)(using Context): Boolean = {
final def overrides(that: Type, relaxedCheck: Boolean, matchLoosely: => Boolean, checkClassInfo: Boolean = true)(using Context): Boolean = {
def widenNullary(tp: Type) = tp match {
case tp @ MethodType(Nil) => tp.resultType
case _ => tp
}
!checkClassInfo && this.isInstanceOf[ClassInfo]
|| (this.widenExpr frozen_<:< that.widenExpr)
|| matchLoosely && {
val this1 = widenNullary(this)
val that1 = widenNullary(that)
((this1 `ne` this) || (that1 `ne` that)) && this1.overrides(that1, false, checkClassInfo)
}
val overrideCtx = if relaxedCheck then ctx.relaxedOverrideContext else ctx
inContext(overrideCtx) {
!checkClassInfo && this.isInstanceOf[ClassInfo]
|| (this.widenExpr frozen_<:< that.widenExpr)
|| matchLoosely && {
val this1 = widenNullary(this)
val that1 = widenNullary(that)
((this1 `ne` this) || (that1 `ne` that)) && this1.overrides(that1, relaxedCheck, false, checkClassInfo)
}
}
}

/** Is this type close enough to that type so that members
Expand Down
8 changes: 3 additions & 5 deletions compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala
Expand Up @@ -217,11 +217,9 @@ object OverridingPairs:
else
// releaxed override check for explicit nulls if one of the symbols is Java defined,
// force `Null` to be a subtype of non-primitive value types during override checking.
val overrideCtx = if ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined))
then ctx.relaxedOverrideContext else ctx
val relaxedOverriding = ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined))
member.name.is(DefaultGetterName) // default getters are not checked for compatibility
|| memberTp.overrides(otherTp,
member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack
)(using overrideCtx)
|| memberTp.overrides(otherTp, relaxedOverriding,
member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack)

end OverridingPairs
5 changes: 2 additions & 3 deletions compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala
Expand Up @@ -113,9 +113,8 @@ object ResolveSuper {
// Since the super class can be Java defined,
// we use relaxed overriding check for explicit nulls if one of the symbols is Java defined.
// This forces `Null` to be a subtype of non-primitive value types during override checking.
val overrideCtx = if ctx.explicitNulls && (sym.is(JavaDefined) || acc.is(JavaDefined))
then ctx.relaxedOverrideContext else ctx
if !otherTp.overrides(accTp, matchLoosely = true)(using overrideCtx) then
val relaxedOverriding = ctx.explicitNulls && (sym.is(JavaDefined) || acc.is(JavaDefined))
if !otherTp.overrides(accTp, relaxedOverriding, matchLoosely = true) then
report.error(IllegalSuperAccessor(base, memberName, targetName, acc, accTp, other.symbol, otherTp), base.srcPos)
bcs = bcs.tail
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Expand Up @@ -767,7 +767,7 @@ object RefChecks {
for (mbrd <- self.member(name).alternatives) {
val mbr = mbrd.symbol
val mbrType = mbr.info.asSeenFrom(self, mbr.owner)
if (!mbrType.overrides(mbrd.info, matchLoosely = true))
if (!mbrType.overrides(mbrd.info, relaxedCheck = false, matchLoosely = true))
report.errorOrMigrationWarning(
em"""${mbr.showLocated} is not a legal implementation of `$name` in $clazz
| its type $mbrType
Expand Down
3 changes: 1 addition & 2 deletions compiler/test/dotty/tools/dotc/CompilationTests.scala
Expand Up @@ -52,7 +52,6 @@ class CompilationTests {
),
compileFile("tests/pos-special/typeclass-scaling.scala", defaultOptions.and("-Xmax-inlines", "40")),
compileFile("tests/pos-special/i7296.scala", defaultOptions.and("-source", "future", "-deprecation", "-Xfatal-warnings")),
compileFile("tests/pos-special/notNull.scala", defaultOptions.and("-Yexplicit-nulls")),
compileDir("tests/pos-special/adhoc-extension", defaultOptions.and("-source", "future", "-feature", "-Xfatal-warnings")),
compileFile("tests/pos-special/i7575.scala", defaultOptions.andLanguageFeature("dynamics")),
compileFile("tests/pos-special/kind-projector.scala", defaultOptions.and("-Ykind-projector")),
Expand Down Expand Up @@ -137,7 +136,6 @@ class CompilationTests {
compileFilesInDir("tests/neg-custom-args/erased", defaultOptions.and("-language:experimental.erasedDefinitions")),
compileFilesInDir("tests/neg-custom-args/allow-double-bindings", allowDoubleBindings),
compileFilesInDir("tests/neg-custom-args/allow-deep-subtypes", allowDeepSubtypes),
compileFilesInDir("tests/neg-custom-args/explicit-nulls", defaultOptions.and("-Yexplicit-nulls")),
compileFilesInDir("tests/neg-custom-args/no-experimental", defaultOptions.and("-Yno-experimental")),
compileDir("tests/neg-custom-args/impl-conv", defaultOptions.and("-Xfatal-warnings", "-feature")),
compileDir("tests/neg-custom-args/i13946", defaultOptions.and("-Xfatal-warnings", "-feature")),
Expand Down Expand Up @@ -249,6 +247,7 @@ class CompilationTests {
compileFilesInDir("tests/explicit-nulls/pos-separate", explicitNullsOptions),
compileFilesInDir("tests/explicit-nulls/pos-patmat", explicitNullsOptions and "-Xfatal-warnings"),
compileFilesInDir("tests/explicit-nulls/unsafe-common", explicitNullsOptions and "-language:unsafeNulls"),
compileFile("tests/explicit-nulls/pos-special/i14682.scala", explicitNullsOptions and "-Ysafe-init"),
)
}.checkCompile()

Expand Down
@@ -1,27 +1,27 @@
-- [E007] Type Mismatch Error: tests/neg-custom-args/explicit-nulls/byname-nullables.scala:19:24 -----------------------
-- [E007] Type Mismatch Error: tests/explicit-nulls/neg/byname-nullables.scala:19:24 -----------------------------------
19 | if x != null then f(x) // error: f is call-by-name
| ^
| Found: (x : String | Null)
| Required: String
|
| longer explanation available when compiling with `-explain`
-- Error: tests/neg-custom-args/explicit-nulls/byname-nullables.scala:43:32 --------------------------------------------
-- Error: tests/explicit-nulls/neg/byname-nullables.scala:43:32 --------------------------------------------------------
43 | if x != null then f(identity(x), 1) // error: dropping not null check fails typing
| ^^^^^^^^^^^
| This argument was typed using flow assumptions about mutable variables
| but it is passed to a by-name parameter where such flow assumptions are unsound.
| Wrapping the argument in `byName(...)` fixes the problem by disabling the flow assumptions.
|
| `byName` needs to be imported from the `scala.compiletime` package.
-- Error: tests/neg-custom-args/explicit-nulls/byname-nullables.scala:68:24 --------------------------------------------
-- Error: tests/explicit-nulls/neg/byname-nullables.scala:68:24 --------------------------------------------------------
68 | if x != null then f(x, 1) // error: dropping not null check typechecks OK, but gives incompatible result type
| ^
| This argument was typed using flow assumptions about mutable variables
| but it is passed to a by-name parameter where such flow assumptions are unsound.
| Wrapping the argument in `byName(...)` fixes the problem by disabling the flow assumptions.
|
| `byName` needs to be imported from the `scala.compiletime` package.
-- [E134] Type Error: tests/neg-custom-args/explicit-nulls/byname-nullables.scala:81:22 --------------------------------
-- [E134] Type Error: tests/explicit-nulls/neg/byname-nullables.scala:81:22 --------------------------------------------
81 | if x != null then f(byName(x), 1) // error: none of the overloaded methods match argument types
| ^
| None of the overloaded alternatives of method f in object Test7 with types
Expand Down
@@ -1,4 +1,4 @@
-- Error: tests/neg-custom-args/explicit-nulls/byname-nullables1.scala:10:6 --------------------------------------------
-- Error: tests/explicit-nulls/neg/byname-nullables1.scala:10:6 --------------------------------------------------------
10 | f(x.fld != null) // error
| ^^^^^^^^^^^^^
| This argument was typed using flow assumptions about mutable variables
Expand Down
@@ -1,18 +1,18 @@
-- [E134] Type Error: tests/neg-custom-args/explicit-nulls/i7883.scala:6:11 --------------------------------------------
-- [E134] Type Error: tests/explicit-nulls/neg/i7883.scala:6:11 --------------------------------------------------------
6 | case r(hd, tl) => Some((hd, tl)) // error // error // error
| ^
| None of the overloaded alternatives of method unapplySeq in class Regex with types
| (m: scala.util.matching.Regex.Match): Option[List[String]]
| (c: Char): Option[List[Char]]
| (s: CharSequence): Option[List[String]]
| match arguments (String | Null)
-- [E006] Not Found Error: tests/neg-custom-args/explicit-nulls/i7883.scala:6:30 ---------------------------------------
-- [E006] Not Found Error: tests/explicit-nulls/neg/i7883.scala:6:30 ---------------------------------------------------
6 | case r(hd, tl) => Some((hd, tl)) // error // error // error
| ^^
| Not found: hd
|
| longer explanation available when compiling with `-explain`
-- [E006] Not Found Error: tests/neg-custom-args/explicit-nulls/i7883.scala:6:34 ---------------------------------------
-- [E006] Not Found Error: tests/explicit-nulls/neg/i7883.scala:6:34 ---------------------------------------------------
6 | case r(hd, tl) => Some((hd, tl)) // error // error // error
| ^^
| Not found: tl
Expand Down
27 changes: 27 additions & 0 deletions tests/explicit-nulls/pos-special/i14682.scala
@@ -0,0 +1,27 @@
class C1:
sealed abstract class Name {
type ThisName <: Name
def compareTo(that: ThisName): Int = ???
}

class LocalName extends Name with Comparable[LocalName] {
type ThisName = LocalName
}

val localName = LocalName()
println(localName)
var count = 0

class C2:
sealed abstract class Name {
type ThisName <: Name
def compareTo(that: ThisName | Null): Int = ???
}

class LocalName extends Name with Comparable[LocalName] {
type ThisName = LocalName
}

val localName = LocalName()
println(localName)
var count = 0
File renamed without changes.