Skip to content

Relax comparison between Null and reference types in explicit nulls #23308

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

Merged
merged 3 commits into from
Jun 7, 2025

Conversation

SuperCl4sh
Copy link
Contributor

An up-to-date version of #19258 with merge conflicts resolved.

@SuperCl4sh
Copy link
Contributor Author

cc @olhotak @HarrisL2

@Gedochao Gedochao requested a review from noti0na1 June 3, 2025 06:29
@SuperCl4sh SuperCl4sh force-pushed the updated-relaxed-equal-nulls branch from 04432cb to c909ba2 Compare June 3, 2025 19:47
@olhotak
Copy link
Contributor

olhotak commented Jun 6, 2025

Before this PR, all three errors in flow-match come from the typer phase:

-- [E007] Type Mismatch Error: tests/explicit-nulls/neg/flow-match.scala:5:15 --
5 |    case s2 => s2 // error
  |               ^^
  |               Found:    (s2 : String | Null)
  |               Required: String
  |
  | longer explanation available when compiling with `-explain`
-- [E172] Type Error: tests/explicit-nulls/neg/flow-match.scala:6:9 ------------
6 |    case null => "other" // error
  |         ^^^^
  |         Values of types Null and String cannot be compared with == or !=
-- [E172] Type Error: tests/explicit-nulls/neg/flow-match.scala:12:9 -----------
12 |    case null => "other" // error
   |         ^^^^
   |        Values of types Null and String cannot be compared with == or !=
3 errors found

After this PR, the two errors on lines 6 and 12 understandably go away.

The error on line 5 prevents the compiler from getting past typer, so match reachability checking never happens.

The test was not intended for match reachability checking but to test for the flow-sensitive analysis in typer.

We should split the test into two, one that errors only in typer, and one that tests only match reachability checking.

@olhotak
Copy link
Contributor

olhotak commented Jun 6, 2025

Here are the proposed two tests. Note that the second one needs to go in warn, not neg.

// Test flow-typing when NotNullInfos are from cases

object MatchTest {
  def f6(s: String | Null): String = s match {
    case s2 => s2 // error
    case s3 => s3 // OK since not null
  }

  def f7(s: String | Null): String = s match {
    case null => "other"
    case s3 => s3 // OK since not null
  }
}
// Test unreachable matches in presence of nulls

object MatchTest2 {
  def f6(s: String | Null): String = s match {
    case s2 => s2.nn
    case null => "other" // warn
    case s3 => s3 // warn
  }

  def f7(s: String | Null): String = s match {
    case null => "other"
    case null => "other" // warn
    case s3: String => s3
    case s4 => s4.nn // warn
  }
}

@@ -0,0 +1,54 @@
//> using options -Xfatal-warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in warn instead of enabling fatal warnings.

@SuperCl4sh SuperCl4sh force-pushed the updated-relaxed-equal-nulls branch 3 times, most recently from da9ae84 to 3a0d79f Compare June 7, 2025 02:24
@SuperCl4sh
Copy link
Contributor Author

OK I think all the comments have been addressed, apologies for the delay. Please let me know if I missed something.

Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

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

LGTM. @noti0na1 can you also have a look?

@SuperCl4sh SuperCl4sh force-pushed the updated-relaxed-equal-nulls branch from 3a0d79f to 297bed4 Compare June 7, 2025 04:23
@noti0na1 noti0na1 merged commit 49286a9 into scala:main Jun 7, 2025
29 checks passed
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.

4 participants