Test Scala 3 support using sbt-projectmatrix #179

bjaglin · 2021-05-14T23:41:10Z

Towards #178

See scalacenter/scalafix.g8#19 for a background on the preliminary commits

This does not add nor fix any Scala 3 behavior (apart from some error handling), which means existing versions of OrganizeImports should work out of the box once sbt-scalafix & scalafix 0.9.28 are out, thanks to the amazing work of scalameta & dotty maintainers!

This PR does ensures through testing that it works for the vast majority of the use cases (limited to the Scala 2 dialect / features obviously). Apart from the expected failures of the RemoveUnused tests, only 2 were failing - I disabled them for now by moving them to scala-2. It's likely that the bugs should be reported in scalameta.

Note that it should be trivial to start dropping new input/output tests into scala-3 folders after this is merged.

Impact of using sbt-projectmatrix

sbt:scalafix-organize-imports> projects
[info] In file:/home/bjaglin/git/projects/scalafix-organize-imports/
[info] 	   input2_11
[info] 	   input2_12
[info] 	   input2_13
[info] 	   input3
[info] 	   inputUnusedImports2_11
[info] 	   inputUnusedImports2_12
[info] 	   inputUnusedImports2_13
[info] 	   inputUnusedImports3
[info] 	   output2_11
[info] 	   output2_12
[info] 	   output2_13
[info] 	   output3
[info] 	 * scalafix-organize-imports
[info] 	   rules2_11
[info] 	   rules2_12
[info] 	   rules2_13
[info] 	   shared2_11
[info] 	   shared2_12
[info] 	   shared2_13
[info] 	   shared3
[info] 	   tests
[info] 	   testsTarget2_11
[info] 	   testsTarget2_12
[info] 	   testsTarget2_13
[info] 	   testsTarget3

bjaglin · 2021-05-17T12:22:36Z

I will rework this against scalacenter/sbt-scalafix#219 once it's merged, as most of the quirks have been abstracted there.

bjaglin · 2021-05-17T14:34:18Z

build.sbt

@@ -1,5 +1,8 @@
 lazy val v = _root_.scalafix.sbt.BuildInfo

+lazy val rulesCrossVersions = Seq(v.scala213, v.scala212, v.scala211)


FTR, we might cross-build scalafix & rules to Scala3 in the future, but for the moment it's not a priority as the vast majority of the rules don't need to match the sources they are fixing - see scalacenter/scalafix#1316 & scalacenter/sbt-scalafix#214.

bjaglin · 2021-05-17T14:41:45Z

build.sbt

    scalacOptions ++= List(
-      "-deprecation",
-      "-Yrangepos",
-      "-P:semanticdb:synthetics:on"


this wasn't useful (nor documented)

I guess scalac-semanticdb does not honor the documented default of false as synthestics are present without this on Scala 2.

~/git/projects/scalafix-organize-imports$ metap ./input/target/scala-2.12/classes/META-INF/semanticdb/input/src/main/scala/fix/ExplicitlyImportedImplicits.scala.semanticdb | tail -n3 Synthetics: [14:2..14:6) => *(intImplicit) [15:2..15:6) => *(stringImplicit)

bjaglin · 2021-05-17T14:42:33Z

build.sbt

    ),
-    conflictManager := ConflictManager.strict,
+//    conflictManager := ConflictManager.strict,


[error] (shared3 / update) lmcoursier.internal.shaded.coursier.error.conflict.StrictRule: Rule Strict(Set(ModuleMatcher(*:*)), Set(), false, true, false) not satisfied: lmcoursier.internal.shaded.coursier.params.rule.Strict$EvictedDependencies: Unsatisfied rule Strict(*:*): Found evicted dependencies: [error] [error] org.jsoup:jsoup:1.13.1 (1.10.2 wanted) [error] └─ com.vladsch.flexmark:flexmark-html-parser:0.42.12 wants org.jsoup:jsoup:1.10.2 [error] └─ org.scala-lang:scaladoc_3:3.0.0 [error] [error] org.jsoup:jsoup:1.13.1 (1.7.2 wanted) [error] └─ nl.big-o:liqp:0.6.7 wants org.jsoup:jsoup:1.7.2 [error] └─ org.scala-lang:scaladoc_3:3.0.0 [error] [error] org.antlr:antlr-runtime:3.5.1 (3.5 wanted) [error] └─ org.antlr:ST4:4.0.7 wants org.antlr:antlr-runtime:3.5 [error] └─ org.antlr:antlr:3.5.1 [error] └─ nl.big-o:liqp:0.6.7 [error] └─ org.scala-lang:scaladoc_3:3.0.0 [error] [error] com.fasterxml.jackson.core:jackson-core:2.9.8 (2.2.3 wanted) [error] └─ com.fasterxml.jackson.core:jackson-databind:2.2.3 wants com.fasterxml.jackson.core:jackson-core:2.2.3 [error] └─ nl.big-o:liqp:0.6.7 [error] └─ org.scala-lang:scaladoc_3:3.0.0 [error] [error] com.fasterxml.jackson.core:jackson-core:2.9.8 (2.2.3 wanted) [error] └─ nl.big-o:liqp:0.6.7 wants com.fasterxml.jackson.core:jackson-core:2.2.3 [error] └─ org.scala-lang:scaladoc_3:3.0.0

can you advise on your policy to handle this?

I usually just try to pick the newest version when possible.

Actually, I wonder if the conflict manager should only be set on rules since that's where it matters?

This is a failure in shared not related to scalafix bump, but to the cross-building to Scala 3. It seems to come from org.scala-lang:scala3-library_3:3.0.0 itself as it brings inconsistent transitive dependencies, and I am not sure it's relevant to silence this in this project?

# https://get-coursier.io/docs/cli-installation $ cs resolve -t org.scala-lang:scaladoc_3:3.0.0 | grep incompatibility Result: │ └─ org.jsoup:jsoup:1.10.2 -> 1.13.1 (possible incompatibility) │ ├─ com.fasterxml.jackson.core:jackson-core:2.2.3 -> 2.9.8 (possible incompatibility) │ │ └─ com.fasterxml.jackson.core:jackson-core:2.2.3 -> 2.9.8 (possible incompatibility) │ └─ org.jsoup:jsoup:1.7.2 -> 1.13.1 (possible incompatibility)

Or maybe we should report upstream?

bjaglin · 2021-05-17T14:45:09Z

Scalafix 0.9.28 with proper support for Scala 3 in testing will be released tomorrow, but this builds against a master immutable SNAPSHOT, so it's ready for review.

bjaglin · 2021-05-17T15:08:39Z

build.sbt

+  .aggregate(
+    rules.projectRefs ++
+      input.projectRefs ++
+      output.projectRefs ++
+      tests.projectRefs: _*
+  )


required with https://github.com/sbt/sbt-projectmatrix to keep aggregation working (for scalafmtAll for example)

bjaglin · 2021-05-18T09:20:37Z

build.sbt

+lazy val testsAggregate = Project("tests", file("target/testsAggregate"))
+  .aggregate(tests.projectRefs: _*)


Aggregation dummy project to maintain backward compatibility on CI statements and for convenience for sbt ~tests/test

codecov · 2021-05-20T08:25:45Z

Codecov Report

Merging #179 (dbab6b1) into master (68d9108) will decrease coverage by 0.62%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
- Coverage   91.63%   91.01%   -0.63%     
==========================================
  Files           4        4              
  Lines         263      267       +4     
  Branches       12       14       +2     
==========================================
+ Hits          241      243       +2     
- Misses         22       24       +2

Impacted Files	Coverage Δ
rules/src/main/scala/fix/OrganizeImports.scala	`95.63% <60.00%> (-0.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68d9108...dbab6b1. Read the comment docs.

liancheng · 2021-05-26T18:00:42Z

Hey @bjaglin, thanks a bunch for working on this! Honestly, I don't have enough context or confidence to review this PR, so please feel free to nominate other reviewers you believe are suitable. I'm happy to help merge this PR when you think it's ready.

One feedback from me is that it would be great if you can provide a summary of end-user-facing changes introduced by this PR, and follow-up work needed to get better Scala 3 support (e.g., support for given imports, etc.).

bjaglin · 2021-06-01T08:29:43Z

please feel free to nominate other reviewers you believe are suitable

@mlachkar, can you have a look?

One feedback from me is that it would be great if you can provide a summary of end-user-facing changes introduced by this PR, and follow-up work needed to get better Scala 3 support (e.g., support for given imports, etc.).

Good point, before merging this, I am planning to

add some docs in the README in this PR
run the rule against the Dotty codebase (the biggest in Scala3?) and list what's remaining in Scala 3 support #178
figure out what is going on with codecov (your help is appreciated on this as I believe I don't have access to the full setup)

liancheng · 2021-06-04T03:18:50Z

@bjaglin, the Codecov failure is cosmetic. The test coverage drop is less than 1% and can be ignored. I can tune Codecov GitHub check threshold to get rid of it. But we don't necessarily need to block on this. I'm OK with merging this PR before getting this addressed.

tgodzik · 2021-06-04T16:38:30Z

I tried to release it locally and run within Metals, but getting:

java.lang.IllegalArgumentException: Unsupported scala version 3.0
	at scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:122)
	at scalafix.interfaces.Scalafix.fetchAndClassloadInstance(Scalafix.java:87)
	at scala.meta.internal.metals.ScalafixProvider.$anonfun$getScalafix$2(ScalafixProvider.scala:217)
	at scala.util.Try$.apply(Try.scala:213)
	at scala.meta.internal.metals.ScalafixProvider.$anonfun$getScalafix$1(ScalafixProvider.scala:217)
	at scala.meta.internal.metals.StatusBar.trackBlockingTask(StatusBar.scala:46)
	at scala.meta.internal.metals.ScalafixProvider.getScalafix(ScalafixProvider.scala:215)
	at scala.meta.internal.metals.ScalafixProvider.scalafixEvaluate(ScalafixProvider.scala:164)
	at scala.meta.internal.metals.ScalafixProvider.$anonfun$organizeImports$2(ScalafixProvider.scala:78)
	at scala.concurrent.Future.$anonfun$flatMap$1(Future.scala:307)
	at scala.concurrent.impl.Promise.$anonfun$transformWith$1(Promise.scala:41)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Should it work with Scalafix 0.9.29 ?

bjaglin · 2021-06-05T07:42:46Z

scalafix-organize-imports 0.5.0 should work with Scalafix 0.9.28+ - this PR is just about testing that it works (and providing a proper error or fixing the few cases where it does not). The problem is that Metals tries to classload Scalafix with the same binary version as the project sources, and as discussed in scalacenter/scalafix#1316, cross-building scalafix-core and rules against 3.0 is only the end goal. For now, you should classload 2.12 or 2.13, indifferently.

The reason for classloading a matching scala binary version was for the built-in rule using the presentation compiler (ExplicitResultTypes), but it doesn't work anyway for the moment on Scala 3. See #179 (comment). In other words, we are backpedaling from scalacenter/scalafix#1108 (comment).

tgodzik · 2021-06-05T10:29:35Z

Och, I totally missed that. It does actually work for Scala 3 if I use 2.13 binary version for it. Thanks! I think we can do with that for the time being, since we don't yet support any other rules.

tgodzik · 2021-06-05T11:00:36Z

Got it working! scalameta/metals#2857

mlachkar

This PR adds tests for scala 3, and doesn't impacts the scala 2 behavior of this rule.
I think it's nice to have tests for scala 3, but we can maybe add some documentation as mentioned by @bjaglin to explain what won't work (ie removeUnusued.)

mlachkar · 2021-06-07T07:50:57Z

build.sbt

+    publish / skip := true,
+    coverageEnabled := false
+  )
+  .defaultAxes(VirtualAxis.jvm)


It's pity that the default it's not VirtualAxis.jvm: so we wouldn't need to specify it everytime

liancheng · 2021-06-09T23:23:34Z

@bjaglin, I've loosened Codecov threshold in #186 to tolerant minor coverage drops caused by cosmetic changes. Would you please rebase this PR to the most recent master and see whether it works now?

* Check Scala 2.x input/output with Scala 2.x rule (as before) * Check Scala 3 input/output with Scala 2.12 rule * Disable coverage (even after `coverage` command) on input/output projects as scoverage is not available in Scala 3 and coverage there is not useful there anyway * Limit strict conflictManager to rules to workaround false positives caused by org.scala-lang:scaladoc_3:3.0.0

bjaglin · 2021-06-11T17:48:03Z

@liancheng I rebased against master but there is still something fishy in the patch check - I suspect it's because the module structure changed, impacting how results are collected, but I don't have enough access on codecov to confirm/infirm this.

bjaglin · 2021-06-24T23:39:03Z

I finally got the time to look into the failures, and document them as known limitations - see the last 3 commits separately.

@liancheng I suggest we merge this to unlock iterating in other PRs, what do you think?

liancheng · 2021-06-29T16:25:32Z

@bjaglin Sorry for the late response, daily workload fell into hell mode recently... I'm merging this PR. Thanks for the awesome work!

bjaglin mentioned this pull request May 14, 2021

Scalafix 0.9.26 breaks scalafix-organize-imports unit test scalacenter/scalafix#1337

Closed

bjaglin force-pushed the scala3 branch 2 times, most recently from 9f677e2 to 40d8a32 Compare May 14, 2021 23:55

bjaglin changed the title ~~Scala 3 support~~ Test Scala 3 support May 15, 2021

bjaglin force-pushed the scala3 branch 4 times, most recently from 7fc599c to 795ff11 Compare May 15, 2021 07:55

bjaglin changed the title ~~Test Scala 3 support~~ Test Scala 3 support using sbt-projectmatrix May 17, 2021

bjaglin force-pushed the scala3 branch 2 times, most recently from 21e5ede to 2032a12 Compare May 17, 2021 14:32

bjaglin commented May 17, 2021

View reviewed changes

bjaglin force-pushed the scala3 branch from 2032a12 to 0d5da53 Compare May 17, 2021 14:41

bjaglin commented May 17, 2021

View reviewed changes

bjaglin marked this pull request as ready for review May 17, 2021 14:44

bjaglin commented May 17, 2021

View reviewed changes

bjaglin force-pushed the scala3 branch 2 times, most recently from c8942f6 to 70b8db1 Compare May 18, 2021 09:19

bjaglin commented May 18, 2021

View reviewed changes

bjaglin force-pushed the scala3 branch 6 times, most recently from abc77aa to 8c4ff44 Compare May 20, 2021 08:22

bjaglin force-pushed the scala3 branch from 8c4ff44 to 85045cd Compare May 20, 2021 08:33

bjaglin force-pushed the scala3 branch 2 times, most recently from a843e90 to 6762446 Compare May 24, 2021 12:31

bjaglin mentioned this pull request May 30, 2021

Update scalafix to the latest version, fix some rules and run the tests in the CI typelevel/fs2#2415

Merged

mlachkar approved these changes Jun 7, 2021

View reviewed changes

github-brice-jaglin added 2 commits June 10, 2021 08:45

use sbt 1.3+ SemanticdbPlugin to get semanticdb output

8b08f4c

bjaglin force-pushed the scala3 branch from 995d9af to e8424de Compare June 10, 2021 06:46

tgodzik mentioned this pull request Jun 11, 2021

[Scala 3] Add support for given imports #187

Merged

github-brice-jaglin added 2 commits June 25, 2021 01:10

Scala 3: removeUnused cannot work as compiler support is missing

71a5b9b

Scala 3: package objects imports do not work well

af681b4

bjaglin force-pushed the scala3 branch from e8424de to 89068e6 Compare June 24, 2021 23:34

Scala 3: groupExplicitlyImportedImplicitsSeparately has no effect

dbab6b1

bjaglin force-pushed the scala3 branch from 89068e6 to dbab6b1 Compare June 24, 2021 23:41

bjaglin mentioned this pull request Jun 24, 2021

Scala 3 support #178

Closed

liancheng merged commit 1589fa8 into liancheng:master Jun 29, 2021

liancheng mentioned this pull request Jul 27, 2021

Run the CI jobs using JDK 11 #194

Merged

liancheng mentioned this pull request Sep 24, 2021

Merge OrganizeImports 0.6.0+37-596459af into Scalafix as a built-in rule scalacenter/scalafix#1480

Merged

9 tasks

This was referenced Nov 3, 2021

Can we release a new version of scalafix-organize-imports #227

Closed

fix release process #231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Scala 3 support using sbt-projectmatrix #179

Test Scala 3 support using sbt-projectmatrix #179

bjaglin commented May 14, 2021 •

edited

Loading

bjaglin commented May 17, 2021

bjaglin May 17, 2021

bjaglin May 17, 2021

bjaglin Jun 24, 2021

bjaglin May 17, 2021

liancheng May 21, 2021

bjaglin May 22, 2021 •

edited

Loading

bjaglin commented May 17, 2021

bjaglin May 17, 2021

bjaglin May 18, 2021

codecov bot commented May 20, 2021 •

edited

Loading

liancheng commented May 26, 2021

bjaglin commented Jun 1, 2021 •

edited

Loading

liancheng commented Jun 4, 2021

tgodzik commented Jun 4, 2021

bjaglin commented Jun 5, 2021 •

edited

Loading

tgodzik commented Jun 5, 2021

tgodzik commented Jun 5, 2021

mlachkar left a comment •

edited

Loading

mlachkar Jun 7, 2021

liancheng commented Jun 9, 2021

bjaglin commented Jun 11, 2021 •

edited

Loading

bjaglin commented Jun 24, 2021

liancheng commented Jun 29, 2021

		@@ -1,5 +1,8 @@
		lazy val v = _root_.scalafix.sbt.BuildInfo

		lazy val rulesCrossVersions = Seq(v.scala213, v.scala212, v.scala211)

		lazy val testsAggregate = Project("tests", file("target/testsAggregate"))
		.aggregate(tests.projectRefs: _*)

Test Scala 3 support using sbt-projectmatrix #179

Test Scala 3 support using sbt-projectmatrix #179

Conversation

bjaglin commented May 14, 2021 • edited Loading

Impact of using sbt-projectmatrix

bjaglin commented May 17, 2021

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

bjaglin May 22, 2021 • edited Loading

Choose a reason for hiding this comment

bjaglin commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 20, 2021 • edited Loading

Codecov Report

liancheng commented May 26, 2021

bjaglin commented Jun 1, 2021 • edited Loading

liancheng commented Jun 4, 2021

tgodzik commented Jun 4, 2021

bjaglin commented Jun 5, 2021 • edited Loading

tgodzik commented Jun 5, 2021

tgodzik commented Jun 5, 2021

mlachkar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liancheng commented Jun 9, 2021

bjaglin commented Jun 11, 2021 • edited Loading

bjaglin commented Jun 24, 2021

liancheng commented Jun 29, 2021

bjaglin commented May 14, 2021 •

edited

Loading

bjaglin May 22, 2021 •

edited

Loading

codecov bot commented May 20, 2021 •

edited

Loading

bjaglin commented Jun 1, 2021 •

edited

Loading

bjaglin commented Jun 5, 2021 •

edited

Loading

mlachkar left a comment •

edited

Loading

bjaglin commented Jun 11, 2021 •

edited

Loading