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

jpath in the reflection API can return null but this isn't documented anywhere #13007

Closed
smarter opened this issue Jul 2, 2021 · 1 comment · Fixed by #13177
Closed

jpath in the reflection API can return null but this isn't documented anywhere #13007

smarter opened this issue Jul 2, 2021 · 1 comment · Fixed by #13177

Comments

@smarter
Copy link
Member

smarter commented Jul 2, 2021

jpath in https://github.com/lampepfl/dotty/blob/147e9baf778a24f9f363ecf8e3898d69c40421ee/library/src/scala/quoted/Quotes.scala#L4154-L4155 is implemented as self.file.jpath which can be null (for example when using the REPL because file will be a VirtualFile).

We should update the API documentation to mention this fact, but I think it'd be even better to deprecate this method and replace it by a method that returns an Option, this would avoid mistakes like https://github.com/scalatest/scalatest/blob/e6e656bac6e54be2a9ae4f286a3727714ac09d1e/dotty/core/src/main/scala/org/scalatest/AssertionsMacro.scala#L53 which lead to the scalatest assert macro throwing an NPE from the REPL:

$ cs launch scala3-repl:3.0.0 -- -classpath "$(cs fetch -p org.scalatest:scalatest_3:3.2.9)"
scala> org.scalatest.Assertions.assert(List(3, 2, 1).sorted == List(1, 2, 3))
1 |org.scalatest.Assertions.assert(List(3, 2, 1).sorted == List(1, 2, 3))
  |^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |Exception occurred while executing macro expansion.
  |java.lang.NullPointerException
  |     at org.scalatest.AssertionsMacro$.transform(AssertionsMacro.scala:53)
  |     at org.scalatest.AssertionsMacro$.assert(AssertionsMacro.scala:33)
  |
  | This location contains code that was inlined from rs$line$4:1
@smarter
Copy link
Member Author

smarter commented Jul 2, 2021

Looks like the same issue exists in munit: https://github.com/scalameta/munit/blob/be448cb039fee43a97b80c2b94cf89e2c39dba75/munit/shared/src/main/scala-3/munit/internal/MacroCompat.scala#L18
There might be many macros broken like this, so deprecation sounds like a good idea. We should also expose SourceFile#name so it can be accessed without going through jpath

@anatoliykmetyuk anatoliykmetyuk added Spree Suitable for a future Spree area:documentation labels Jul 20, 2021
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 28, 2021
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 28, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 28, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 28, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 28, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 29, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jul 30, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
@dwijnand dwijnand removed the Spree Suitable for a future Spree label Aug 2, 2021
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Aug 17, 2021
And deprecate SourceFile.jpath

Fixes scala#13007
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 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.

5 participants