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

-Yexplicit-nulls fails to detect null pointer dereference #21380

Closed
theosotr opened this issue Aug 13, 2024 · 3 comments · Fixed by #21389
Closed

-Yexplicit-nulls fails to detect null pointer dereference #21380

theosotr opened this issue Aug 13, 2024 · 3 comments · Fixed by #21389
Assignees
Labels
area:nullability area:private options Issues tied to -Y private/internal compiler settings. itype:bug
Milestone

Comments

@theosotr
Copy link

-Yexplicit-nulls fails to detect method call on a variable holding null.
This might be regression, as scalac v3.3.3 detects the error as expected.

Compiler version

3.4.2

Minimized code

@main def test() = {
  var x: String | Null = null
  if (false) {
    x = ""

  } else {
    x = ""
  }
  try {
    x = ""
    throw new Exception()
  }
  catch {
    case e: Exception => {
      x = null
    }
  }
  x.replace("", "") 
}

Output

The code compiles with -Yexplicit-nulls, but at runtime you get NPE

java.lang.NullPointerException
	at test$package$.test(test.scala:18)
	at test.main(test.scala:1)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at dotty.tools.runner.RichClassLoader$.run$extension$$anonfun$1(ScalaClassLoader.scala:36)
	at dotty.tools.runner.ScalaClassLoader$.asContext(ScalaClassLoader.scala:80)
	at dotty.tools.runner.RichClassLoader$.dotty$tools$runner$RichClassLoader$$$asContext$extension(ScalaClassLoader.scala:18)
	at dotty.tools.runner.RichClassLoader$.run$extension(ScalaClassLoader.scala:36)
	at dotty.tools.runner.CommonRunner.run(ObjectRunner.scala:23)
	at dotty.tools.runner.CommonRunner.run$(ObjectRunner.scala:13)
	at dotty.tools.runner.ObjectRunner$.run(ObjectRunner.scala:48)
	at dotty.tools.runner.CommonRunner.runAndCatch(ObjectRunner.scala:30)
	at dotty.tools.runner.CommonRunner.runAndCatch$(ObjectRunner.scala:13)
	at dotty.tools.runner.ObjectRunner$.runAndCatch(ObjectRunner.scala:48)
	at dotty.tools.MainGenericRunner$.run$1(MainGenericRunner.scala:215)
	at dotty.tools.MainGenericRunner$.process(MainGenericRunner.scala:270)
	at dotty.tools.MainGenericRunner$.main(MainGenericRunner.scala:281)
	at dotty.tools.MainGenericRunner.main(MainGenericRunner.scala)

Expectation

The code should have been rejected with

-- [E008] Not Found Error: test.scala:18:4 ----------------------------------------------------------------------------------------------------------------------------------------------------
18 |  x.replace("", "")
   |  ^^^^^^^^^
   |  value replace is not a member of String | Null.
   |  Since explicit-nulls is enabled, the selection is rejected because
   |  String | Null could be null at runtime.
   |  If you want to select replace without checking for a null value,
   |  insert a .nn before .replace or import scala.language.unsafeNulls.
1 error found
@theosotr theosotr added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 13, 2024
@noti0na1 noti0na1 self-assigned this Aug 13, 2024
@noti0na1
Copy link
Member

The result of bisect:

Last good release: 3.4.0-RC1-bin-20230929-8020677-NIGHTLY
First bad release: 3.4.0-RC1-bin-20231001-09ea77e-NIGHTLY

74f6851 is the first bad commit

Looks like a regression of #18206

@olhotak

@olhotak
Copy link
Contributor

olhotak commented Aug 14, 2024

Here's a simpler test that also fails (i.e. compiles successfully even though it shouldn't):

@main def test() = {
  var x: String | Null = null
  x = ""
  1 match {
    case 1 => x = null
  }
  x.replace("", "") // error
}

The problem is an inherent flaw in #18206. Before #18206, the compiler gives up analyzing any variable that has any assignments under match, case, or try. #18206 enables analysis for such variables. When those assignments are non-null (e.g. x = ""), this is fine, but the case when the assignments are null was missed in #18206. For that case, the typedMatch, typedCases, typedTry in Typer would need to be augmented to compute .withNotNullInfo in a similar way as typedIf and typedWhileDo.

olhotak added a commit to dotty-staging/dotty that referenced this issue Aug 14, 2024
@Gedochao Gedochao added the area:private options Issues tied to -Y private/internal compiler settings. label Aug 19, 2024
@Gedochao
Copy link
Contributor

Gedochao commented Aug 19, 2024

This might be regression, as scalac v3.3.3 detects the error as expected.

The result of bisect:

Last good release: 3.4.0-RC1-bin-20230929-8020677-NIGHTLY
First bad release: 3.4.0-RC1-bin-20231001-09ea77e-NIGHTLY

74f6851 is the first bad commit

Looks like a regression of #18206

@theosotr @noti0na1 side note: issues related to -Y* private options generally aren't treated as regressions, as that's not stable API, there's no strict guarantees for those.

@Gedochao Gedochao removed the stat:needs triage Every issue needs to have an "area" and "itype" label label Aug 19, 2024
noti0na1 pushed a commit to dotty-staging/dotty that referenced this issue Sep 9, 2024
@WojciechMazur WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024
WojciechMazur added a commit that referenced this issue Dec 4, 2024
…21380)" to LTS (#22121)

Backports #21389 to the 3.3.5.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:nullability area:private options Issues tied to -Y private/internal compiler settings. itype:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants