Skip to content

Apply box adaptation when checking overrides #16479

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jan 13, 2023
Merged

Apply box adaptation when checking overrides #16479

merged 10 commits into from
Jan 13, 2023

Conversation

Linyxus
Copy link
Contributor

@Linyxus Linyxus commented Dec 8, 2022

In this PR, we allow box adaptation when checking the compatibility of overriding pairs in capture checking phase. For example, it allows the following code to compile:

class IO

abstract class A[X] {
  def foo(x: Unit): X
  def bar(op: X => Int): Int
}

class C

def test(io: {*} IO) = {
  class B extends A[{io} C] {  // X =:= {io} C
    def foo(x: Unit): {io} C = ???
    def bar(op: ({io} C) => Int): Int = 0
  }
}

The foo and bar in B are both valid overrides, since the types in B and A[{io} C] is compatible with box adaptation.

@Linyxus Linyxus force-pushed the box-adapt-override branch from f16bb13 to d24451f Compare December 8, 2022 15:32
@Linyxus Linyxus requested a review from odersky December 13, 2022 14:22
@Linyxus Linyxus self-assigned this Dec 13, 2022
@Linyxus Linyxus force-pushed the box-adapt-override branch 4 times, most recently from 5a5d767 to 97008df Compare December 19, 2022 12:56
@Linyxus Linyxus mentioned this pull request Dec 19, 2022
Closed
@Linyxus Linyxus assigned odersky and unassigned Linyxus Dec 23, 2022
odersky
odersky reviewed Jan 5, 2023
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM. Let's discuss the changes in baseType when I am back next week.

compiler/src/dotty/tools/dotc/core/SymDenotations.scala Outdated
case tp: CachedType => btrCache.contains(tp)
case _ => false
def record(tp: CachedType, baseTp: Type) = {
if (Stats.monitored) {
Stats.record("basetype cache entries")
if (!baseTp.exists) Stats.record("basetype cache NoTypes")
}
if (!tp.isProvisional)
if (!tp.isProvisional && !tp.isEventuallyCapturingType && !Setup.isDuringSetup)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to optimize this better. I am worried that (1) the tests here and one line 2143 are too expensive and (2) that we end up caching not enough types, which will also make computation slow.

Why do we need Does to exclude EventuallyCapturingTypes? Maybe excluding CapturingTypes is enough? That would have the advantage that all other phases except CheckCaptures could cache types with capture annotations.

Maybe there could be a method isUncachable in CapturingTypes companion object that does these tests?

def isUncachable(tp: Type)(using Context) = ...

Then we'd not have to burden SymDenotations with more details about capture checking.

compiler/src/dotty/tools/dotc/typer/RefChecks.scala
@@ -290,8 +297,11 @@ object RefChecks {
* TODO check that classes are not overridden
* TODO This still needs to be cleaned up; the current version is a straight port of what was there
* before, but it looks too complicated and method bodies are far too large.
*
* @param makeOverridePairsChecker A function for creating a OverridePairsChecker instance
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might be able to streamline this, maybe by moving checkOverrides into OverridingPairsChecker. But we can do that later after this PR is merged.

Linyxus added 9 commits January 8, 2023 23:37
@Linyxus
parameterize checkAllOverrides with a subtype checker
caebd00
@Linyxus
apply box adaptation when checking the compatibility of overrides
243037f
@Linyxus
ignore capture annotations in abstract class checks
302f670
@Linyxus
do not dealias FromJavaObject during box adaptation
1467e5a
@Linyxus
fix cc compiler to pass override checks
59484f4
@Linyxus
refactor box adaptation for override checking
ac5d92e 
- make OverridingPairsChecker extensible, with the method for comparing subtype
  being abstract, and extend it with the box-adapt-enabled subtyping in CC

- delay override checking after checking captures

- delegate captured references of methods to the class
@Linyxus
fix baseType caching during capture checking
7e74341 
- disable caching during Setup
- never cache capturing types
@Linyxus
several fixes to CC compiler
c1753fe
@Linyxus
add a field for subtype checker to avoid creating closures repeatedly
b9b4767
@Linyxus Linyxus force-pushed the box-adapt-override branch 2 times, most recently from adefc4a to 4038b4b Compare January 9, 2023 17:37
@odersky
Copy link
Contributor

odersky commented Jan 10, 2023

test performance please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with main (4fa0715)

@Linyxus Linyxus force-pushed the box-adapt-override branch from 4038b4b to 5a97700 Compare January 12, 2023 14:31
@Linyxus Linyxus force-pushed the box-adapt-override branch from 5a97700 to 9dd4f52 Compare January 12, 2023 14:31
@Linyxus
Copy link
Contributor Author

Linyxus commented Jan 12, 2023

From the test results it seems that this PR won't slow the compiler down. I just force pushed to fix one typo in the comments. Shall we get it merged? @odersky

@odersky
Copy link
Contributor

odersky commented Jan 12, 2023

Yes, ready to merge.

@odersky odersky enabled auto-merge January 12, 2023 14:52
odersky
odersky approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Looks good now!

@odersky odersky merged commit f96b29e into scala:main Jan 13, 2023
@odersky odersky deleted the box-adapt-override branch January 13, 2023 14:35
@Linyxus Linyxus mentioned this pull request Feb 6, 2023
Closed
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Linyxus @odersky @dottybot @Kordyjan