Skip to content

warn about unnecessary uses of .nn #23327

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

olhotak
Copy link
Contributor

@olhotak olhotak commented Jun 6, 2025

Issue warnings when .nn is called on a receiver that is already not null or when the expected type of the result of .nn admits null.

cc @noti0na1

@olhotak olhotak requested a review from noti0na1 June 6, 2025 23:33
@olhotak
Copy link
Contributor Author

olhotak commented Jun 7, 2025

I can't figure out the bootstrapped test failures. It seems to be a Heisenbug.

Locally, if I do scala3-compiler-bootstrapped/testCompilation after rm -rf out; sbt clean, sometimes it fully passes and sometimes every test fails.

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.

Otherwise LGTM!

Comment on lines 54 to 63
def admitsNull(using Context): Boolean = {
self.isNullType || self.isAny || (self match
case OrType(l, r) => r.admitsNull || l.admitsNull
case AndType(l, r) => r.admitsNull && l.admitsNull
case TypeBounds(lo, hi) => lo.admitsNull
case FlexibleType(lo, hi) => true
case tp: TypeProxy => tp.underlying.admitsNull
case _ => false
)
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to delete the case tp: TypeProxy, since it will essentially widen a type.
Let's also move it next to isNotNull because of the similarity.

def admitsNull(using Context): Boolean =
  self.isNullType || self.isAny || self.dealias match
    case OrType(l, r) => r.admitsNull || l.admitsNull
    case AndType(l, r) => r.admitsNull && l.admitsNull
    case tp: TypeBounds => tp.lo.admitsNull
    case _: FlexibleType => true
    case _ => false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the TypeProxy case, the following test fails (no warning):

def f6[T >: String|Null](s: String|Null): T = s.nn // warn

Since the test is approximately NullType <:< tp, what's the downside of widening tp if the original narrow tp fails the test. (I can see that widening eagerly before the match would cause us to miss some cases.)

Copy link
Member

Choose a reason for hiding this comment

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

Will it be enough if we add a case for TypeRef and recur on its info?

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 does cover this f6 test case, but would miss other cases such as AnnotatedType.

Do you have a particular example in mind where TypeProxy would do the wrong thing?

@olhotak olhotak force-pushed the warn-unnecessary-nn branch from 608a506 to 0f76b83 Compare June 9, 2025 14:55
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.

2 participants