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

scalameta 4.9.x #1927

Merged
merged 5 commits into from
Feb 21, 2024
Merged

scalameta 4.9.x #1927

merged 5 commits into from
Feb 21, 2024

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented Feb 10, 2024

https://github.com/scalameta/scalameta/releases/tag/v4.9.0

scalameta/scalameta#3425 is a non-backward compatible feature, as quasiquotes expand to code referencing scala.meta.trees.Origin which does not exist in 4.8.x.

This means that Scalafix rules containing quasiquotes compiled against scalameta 4.9.x will cause link errors if ran on older Scalafix versions. As a result, we need a minor Scalafix bump when taking that in, despite the fact that it's "just" a minor bump in a SemVer lib (we could argue that a major bump of scalameta was needed, but backward compatibility guarantees with macros is not very well defined).

Edit after v0.12.0 release

I originally warned rule authors of this in the release notes:

  • Changes breaking forward binary compatibility
  • Usage of quasiquotes for tree traversal or pretty-printing in rules built against Scalafix 0.12.x will result in runtime errors when executed with Scalafix 0.11.1 or earlier (quasiquotes: preserve origin or dialect if possible scalameta/scalameta#3425)
    • Rules using quasiquotes and built with Scalafix 0.12.x should signal this via a major bump
    • Users of Scalafix 0.11.1 or earlier loading such rules will get a warning as documented

But this turns out NOT to be an issue with old scalafix-cli, as scala.meta.trees.Origin is a brand new symbol, properly resolved by the tool classpath URLClassLoader which contains scalameta 4.9.x (which the parent scalafix-cli classloader containing scalameta 4.8.x reverts to).

@bjaglin
Copy link
Collaborator Author

bjaglin commented Feb 11, 2024

6d8b555 causes core compilation to succeed, but docs2_13 to fail

[error] Caused by: java.lang.ClassNotFoundException: scala.meta.trees.Origin

That's because for some reason, coursier prefers 4.8.15 over 4.8.15+150-a7c0baa0-SNAPSHOT there:

sbt:scalafix> show docs2_13/evicted
...
[info] 	* org.scalameta:scalameta_2.13:4.8.15 is selected over {4.8.15+150-a7c0baa0-SNAPSHOT, 4.8.15+150-a7c0baa0-SNAPSHOT, 4.8.15+150-a7c0baa0-SNAPSHOT, 4.8.15+150-a7c0baa0-SNAPSHOT
[info] 	    +- org.scalameta:mdoc_2.13:2.5.2                      (depends on 4.8.15)
[info] 	    +- org.scalameta:mdoc-cli_2.13:2.5.2                  (depends on 4.8.15)
[info] 	    +- org.scalameta:semanticdb-scalac-core_2.13.12:4.8.15+150-a7c0baa0-SNAPSHOT (depends on 4.8.15+150-a7c0baa0-SNAPSHOT)
[info] 	    +- ch.epfl.scala:scalafix-core_2.13:0.11.1+108-789b602c-SNAPSHOT (depends on 4.8.15+150-a7c0baa0-SNAPSHOT)

Adding scalameta at the root does affect resolution, so it looks like Coursier is following nearest-win instead of latest-win (despite what's expected).

 lazy val docs = projectMatrix
   .in(file("scalafix-docs"))
   .settings(
+    libraryDependencies += scalameta,
sbt:scalafix> show docs2_13/evicted
...
[info] 	* org.scalameta:scalameta_2.13:4.8.15+150-a7c0baa0-SNAPSHOT is selected over {4.8.15, 4.8.15}
[info] 	    +- ch.epfl.scala:scalafix-docs_2.13:0.11.1+108-789b602c+20240211-1152-SNAPSHOT (depends on 4.8.15+150-a7c0baa0-SNAPSHOT)
[info] 	    +- ch.epfl.scala:scalafix-core_2.13:0.11.1+108-789b602c+20240211-1152-SNAPSHOT (depends on 4.8.15+150-a7c0baa0-SNAPSHOT)
[info] 	    +- org.scalameta:semanticdb-scalac-core_2.13.12:4.8.15+150-a7c0baa0-SNAPSHOT (depends on 4.8.15+150-a7c0baa0-SNAPSHOT)
[info] 	    +- org.scalameta:mdoc_2.13:2.5.2                      (depends on 4.8.15)
[info] 	    +- org.scalameta:mdoc-cli_2.13:2.5.2                  (depends on 4.8.15)

Forcing eviction via ea2e835

@bjaglin
Copy link
Collaborator Author

bjaglin commented Feb 11, 2024

ea2e835 exposes an incompatibility between mdoc and the latest scalameta SNAPSHOT

[error] Caused by: java.lang.ClassNotFoundException: scala.meta.internal.prettyprinters.enquote$

Following-up in scalameta/mdoc#842

@bjaglin bjaglin force-pushed the test-scalameta branch 2 times, most recently from f511e2c to 3343cf8 Compare February 11, 2024 11:48
@bjaglin
Copy link
Collaborator Author

bjaglin commented Feb 11, 2024

[info] warning: patch.md:168:22: local val dialectOnly$macro$4 in value qual$12 is never used
[info] Patch.removeImportee(importee"Future").showDiff()
[info]                      ^^^^^^^^

Following-up in scalameta/scalameta#3546

@bjaglin
Copy link
Collaborator Author

bjaglin commented Feb 11, 2024

[info] error: patch.md:168:1: token not found: 
[info] Patch.removeImportee(importee"Future").showDiff()
[info] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[info] java.util.NoSuchElementException: token not found: 
[info] 	at scalafix.util.TokenList.$anonfun$tok2idx$2(TokenList.scala:27)
[info] 	at scala.collection.immutable.Map$WithDefault.default(Map.scala:181)
[info] 	at scala.collection.MapOps.apply(Map.scala:176)
[info] 	at scala.collection.MapOps.apply$(Map.scala:175)
[info] 	at scala.collection.AbstractMap.apply(Map.scala:405)
[info] 	at scalafix.util.TokenList.leading(TokenList.scala:15)
[info] 	at scalafix.internal.patch.ImportPatchOps$.remove$1(ImportPatchOps.scala:229)
[info] 	at scalafix.internal.patch.ImportPatchOps$.$anonfun$superNaiveImportPatchToTokenPatchConverter$34(ImportPatchOps.scala:264)
[info] 	at scala.collection.StrictOptimizedIterableOps.map(StrictOptimizedIterableOps.scala:100)
[info] 	at scala.collection.StrictOptimizedIterableOps.map$(StrictOptimizedIterableOps.scala:87)
[info] 	at scala.collection.mutable.LinkedHashSet.map(LinkedHashSet.scala:34)
[info] 	at scalafix.internal.patch.ImportPatchOps$.superNaiveImportPatchToTokenPatchConverter(ImportPatchOps.scala:264)
[info] 	at scalafix.internal.patch.PatchInternals$.treePatchApply(PatchInternals.scala:118)

if (toRemove.pos == Position.None) return Patch.empty
brings false negatives for quasiquotes-built trees since scalameta/scalameta#3450, resulting in an exception instead of the documented no-op.

Another similar guard is impacted, although it causes a slight regression that might not be worth fixing:

if (from.pos == Position.None) Patch.empty

  • Patch.replaceTree(q"foo", "bar") is no-longer a no-op as the early-return no longer applies now that quasiquotes generate trees with positions
    • It is now the same as Patch.addLeft(q"foo", "bar") or Patch.addLeft(Term.Name("foo"), "bar") (unchanged behavior for them): it adds a leading string to any tree, as the BOF token has the same hash no matter the origin, causing a false positive
  • Patch.replaceTree(Term.Name("foo"), "bar") remains a no-op thanks to the early-return

@bjaglin bjaglin force-pushed the test-scalameta branch 5 times, most recently from 119b399 to f76d880 Compare February 13, 2024 06:12
@bjaglin bjaglin changed the title Latest scalameta scalameta 4.9.x Feb 15, 2024
Coursier seems to prefer mdoc's scalameta 4.8.15 over core2_13's scalameta
4.8.15+150-a7c0baa0-SNAPSHOT. Adding scalameta as a top-level docs2_13
libraryDependencies does change this behavior, so I suspect Coursier is
following nearest-wins semantics for some reason, while we expect it to
following latest-wins semantics.

http://eed3si9n.com/dependency-resolver-semantics#coursiers-latest-wins-semantics

By using dependencyOverrides, we ensure that docs run are generated with the
actual scalameta version used in scalafix, rather than the one used in mdoc.
Quasiquotes-generated trees have positions since scalameta#3450. Fixes:

error: patch.md:168:1: token not found:
Patch.removeImportee(importee"Future").showDiff()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
java.util.NoSuchElementException: token not found:
  at scalafix.util.TokenList.$anonfun$tok2idx$2(TokenList.scala:27)
  at scala.collection.immutable.Map$WithDefault.default(Map.scala:181)
  at scala.collection.MapOps.apply(Map.scala:176)
  at scala.collection.MapOps.apply$(Map.scala:175)
  at scala.collection.AbstractMap.apply(Map.scala:405)
  at scalafix.util.TokenList.leading(TokenList.scala:15)
  at scalafix.internal.patch.ImportPatchOps$.remove$1(ImportPatchOps.scala:229)

object ScalafixScalametaHacks {
def resetOrigin(tree: Tree): Tree = tree.withOrigin(Origin.None)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused since 1ab1d7e

@@ -200,7 +201,11 @@ object ImportPatchOps {
allImports.filter(_.importers.forall(isRemovedImporter))

def remove(toRemove: Tree): Patch = {
if (toRemove.pos == Position.None) return Patch.empty
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brings false negatives now that quasiquotes have positions (since scalameta/scalameta#3425). This is effectively a very minor breaking change in the API.

@bjaglin bjaglin marked this pull request as ready for review February 15, 2024 16:26
@bjaglin bjaglin merged commit 7902fb7 into main Feb 21, 2024
8 checks passed
@bjaglin bjaglin deleted the test-scalameta branch February 26, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant