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.

Sign up for GitHub

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

Jump to bottom

-Yexplicit-nulls exhibits false negative when combining try/match #21619

Closed
theosotr opened this issue Sep 20, 2024 · 2 comments · Fixed by #21624
Closed

-Yexplicit-nulls exhibits false negative when combining try/match #21619

theosotr opened this issue Sep 20, 2024 · 2 comments · Fixed by #21624
Assignees
@noti0na1
Labels
area:nullability itype:bug

Comments

@theosotr
Copy link

Compiler version

3.6.0-RC1-bin-SNAPSHOT-git-e9d4831

Minimized code

Related to #21380

@main def test(): Unit = {
  var x: String | Null = null
  x = ""
  var i: Int = 1
  try {
    i match {
      case _ => {
        x = null
        throw new Exception()
      }
    }
    x = ""
  }
  catch {
    case (e: Exception) => {}
  }
  x.replace("", "")
}

Output

Compiling the program with -Yexplicit-nulls results in the following error at runtime

java.lang.NullPointerException: Cannot invoke "String.replace(java.lang.CharSequence, java.lang.CharSequence)" because "x" is null
	at test$package$.test(test.scala:17)
	at test.main(test.scala:1)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	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 program should have been rejected.
@theosotr theosotr added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Sep 20, 2024
@noti0na1
Copy link
Member

We should make NotNullInfo remembering the last info, and provide a way to collect refs ever being retracted.

cc @olhotak

@Gedochao Gedochao removed the stat:needs triage Every issue needs to have an "area" and "itype" label label Sep 20, 2024
@noti0na1 noti0na1 mentioned this issue Sep 21, 2024
Merged
@theosotr
Copy link
Author

theosotr commented Nov 14, 2024 •
edited
Loading

Here's another case that produces NPE without using try/catch.

@main def foo() = {
  var x: String | Null = ""
  var y: String = ""
  x = ""
  y = if (false) x else 1 match {
    case _ => {
      x = null
      y
    }
  }
  x.replace("", "")
}
java.lang.NullPointerException: Cannot invoke "String.replace(java.lang.CharSequence, java.lang.CharSequence)" because "x" is null
        at test$package$.foo(test.scala:11)
        at foo.main(test.scala:1)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        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)

noti0na1 added a commit that referenced this issue Dec 10, 2024
@noti0na1
Fix #21619: Refactor NotNullInfo to record every reference which is r…
ee0dd7a 
…etracted once. (#21624)

This PR improves the flow typing for returning and exceptions.

The `NotNullInfo` is defined as following now:

```scala
case class NotNullInfo(asserted: Set[TermRef] | Null, retracted: Set[TermRef]):
```

* `retracted` contains variable references that are ever assigned to
null;
* if `asserted` is not `null`, it contains `val` or `var` references
that are known to be not null, after the tree finishes executing
normally (non-exceptionally);
* if `asserted` is `null`, the tree is know to terminate, by throwing,
returning, or calling a function with `Nothing` type. Hence, it acts
like a universal set.

`alt` is defined as `<a1,r1>.alt(<a2,r2>) = <a1 intersect a2, r1 union
r2>`.

The difficult part is the `try ... catch ... finally ...`. We don't know
at which point an exception is thrown in the body, and the catch cases
may be not exhaustive, we have to collect any reference that is once
retracted.

Fix #21619
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@noti0na1 noti0na1

Labels
area:nullability itype:bug
Projects
None yet
Milestone
No milestone
3 participants
@theosotr @noti0na1 @Gedochao