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

enable -Yexplicit-nulls for scala3-library #13729

Merged
merged 2 commits into from Nov 8, 2021

Conversation

olhotak
Copy link
Contributor

@olhotak olhotak commented Oct 11, 2021

No description provided.

@olhotak olhotak requested a review from noti0na1 October 11, 2021 10:52
Copy link

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Comments while reading out of curiosity

true
}
}
.map(_.get(null).asInstanceOf[sun.misc.Unsafe])
.map(_.nn.get(null).asInstanceOf[sun.misc.Unsafe])
Copy link

Choose a reason for hiding this comment

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

.nn.get(null) reads strange to me - it looks redundant contradictory?

Copy link
Member

Choose a reason for hiding this comment

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

Here classOf[sun.misc.Unsafe].getDeclaredFields.nn is an Array[Field | Null] and then after calling find is an Option[Field | Null], so calling map would require asserting the inner value is also not null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.map is being called on Option[Field | Null].

The type of _ is thus Field | Null.

The type of _.nn is thus java.lang.reflect.Field. This type has a get(Object) method to read the value of the field. The API of this method says that for a static field, the parameter of type Object is ignored and should be null.

Choose a reason for hiding this comment

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

OK, thanks for the explanation!

Comment on lines -18 to +19
if (x == null) throw new NullPointerException("tried to cast away nullability, but value is null")
val isNull = x == null
if (isNull) throw new NullPointerException("tried to cast away nullability, but value is null")

Choose a reason for hiding this comment

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

What is the benefit of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A workaround for #7882

@noti0na1 noti0na1 self-assigned this Oct 12, 2021
@@ -328,54 +328,59 @@ object IArray:
extension [T, U >: T: ClassTag](x: T)
def +:(arr: IArray[U]): IArray[U] = genericArrayOps(arr).prepended(x)

// For backwards compatibility with code compiled without -Yexplicit-nulls
private def mapNull[A, B](a: A, f: =>B): B =
if((a: A|Null) == null) null.asInstanceOf[B] else f
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't use eq for nulls anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently not. The motivation comes from:
https://contributors.scala-lang.org/t/wip-scala-with-explicit-nulls/2761/4

I don't think this was discussed widely so if people feel strongly about it, we can discuss it.

Co-authored-by: Nicolas Stucki <nicolas.stucki@gmail.com>
@olhotak olhotak changed the title WIP enable -Yexplicit-nulls for scala3-library enable -Yexplicit-nulls for scala3-library Oct 19, 2021
Copy link
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -328,54 +328,59 @@ object IArray:
extension [T, U >: T: ClassTag](x: T)
def +:(arr: IArray[U]): IArray[U] = genericArrayOps(arr).prepended(x)

// For backwards compatibility with code compiled without -Yexplicit-nulls
private inline def mapNull[A, B](a: A, inline f: B): B =
if((a: A|Null) == null) null.asInstanceOf[B] else f
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if A = X | Null (for some type X)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That shouldn't be a problem: an expression of type X | Null | Null is still allowed to be compared with null.

@olhotak olhotak merged commit ae0f828 into scala:master Nov 8, 2021
@olhotak olhotak deleted the wip-expnull-library branch November 8, 2021 08:43
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Nov 10, 2021
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Dec 5, 2021
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Dec 5, 2021
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Dec 9, 2021
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Jan 13, 2022
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Jan 22, 2022
timotheeandres added a commit to dotty-staging/dotty that referenced this pull request Feb 6, 2022
@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 2023
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.

None yet

7 participants