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

Null checks with pattern matching are not reflected to the return type with -Yexplicit-nulls #11967

Closed
lolgab opened this issue Apr 1, 2021 · 4 comments · Fixed by #18212
Closed

Comments

@lolgab
Copy link
Contributor

lolgab commented Apr 1, 2021

Compiler version

Scala 3.0.0-RC2 with -Yexplicit-nulls

Minimized code

// This works correctly
def f1(s: String | Null): String = if(s == null) "other" else s

// This fails to compile
def f(s: String | Null): String = s match {
  case null => "other"
  case s => s
}

Output

  case s => s
            ^
            Found:    (s : String | Null)
            Required: String

Expectation

My expectation is that what happens with def f(s: String | Null): String = if(s == null) "other" else s should apply to pattern matching too.

I couldn't find any open issue treating this, so I opened one.

Thank you.

@noti0na1 noti0na1 self-assigned this Apr 1, 2021
@noti0na1
Copy link
Member

noti0na1 commented Apr 1, 2021

Currently, the flow typing of explicit nulls only detects the NotNull cases in pattern matching (see Nullables.matchesNotNull).

For example:

val s: String | Null = ???
s match {
  case _: String => println(s.length)
  case _ => println(0)
}

I think it would be better to detect the null case as well.

@noti0na1
Copy link
Member

noti0na1 commented Apr 3, 2021

Sorry, I made a mistake. The null case is indeed detected by flow typing.
In your example, the s at the case position is new variable, it will still have the original type.

If you write it in this way, then it can be compiled.

def f(s: String | Null): String = s match {
  case null => "other"
  case _ => s
}

@noti0na1 noti0na1 linked a pull request Apr 3, 2021 that will close this issue
@lolgab
Copy link
Contributor Author

lolgab commented Apr 3, 2021

In your example, the s at the case position is new variable, it will still have the original type.

It is a new variable yes, but it's defined after the null is already matched, so in theory it's type should be String, not String | Null, right?

@noti0na1
Copy link
Member

noti0na1 commented Apr 3, 2021

The variables binded by the patterns are not handled by flow typing now. I agree we should consider them as well.

@noti0na1 noti0na1 removed a link to a pull request Apr 27, 2021
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 14, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 17, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 19, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 21, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 21, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 21, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 24, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 24, 2023
olhotak added a commit to dotty-staging/dotty that referenced this issue Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants