Refine overriding pairs in RefChecks #12982

odersky · 2021-06-29T21:38:00Z

We now exclude a pair of overriding / overridden symbols that
have a common subclass parent of the base class only under the
additional condition that their signatures also match when seen
as members of the parent class.

#12828 shows a case where this matters:

trait Foo[A]:
  def foo(x: A): Unit

trait Bar[A] extends Foo[A]:
  def foo(x: A & String): Unit = println(x.toUpperCase)

object Baz extends Bar[Int] // error overriding foo: incompatible type

@main def run() = Baz.foo(42)

When checking Baz, there is a common subclass parent (namely Bar),
but the signatures of the two foo definitions as seen from Bar are
different, so we cannot assume that the pair has already been checked.

Fixes #12828

@main

We now exclude a pair of overriding / overridden symbols that have a common subclass parent of the base class only under the additional condition that their signatures also match. scala#12828 shows a case where this matters: ```scala trait Foo[A]: def foo(x: A): Unit trait Bar[A] extends Foo[A]: def foo(x: A & String): Unit = println(x.toUpperCase) object Baz extends Bar[Int] // error overriding foo: incompatible type @main def run() = Baz.foo(42) ``` When checking `Baz`, there is a common subclass parent (namely `Bar`), but the signatures of the two `foo` definitions as seen from `Bar` are different, so we cannot assume that the pair has already been checked. Fixes scala#12828

odersky · 2021-06-29T21:39:11Z

test performance please

dottybot · 2021-06-29T21:41:05Z

performance test scheduled: 1 job(s) in queue, 0 running.

smarter · 2021-06-29T22:46:14Z

Before complicating OverridingPairs more I think it's worth considering how what we're doing differ from Scala 2 which is known to be more correct than us (#7551), namely we know that https://github.com/lampepfl/dotty/blob/master/tests/run/i5094.scala is compiling when it shouldn't because the final method SO is override by the method in SOIO. This was fixed in Scala 2 by scala/scala@cc55bd9 which says:

Subclassing on traits does not imply suffixing for their linearisations

And indeed if we change OverridingPairs#isSubParent to also check !parent.is(Trait), then i5094.scala stops compiling, and if we also disable the if (member.owner != clazz) { ... block in RefChecks#checkOverride, #12828 is also fixed (and the issue doesn't reappear when replacing the traits by abstract classes). By contrast the fix in this PR doesn't stop i5094.scala from compiling, so it's not clear to me it's the right way forward.

dottybot · 2021-06-29T22:52:42Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/12982/ to see the changes.

Benchmarks is based on merging with master (d96a500)

odersky · 2021-06-30T07:01:32Z

test performance please

dottybot · 2021-06-30T07:02:21Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2021-06-30T07:57:22Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/12982/ to see the changes.

Benchmarks is based on merging with master (f34a3bb)

odersky · 2021-06-30T09:21:52Z

@smarter Unfortunately, the situation between Scala 2 and 3 is not comparable since Scala 3 is signature based and Scala 2 isn't. So the algorithms are different. The problem with trait ordering in linearization can be fixed easily with another test in OverridingPairs.

But it turns out that the duplicate checks in checkAllOverride that looked redundant are not. They re-introduce the problem. Specifically, this one:

      // return if we already checked this combination elsewhere
      if (member.owner != clazz) {
        def deferredCheck = member.is(Deferred) || !other.is(Deferred)
        def subOther(s: Symbol) = s derivesFrom other.owner
        def subMember(s: Symbol) = s derivesFrom member.owner

        if (subOther(member.owner) && deferredCheck)
          //Console.println(infoString(member) + " shadows1 " + infoString(other) " in " + clazz);//DEBUG
          return

Here, if we make foo in Foo concrete, the condition kicks in and the pair is discarded even though the signatures are different. Hence, the test compiles and we get a crash at runtime.

But if Ii remove the early returnn in checkAllOverrides, stdlib213 does not compile anymore. It complains as follows:

[error] -- Error: /__w/dotty/dotty/community-build/community-projects/stdLib213/src/library/scala/collection/MapView.scala:19:6 
[error] 19 |trait MapView[K, +V]
[error]    |      ^
[error]    |trait MapView inherits conflicting members:
[error]    |  method concat in trait IterableOps of type [B >: (K, V)](suffix: IterableOnce[B]): scala.collection.View[B]  and
[error]    |  method concat in trait MapOps of type [V2 >: V](suffix: IterableOnce[(K, V2)]): scala.collection.View[(K, V2)]
[error]    |(Note: this can be resolved by declaring an override in trait MapView.);
[error]    |other members with override errors are:: method ++, method ++:

At first glance, this looks like a correct error, and might need a fix stdlib213.

EDIT: It's a correct error only if we decide that two members override if their signatures match. The last commits go into another direction: Decree that there's not a real override since the full parameter types do not match, and suppress the error message and also any bridge generations.

Remove illogical conditions that suppressed certain checks. Replace them by a separate condition that certain override checks are suppressed if the types of the members do not match (i.e. it's just the signatures that match but not the types). In that case, we have arguably overshot with declaring an override. Bridge generation is also suppressed in this case. Scala 2 behaves effectively the same way, by never taking signatures into account.

Refchecks now decouples overriding from signature matching. A signature match is in essence a pre-check that two members might override, but if the two members are methods we also need to compare their parameter types. This has three consequences: 1. Before emitting an override error, test that parameter types match. If not, we have a false override and no error should be reported. 2. Before emitting a bridge, do the same test and omit the generation if the test fails 3. When checking whether a class is fully implemented, check that any implementation also agrees completely in its parameter types.

odersky · 2021-06-30T15:35:08Z

Refchecks now decouples overriding from signature matching. A signature match is in essence a pre-check that two members might override, but if the two members are methods we also need to compare their parameter types. This has three consequences:

Before emitting an override error, test that parameter types match. If not, we have a false override and no error should be reported.
Before emitting a bridge, do the same test and omit the generation if the test fails
When checking whether a class is fully implemented, check that any implementation also agrees completely in its parameter types.

Apply linearization filter only to refchecks, not to other overriding pairs cursors.

odersky · 2021-06-30T22:14:51Z

test performance please

dottybot · 2021-06-30T22:16:27Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2021-06-30T23:27:50Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/12982/ to see the changes.

Benchmarks is based on merging with master (79fae19)

odersky · 2021-07-01T07:37:13Z

That was quite a rabbit hole.

smarter · 2021-07-02T09:32:41Z

compiler/src/dotty/tools/dotc/core/Denotations.scala

@@ -1021,7 +1021,7 @@ object Denotations {
     *  erasure (see i8615b, i9109b), Erasure takes care of adding any necessary
     *  bridge to make this work at runtime.
     */
-    def matchesLoosely(other: SingleDenotation)(using Context): Boolean =
+    def matchesLoosely(other: SingleDenotation, alwaysCompareParams: Boolean = false)(using Context): Boolean =


I think this should be called alwaysCompareTypes since Type#matches can also check the result type if we're after erasure.

smarter · 2021-07-02T09:34:30Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

+       *  If not we treat them as not a real override and don't issue certain
+       *  error messages. Also, bridges are not generated in this case.


Why not skip all error messages instead of only certain ones? Currently it looks like we still emit overrideAccessError and overrideTargetNameError but if these methods are not true override then I don't see why we need to.

In fact it's just the comment that is wrong. All errors go to overrideError which does the check whether it's a true override.

All errors go to overrideError which does the check whether it's a true override

In that case, why not immediately check for false overrides at the top of checkOverride before doing any of the checks?

Because the false overrides check is expensive and override checks are common while override errors are rare.

smarter

LGTM, great to see this on more solid footing!

julienrf · 2021-07-05T11:54:23Z

tests/neg/i11828.check

@@ -0,0 +1,8 @@
+-- Error: tests/neg/i12828.scala:7:7 -----------------------------------------------------------------------------------


I think this file should be removed

Remove 2 out of 3 conditions that should be redundant

e008f6e

odersky force-pushed the fix-12828 branch from 9a8f575 to e008f6e Compare June 30, 2021 09:30

odersky force-pushed the fix-12828 branch from dee1e03 to 8138eb4 Compare June 30, 2021 14:37

odersky added 3 commits June 30, 2021 17:47

Avoid rechecking already handled pairs only if in same order

de523b3

Apply linearization filter only to refchecks

8c405ac

Apply linearization filter only to refchecks, not to other overriding pairs cursors.

Fix test comment

a784915

odersky force-pushed the fix-12828 branch from e819415 to a784915 Compare June 30, 2021 21:53

odersky requested a review from smarter July 1, 2021 07:37

odersky assigned odersky and smarter and unassigned odersky Jul 1, 2021

smarter requested changes Jul 2, 2021

View reviewed changes

smarter assigned odersky and unassigned smarter Jul 2, 2021

Address review comments

c238f82

odersky assigned smarter and unassigned odersky Jul 2, 2021

bishabosha requested a review from smarter July 5, 2021 09:09

smarter linked an issue Jul 5, 2021 that may be closed by this pull request

Port RefChecks fixes from scalac related to linearization #7551

Closed

smarter approved these changes Jul 5, 2021

View reviewed changes

smarter merged commit 40c587f into scala:master Jul 5, 2021

smarter deleted the fix-12828 branch July 5, 2021 11:48

julienrf reviewed Jul 5, 2021

View reviewed changes

Kordyjan added this to the 3.0.2 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine overriding pairs in RefChecks #12982

Refine overriding pairs in RefChecks #12982

odersky commented Jun 29, 2021 •

edited

Loading

odersky commented Jun 29, 2021

dottybot commented Jun 29, 2021

smarter commented Jun 29, 2021

dottybot commented Jun 29, 2021

odersky commented Jun 30, 2021

dottybot commented Jun 30, 2021

dottybot commented Jun 30, 2021

odersky commented Jun 30, 2021 •

edited

Loading

odersky commented Jun 30, 2021

odersky commented Jun 30, 2021

dottybot commented Jun 30, 2021

dottybot commented Jun 30, 2021

odersky commented Jul 1, 2021

smarter Jul 2, 2021

smarter Jul 2, 2021 •

edited

Loading

odersky Jul 2, 2021

smarter Jul 4, 2021

odersky Jul 5, 2021

smarter left a comment

julienrf Jul 5, 2021

		* If not we treat them as not a real override and don't issue certain
		* error messages. Also, bridges are not generated in this case.

		@@ -0,0 +1,8 @@
		-- Error: tests/neg/i12828.scala:7:7 -----------------------------------------------------------------------------------

Refine overriding pairs in RefChecks #12982

Refine overriding pairs in RefChecks #12982

Conversation

odersky commented Jun 29, 2021 • edited Loading

odersky commented Jun 29, 2021

dottybot commented Jun 29, 2021

smarter commented Jun 29, 2021

dottybot commented Jun 29, 2021

odersky commented Jun 30, 2021

dottybot commented Jun 30, 2021

dottybot commented Jun 30, 2021

odersky commented Jun 30, 2021 • edited Loading

odersky commented Jun 30, 2021

odersky commented Jun 30, 2021

dottybot commented Jun 30, 2021

dottybot commented Jun 30, 2021

odersky commented Jul 1, 2021

smarter Jul 2, 2021

Choose a reason for hiding this comment

smarter Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

odersky Jul 2, 2021

Choose a reason for hiding this comment

smarter Jul 4, 2021

Choose a reason for hiding this comment

odersky Jul 5, 2021

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

julienrf Jul 5, 2021

Choose a reason for hiding this comment

odersky commented Jun 29, 2021 •

edited

Loading

odersky commented Jun 30, 2021 •

edited

Loading

smarter Jul 2, 2021 •

edited

Loading