Support sbt 1.4 virtual files when displaying error source #10648

mkurz · 2021-01-22T10:24:35Z

Backport for 2.8.x: #10649
Backport for 2.7.x: #10650

sbt 1.4 introduced a virtual file system it is using internally. So instead of Java File objects, Play now receives BasicVirtualFileRef and MappedVirtualFile objects, which both are subclasses of the VirtualFileRef interface.
Fortunately, that VirtualFileRef interface has methods that, with some reflection kung fu, we can use to figure out the correct file system path which we want to display on a Play error page if an exception occured: id() and names().
Such virtual file objects wrap a path in two possible ways:

If the file is located within the root (or base) project, its path is prefixed with ${BASE} (like ${BASE}/app/controllers/HomeController.scala
If the file is located outside of the root (or base) project, its path is not prefixed, but absolute (like /home/user/my_sub_project/src/main/scala/Foo.scala) That is the case for example with sub-projects that are not contained within the base project.

I also added scripted tests to make sure future changes or sbt upgrades that break things will be noticed asap.

BTW: I also went that annoying extra kilometer and did test this patch and the scripted tests on Microsoft® Windows 10 to make sure that file system path stuff also works with these obscure backslash and c: style of doing things as well.

Making-of:
This pull request is inspired by the reflection porn done in lombok to make it support sbt's virtual file system.
Originally I submitted a patch (projectlombok/lombok#2643) which was refused (because it was just a workaround) and instead the maintainer did his thing and came up with a reflection heavy commit that actually worked and did fix that bug. However soon I ran into another problem because files outside of a root project did not work yet, so I came up with an additional fix.

Oh and yes, along the way I discovered another change of behaviour ins sbt: sbt/sbt#6275 But that is not that relevant for this pull request here.

So yeah, as you can see, sbt 1.4 already caused me some headache...

jtjeferreira

LGTM, just some minor comments.

However what about requiring sbt 1.4+ in the next release of play so that we could remove the reflection stuff?

jtjeferreira · 2021-01-22T13:36:59Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+              case vf => { // sbt 1.4+ virtual file, see #10486
+                vf.getClass.getSimpleName match {
+                  case "BasicVirtualFileRef" | "MappedVirtualFile" => {
+                    val names = vf.getClass.getMethod("names").invoke(vf).asInstanceOf[Array[String]]


Would be worth to store the result of getClass.getMethod("names") somewhere, so we don't need to pay this penalty for every file on every compilation?

Alternatively, would be worth to implement this with structural typing? I know the reflection is still there, but...

..., so we don't need to pay this penalty for every file on every compilation?

This code isn't called for every file on every compilation, but just for one file (the source file that caused the exception) and only when a runtime error occurs and only in dev mode (when the play error page pops up in your browser).
So I don't think there is the need for any caching, basically there is no penalty. It would just complicate the code, plus I think the cache would be gone immediately anyway because after an exception in dev mode the server restarts anyway...

Alternatively, would be worth to implement this with structural typing? I know the reflection is still there, but...

You mean by instead of checking the class names, by observing if a certain methods with certain parameters and with a certain return type exists and if everything matches, then call it? I think my solution is good enough, I guess (or hope) sbt won't change the class and methods names on every minor release, so I think it's good enough for now.

AFAICT, this method sourceMap is called on play.sbt.run.PlayReload#compile which happens for each compilation... Then eventually the resulting Map from this, will be used on play.runsupport.Reloader#findSource...

However looking again at the stack trace from #10486, you are right and the function inside the flatMap is only called on findSource. I think this is just luck because analysis.relations.classes.reverseMap is lazy (you can see in the exception that it is a MappedValues) .

That said forget my suggestion, because I think this might need proper profiling to see if it is a problem in a big project or not...

Agree. Probably it's luck, I didn't really check when this method is called where, I just debugged and it was always only called when runtime exceptions occured.
Anyway, if the reflection solution will ever be a problem performance wise, we can soon just require sbt 1.4 for an upcoming release and replace the reflection code with the correct implementation. So don't think this is worth anymore effort right now.

jtjeferreira · 2021-01-22T13:38:21Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+                      Paths.get(names.drop(1).head, names.drop(2): _*)
+                    } else {
+                      // It's an absolute path, sbt uses them e.g. for subprojects located outside of the base project
+                      val id = vf.getClass.getMethod("id").invoke(vf).asInstanceOf[String]


Same as above...

Same answer as above...

jtjeferreira · 2021-01-22T13:46:22Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+                      val prefix = "file://" +
+                        (if (!id.startsWith("/")) {
+                           "/" // In Windows the sbt virtual file id does not start with a slash, but absolute paths in Java URIs need that
+                         } else "")
+                      // The URI will be like file:///home/user/project/SomeClass.scala (Linux/Mac) or file:///C:/Users/user/project/SomeClass.scala (Windows)
+                      Paths.get(URI.create(s"$prefix$id"));


I tried to find something to avoid this logic in https://github.com/sbt/io/blob/develop/io/src/main/scala/sbt/io/IO.scala but no luck.

mkurz · 2021-01-22T15:16:27Z

@jtjeferreira

However what about requiring sbt 1.4+ in the next release of play so that we could remove the reflection stuff?

Not my decision, that's up the the Lightbend team which sbt version will be required for the next Play 2.9 release (whenever that happens). I started to work on this fix on the 2.8.x branch, where reflection is needed because people out here right now use sbt 1.2.x or 1.3.x, so if we want to fix it for Play 2.7.x/2.8.x we have to use reflection anyway. In the end I just cherry-picked my patches from the 2.8.x branch to the master branch.

mkurz · 2021-01-28T15:12:15Z

Actually, this pull request is about runtime errors.
There are still problems with sbt 1.4 with compile time errors (even with this patch here applied), which I will provide a fix for as well (probably a seperate pull request)

octonato

It's looking good.

Thanks @mkurz for all the research work. I can imagine that it's not easy to assemble all that info.

I left a few suggestions, no blockers.

For the Scala extractors I mention, I would like to try it out and eventually send a commit.

I'm approving it otherwise already.

octonato · 2021-02-04T11:17:57Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+                      val prefix = "file://" +
+                        (if (!id.startsWith("/")) {
+                           "/" // In Windows the sbt virtual file id does not start with a slash, but absolute paths in Java URIs need that
+                         } else "")


nitpicking...

I find it hard to read when we do if/else (or any logic or computation) embedded in strings or other places.

I think this could benefit of a new val

We can also revert the logic to avoid !.

// In Windows the sbt virtual file id does not start with a slash, but absolute paths in Java URIs need that val extraSlash = if (id.startsWith("/")) "" else "/" val prefix = "file://" + extraSlash

Applied you suggestion.

octonato · 2021-02-04T11:23:41Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+          case Some(file) =>
+            file match {
+              case file: File => // sbt < 1.4
+                Map(name -> Source(file, MaybeGeneratedSource.unapply(file).flatMap(_.source)))
+              case vf => { // sbt 1.4+ virtual file, see #10486
+                vf.getClass.getSimpleName match {
+                  case "BasicVirtualFileRef" | "MappedVirtualFile" => {
+                    val names = vf.getClass.getMethod("names").invoke(vf).asInstanceOf[Array[String]]


Nexting pattern matching can be avoided with Scala custom extractors. I usually prefer to avoid them, but in this case I will make the code more clear.

It could look like:

files.headOption match { case None => Map.empty[String, Source] case SomeFile(file) => case SomeVirtualFile(file) => }

btw, why we need .asInstanceOf[Option[Any]]?

btw, why we need .asInstanceOf[Option[Any]]?

This was needed (before you added the extractors). I was wondering as well, however if it was removed following exception would be thrown (I just tried again):

[info] java.lang.ClassCastException: class sbt.internal.inc.MappedVirtualFile cannot be cast to class java.io.File [info] at play.sbt.run.PlayReload$.$anonfun$sourceMap$1(PlayReload.scala:76) [info] at scala.collection.TraversableLike.$anonfun$flatMap$1(TraversableLike.scala:292) [info] at scala.collection.immutable.HashMap$HashMap1.foreach(HashMap.scala:394) [info] at scala.collection.immutable.HashMap$HashTrieMap.foreach(HashMap.scala:721) [info] at scala.collection.TraversableLike.flatMap(TraversableLike.scala:292) [info] at scala.collection.TraversableLike.flatMap$(TraversableLike.scala:289) [info] at scala.collection.AbstractTraversable.flatMap(Traversable.scala:108) [info] at play.sbt.run.PlayReload$.sourceMap(PlayReload.scala:71) [info] at play.sbt.run.PlayReload$.$anonfun$compile$2(PlayReload.scala:66) [info] at scala.util.Either$RightProjection.map(Either.scala:713)

Line 76 was:

case Some(file) =>

I guess the bytecode Scala emitted somehow was casting the file to a File already in this line (because for the compiler files.headOption is Option[File]) even tough actually its a MappedVirtualFile.
Anyway, with the extractors you added it is also working without that asInstanceOf casting.

octonato · 2021-02-04T12:08:32Z

dev-mode/sbt-plugin/src/main/scala/play/sbt/run/PlayReload.scala

+  object JFile {
+    class FileOption(val anyOpt: Option[Any])  extends AnyVal  {
+      def isEmpty: Boolean = anyOpt.exists(_.isInstanceOf[java.io.File])
+      def get: java.io.File = anyOpt.get.asInstanceOf[java.io.File]
+    }
+    def unapply(any: Option[Any]): FileOption = new FileOption(any)
+  }
+
+  object VirtualFile {
+    def unapply(value: Some[Any]): Option[Any] =
+      value.filter { vf =>
+        val name = value.getClass.getSimpleName
+        (name == "BasicVirtualFileRef" || name == "MappedVirtualFile" )
+      }
+  }


@mkurz, I took the liberty to push on this PR and add these two extractors.

Let me know if you find that it improves the pattern matching bellow. If you don't think so, I can revert.

I'm just trying this out to avoid too much pattern matching nesting.

I fixed your code and it's working now.

octonato · 2021-02-04T16:05:48Z

@mkurz, sorry for the bad formatting. I didn't think about it.

mkurz added 2 commits January 22, 2021 10:37

Support sbt 1.4 virtual files when displaying error source

5605640

Scripted tests

ba0e1f3

mkurz added this to the 2.8.8 milestone Jan 22, 2021

jtjeferreira approved these changes Jan 22, 2021

View reviewed changes

octonato approved these changes Feb 4, 2021

View reviewed changes

use extractor to match on File/VirtualFile

328e53e

octonato reviewed Feb 4, 2021

View reviewed changes

octonato and others added 2 commits February 4, 2021 13:10

VirtualFile.unapply should take Option[Any]

4cc3f1d

scalafmt

2118590

mkurz added the status:block-merge label Feb 4, 2021

mkurz added 3 commits February 4, 2021 21:29

Fix extractors

f8ef943

Easier to read

7be01a7

asInstanceOf[Option[Any]] not needed anymore with extractors

87b7a15

mkurz added status:merge-when-green and removed status:block-merge labels Feb 4, 2021

mergify bot merged commit fd163c0 into playframework:master Feb 4, 2021

mergify bot removed the status:merge-when-green label Feb 4, 2021

mkurz deleted the fix_sbt14_error_source_master branch February 4, 2021 22:12

mkurz mentioned this pull request Feb 19, 2021

Support sbt 1.4 virtual files for compilation and config errors #10711

Merged

mkurz mentioned this pull request May 11, 2023

Drop support for sbt < 1.4 in code #11803

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sbt 1.4 virtual files when displaying error source #10648

Support sbt 1.4 virtual files when displaying error source #10648

mkurz commented Jan 22, 2021 •

edited

jtjeferreira left a comment

jtjeferreira Jan 22, 2021

mkurz Jan 22, 2021

jtjeferreira Jan 22, 2021

mkurz Jan 22, 2021 •

edited

jtjeferreira Jan 22, 2021

mkurz Jan 22, 2021

jtjeferreira Jan 22, 2021 •

edited

mkurz Jan 22, 2021

mkurz commented Jan 22, 2021

mkurz commented Jan 28, 2021

octonato left a comment

octonato Feb 4, 2021

mkurz Feb 4, 2021

octonato Feb 4, 2021

mkurz Feb 4, 2021

octonato Feb 4, 2021

mkurz Feb 4, 2021

octonato commented Feb 4, 2021

Support sbt 1.4 virtual files when displaying error source #10648

Support sbt 1.4 virtual files when displaying error source #10648

Conversation

mkurz commented Jan 22, 2021 • edited

jtjeferreira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz Jan 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtjeferreira Jan 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz commented Jan 22, 2021

mkurz commented Jan 28, 2021

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato commented Feb 4, 2021

mkurz commented Jan 22, 2021 •

edited

mkurz Jan 22, 2021 •

edited

jtjeferreira Jan 22, 2021 •

edited