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 override containing type param in explicit nulls #13975

Merged
merged 7 commits into from
Feb 19, 2022

Conversation

noti0na1
Copy link
Member

@noti0na1 noti0na1 commented Nov 19, 2021

If a member signature contains type parameters when overriding a Java class, the check will still fail even if we retract SafeNulls from the ctx, because T | Null is not a subtype of T.

This fix will use a more relaxed approach by forcingNull being a bottom type during overriding check.

A type varibale T from Java is translated to T >: Nothing <: Any. However, null can always be a value of T for Java side. So the best solution here is to let Null be a subtype of non-primitive value types temporarily.

Example:

class D1[T <: AnyRef] extends Comparator[T]:
  override def compare(o1: T, o2: T): Int = 0

class D2[T <: AnyRef] extends Comparator[T]:
  override def compare(o1: T | Null, o2: T | Null): Int = 0

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.

I find it not very appealing to run a deep type map on types for a performance critical method like matches. Is there no better/faster way to achieve what we want? Maybe a change to matches in TypeComparer?

@noti0na1 noti0na1 force-pushed the explicit-nulls-more-relaxed-override branch from 23a52e7 to 10f13ea Compare February 4, 2022 07:23
@noti0na1 noti0na1 force-pushed the explicit-nulls-more-relaxed-override branch from 10f13ea to 247c6da Compare February 10, 2022 00:47
@noti0na1 noti0na1 requested a review from olhotak February 10, 2022 02:16
@michelou
Copy link
Contributor

In source file dotc/core/Mode.scala (line 128) I should read

val RelaxedOverriding: Mode = newMode(30, "RelaxedOverriding")

instead of

val RelaxedOverriding: Mode = newMode(30, "ForceInline")

@noti0na1 noti0na1 force-pushed the explicit-nulls-more-relaxed-override branch from 6bb73a5 to 939ec21 Compare February 18, 2022 18:02
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!

compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
@odersky odersky removed their assignment Feb 19, 2022
@noti0na1 noti0na1 merged commit 72acaef into scala:main Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants