Issue with macros and in-memory (virtual) source files

[vasilmkd]

Compiler version

All Scala 3 and Scala 2 versions.

Background information

IntelliJ IDEA is very particular when it comes to saving files to disk. IDEA likes to work with in-memory files and occasionally, in a very controlled manner, persist the files to disk.

In order to avoid more frequent than absolutely necessary saving to disk, I recently implemented compilation of in-memory files in IntelliJ IDEA, as part of this ticket, when using the Scala 3 compiler to provide error highlighting. This has been rolled out in an EAP version of IDEA 2024.2. If anyone wants to reproduce this project, it is enough to run IDEA 2024.2 EAP with Scala Plugin 2024.2.6 (or 2024.2.405+ nightly), clone https://github.com/scala/scala3-example-project and open the MySuite.scala MUnit test suite.

Initially, everything worked fine. But I didn't think to test it with sources that contain macro expansion.

Since then, there has been the following report.

When in-memory source files are used, the following MUnit macro implementation throws an NPE.

For easy reference, here's the exception:

Exception occurred while executing macro expansion. java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.toString()" because the return value of "scala.quoted.Quotes$reflectModule$SourceFileMethods.jpath(Object)" is null at munit.internal.MacroCompat$.locationImpl(MacroCompat. scala:18)

This specific MUnit Location macro is a perfectly well-written macro. In fact, the Scala 3 documentation shows the same idiom.

What I think the problem is

I searched for "def jpath" in the Scala 3 repo and found the following:

1. NoAbstractFile
2. VirtualDirectory
3. VirtualFile

all of which are implemented as:

  /** Returns null. */
  def jpath: JPath = null

To be fair, these are completely legitimate implementations of the dotty.tools.io.AbstractFile interface, which says:

  /** Returns the underlying Path if any and null otherwise. */
  def jpath: JPath

(in both cases, JPath is a type alias for java.nio.file.Path)

What I tried

I modified VirtualFile.scala in the Scala 3 repo to return:

class VirtualFile(val name: String, override val path: String) extends AbstractFile {
  ...
  def jpath: JPath = Paths.get(path)
  ...
}

I published a local snapshot and rebased https://github.com/scala/scala3-example-project on it, and the issue was gone.

What will we do

Obviously, I will have to disable in-memory sources and revert to always using file system sources for providing error highlighting using the compiler. I will keep the feature around with the hope of enabling it for future versions of Scala.

I will also open the same ticket in Scala 2, as it also contains the same null implementations. That ticket will most likely be just a reference to this one, as the relevant details are the same.

Discussion

How should this problem be handled? When a proper solution for this problem is attempted, it should also be covered by tests, not just for the regular compilation part, but also macro expansion, and any other places that might be using AbstractFile#jpath.

It also feels like the macro documentation should be revised, as it clearly shows unsafe usage of methods that are documented to be nullable.

Looking forward to a discussion.

Thank you in advance.

[bishabosha]

I would suggest that it is munit at fault - if the source file path is null then it should recover from this

[vasilmkd]

@bishabosha Can you please comment on the fact that the Scala 3 documentation shows the exact same code, which I presume the MUnit implementation is based on? Thanks.

https://docs.scala-lang.org/scala3/reference/metaprogramming/reflection.html#positions-1

[bishabosha]

Well that code is not great either - we now have an optional variant also - getJPath - then if that is empty then you can construct some value from path which won't be null but doesn't have to correspond to a real file

[vasilmkd]

Where exactly is getJPath defined? What about Scala 2?

[vasilmkd]

My technical lead @pavelfatin created a nice reproduction. You have to have MUnit 1.0.0 resolved locally:

dotr -cp ~/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scalameta/munit_3/1.0.0/munit_3-1.0.0.jar
scala> class FooTestSuite extends munit.FunSuite {
     |   test("foo") {
     |     assert(true)
     |   }
     | }
-- Error: 
 3 |    assert(true)
   |                ^
   |                Exception occurred while executing macro expansion.
   |                java.lang.NullPointerException
   |                    at munit.internal.MacroCompat$.locationImpl(MacroCompat.scala:18)
   |Inline stack trace
   |This location contains code that was inlined from MacroCompat.scala:11
1 error found

[bishabosha]

Where exactly is getJPath defined?

If I type getJPath into the API docs search bar, then I can see that it is an extension method in the same scope that jpath is defined: https://www.scala-lang.org/api/3.4.2/scala/quoted/Quotes$reflectModule$SourceFileMethods.html

What about Scala 2?

Nothing wrong with just wrapping the call in Option.apply?

[som-snytt]

I would expect code to use path uniformly. The fix on munit tries jpath and then uses path as a fallback, as though jpath were more authentic or reliable.

The Scala 2 usages by munit and sourcecode use path and are unaffected. There is no tempting jpath.

[vasilmkd]

@som-snytt Just to be clear, you're also advocating for making no changes in Scala 2 and 3, instead address specific issues in libraries as they arise?

That's totally fine, I just want to clarify things.

Do you see an issue with the fix in MUnit?
scalameta/munit#795
scalameta/munit#796

Thank you.

[som-snytt]

Yes, with respect to this issue.

The underlying problem is that the API is hard to use. But if the genie gave you only three wishes, would you use one on this problem?

[unkarjedy]

Can you please patch the examples using jpath at:

• https://docs.scala-lang.org/scala3/guides/migration/tutorial-macro-cross-building.html
• https://dotty.epfl.ch/docs/reference/metaprogramming/reflection.html
• https://docs.scala-lang.org/scala3/reference/metaprogramming/reflection.html

To avoid future issues related to following them in other libraries