scala3 icon indicating copy to clipboard operation
scala3 copied to clipboard

Warn unused imports

Open som-snytt opened this issue 3 years ago • 25 comments

Forward port the Scala 2 feature, tracking imports when contexts are created, and registering when selectors are used.

Some support for -rewrite to remove unused selectors (if there is only one import on the source line), disabled.

som-snytt avatar Feb 20 '22 21:02 som-snytt

It doesn't like the mutation in Contexts:

possible data race involving globally reachable variable _initialStore in object Contexts: dotty.tools.dotc.util.Store
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.

neg/i13558.scala needs updated check for prettier-printed wildcard

                    Reference to id is ambiguous,
                    it is both imported by import testcode.ExtensionB.*
                    and imported subsequently by import testcode.ExtensionA.*

-Werror not recognized? The improved error output looks nice.

 Wrong number of errors encountered when compiling out/compileNeg/neg/unused-imports
expected: 5, actual: 1
Unfulfilled expectations:
tests/neg/unused-imports.scala:12
tests/neg/unused-imports.scala:5
tests/neg/unused-imports.scala:6
Unexpected errors:

Reported errors:
 at 11: Found:    ("42" : String)\Required: Int

som-snytt avatar Feb 20 '22 22:02 som-snytt

-Werror is an established option. Maybe scaladoc test didn't like it? as I recall from the previous failure.

It looks like -Wunused:imports is not set in the right place for one of the test regimes. The build is really quite complicated. I am getting better at "run a test" but still have no idea about what is run for CI. I'm trying out each piece of the sbt command to see I can witness which fails. In particular, "it would be nice" if CI output had headers saying what is running, instead of just continuous output from sbt.

Putting this at the top of my test is supposed to mean I don't have to jury-rig something with "tests that require a setting" or however that works:

// scalac: -Wunused:imports -Werror -feature

som-snytt avatar Feb 21 '22 06:02 som-snytt

Not my imagination that this works:

sbt:scala3> testCompilation tests/neg/unused-imports
[info] Test run started
[info] Test dotty.tools.dotc.CompilationTests.negAll started
[=======================================] completed (1/1, 0 failed, 2s)

and

sbt:scala3> scala3-bootstrapped/testCompilation tests/neg/unused-imports
[info] Test run started
[info] Test dotty.tools.dotc.CompilationTests.negAll started
[=======================================] completed (1/1, 0 failed, 2s)

but

sbt:scala3> scala3-bootstrapped/test
[snip]
[info] Test dotty.tools.dotc.CompilationTests.negAll started

Wrong number of errors encountered when compiling out/compileNeg/neg/unused-imports
expected: 5, actual: 1
Unfulfilled expectations:
tests/neg/unused-imports.scala:12
tests/neg/unused-imports.scala:5
tests/neg/unused-imports.scala:6
Unexpected errors:

Reported errors:
 at 11: Found:    ("42" : String)\Required: Int
[=======================================] completed (1380/1380, 1 failed, 58s)
[error] Test dotty.tools.dotc.CompilationTests.negAll failed: java.lang.AssertionError: Neg test should have failed, but did not, took 58.136 se
[error]     at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkExpectedErrors(ParallelTesting.scala:1003)
[error]     at dotty.tools.dotc.CompilationTests.negAll(CompilationTests.scala:190)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error]     at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error]     at java.lang.reflect.Method.invoke(Method.java:568)
[error]     ...

som-snytt avatar Feb 21 '22 06:02 som-snytt

maybe I'll just ~delete~ disable the test.

som-snytt avatar Feb 21 '22 19:02 som-snytt

The fixup edits are moved to https://github.com/lampepfl/dotty/pull/14533 The rewrite result didn't get manual edits so it could be re-run any time. That was mostly just a test. (Still managed a few conflicts, however.)

som-snytt avatar Feb 22 '22 00:02 som-snytt

Rebased and squashed, but I haven't made the test work under all regimes. This PR feels like a long time ago but it says just a fortnight.

Edit: test should be neg-with-compiler

[info] Test dotty.tools.dotc.BootstrappedOnlyCompilationTests.negWithCompiler started
[=======================================] completed (1/1, 0 failed, 0s)

Edit: or maybe that is not enough to survive vulpix scala3-bootstrapped/test is the command to beat. Maybe I'll have to take the vulpix coursera after all.

som-snytt avatar Mar 07 '22 15:03 som-snytt

*** error while checking tests/neg-with-compiler/unused-imports-live.scala after phase capturedVars ***
class dotty.tools.dotc.reporting.Diagnostic$Error at tests/neg-with-compiler/unused-imports-live.scala:<1172..1172>: Definition of implicit conversion method mkOrderingOps should be enabled

seems to be https://github.com/lampepfl/dotty/issues/14637 but I also haven't drilled into how vulpix is compiling it.

Yes, got it to pass by removing -feature from the test file.

Or, vulpix is not a great experience. I remember hating partest when it was half-baked.

[info] Test dotty.tools.dotc.BootstrappedOnlyCompilationTests.negWithCompiler started

No errors found when compiling neg test out/compileNegWithCompiler/neg-with-compiler/unused-imports-live
[=======================================] completed (2/2, 1 failed, 1s)

som-snytt avatar Mar 07 '22 23:03 som-snytt

@griggt Thanks, I see the test has language.future, so I must have tried it out. I'll try to rediscover the mechanism. The test needs to be under neg-with-compiler to work under different test modes, I think is what I learned at some point, so I'll attempt it.

Imports with corresponding compiler options are tricky. Do I want to see a warning for language.postfixOps because the compiler option was supplied somewhere in a build file? Anything unused may be redundant, such as var x shadowed by another due to a merge mistake. What about duplicate imports? Currently I would expect a warning, but I have not pondered the case recently.

I see I had a commit for feature imports before I squashed.

som-snytt avatar May 06 '22 04:05 som-snytt

The issue with the test is weird. I don't see any reason that it needs to be in neg-with-compiler, however.

The symptoms lead me to believe it has something to do with parallelism, as it works when run singly using testCompilation.

Try putting the test file in its own subdirectory under tests/neg, e.g. tests/neg/unused-imports/unused-imports.scala with the check file as tests/neg/unused-imports.check (yes, in the parent directory, the check file should have the same name as the subdirectory; it will need updating to reflect the paths).

griggt avatar May 06 '22 05:05 griggt

Hey, don't break my brain! The comment in the test says

// disabled because doesn't work under scala3-bootstrapped/test
// works under testCompilation

Under neg-with, it only runs when bootstrapped, so that's correct. If vulpix is making me think too hard, the overhead isn't worth it. (Normally I enjoy testing puzzles like any other.)

I see the language version code, so I will try to do something not bad.

Actually, sourceVersion in file shadows compiler options, but language feature in options means redundant import is not checked.

som-snytt avatar May 06 '22 05:05 som-snytt

Ignore my previous theory on how to use vulpix. I'll look at what sbt command it's actually running and then try what you said.

 Wrong number of errors encountered when compiling out/compileNegWithCompiler/neg-with-compiler/unused-imports
expected: 5, actual: 1
Unfulfilled expectations:
tests/neg-with-compiler/unused-imports.scala:5
tests/neg-with-compiler/unused-imports.scala:6
tests/neg-with-compiler/unused-imports.scala:12
Unexpected errors:

Reported errors:
 at 11: Found:    ("42" : String)\Required: Int

It doesn't list multiple unfulfilled expectations on a line.

som-snytt avatar May 06 '22 06:05 som-snytt

I've looked into the test problem some more and I'm convinced the issue is shared state corruption when multiple compilations are running in parallel. I think the fix is something like this:

diff --git a/compiler/src/dotty/tools/dotc/core/Contexts.scala b/compiler/src/dotty/tools/dotc/core/Contexts.scala
index a0ceddce9d..ea616b5843 100644
--- a/compiler/src/dotty/tools/dotc/core/Contexts.scala
+++ b/compiler/src/dotty/tools/dotc/core/Contexts.scala
@@ -60,7 +60,7 @@ object Contexts:
   private val (notNullInfosLoc,     store8) = store7.newLocation[List[NotNullInfo]]()
   private val (importInfoLoc,       store9) = store8.newLocation[ImportInfo | Null]()
   private val (typeAssignerLoc,    store10) = store9.newLocation[TypeAssigner](TypeAssigner)
-  private val (usagesLoc,          store11) = store10.newLocation[Usages](Usages())
+  private val (usagesLoc,          store11) = store10.newLocation[Usages]()
 
   private val initialStore = store11
 
@@ -837,6 +837,7 @@ object Contexts:
     store = initialStore
       .updated(settingsStateLoc, settingsGroup.defaultState)
       .updated(notNullInfosLoc, Nil)
+      .updated(usagesLoc, Usages())
       .updated(compilationUnitLoc, NoCompilationUnit)
     searchHistory = new SearchRoot
     gadt = EmptyGadtConstraint

The test should be able to live in tests/neg/unused-imports.scala.

griggt avatar May 06 '22 08:05 griggt

@griggt Thanks for the hint!

som-snytt avatar May 06 '22 09:05 som-snytt

Update includes all of @griggt suggestions.

Intuitively, I'd like to know if my import language.next changed any behavior, but the behavior is too diffuse. For instance, some code just checks version eq future and does something different. Moreover, version selection might be a backstop against future edits. Therefore, just ignore the imports.

Similarly, masking imports are still ignored. Even if you track that something was masked, such imports can be useful as a defense mechanism.

-rewrite support is moved to TyperPhase.emitDiagnostics. -Yrewrite-imports -Wunused:imports -rewrite to enable, baroque enough to be safe. Either rewrite or report, not both. (The rewrite test requires that?) It will report what it could not rewrite; the restriction is still that import is by itself on the line, modulo comments.

Noting that it's much easier to add compiler options to test code as "directives" than touch the test rig; tests/rewrites requires at least mentioning the test but perhaps could just compile in dir with all options as directives, or -rewrite among the defaults.

som-snytt avatar May 06 '22 19:05 som-snytt

I'm currently running this through the community build (-Wunused:imports -rewrite -Yrewrite-imports)

I'll post the results when complete, but thus far it's definitely uncovered some issues.

griggt avatar May 11 '22 03:05 griggt

@griggt thanks I re-drafted to fix how the rewrite happens and also to rewrite under a flag. I haven't look at other unused symbols yet.

som-snytt avatar May 11 '22 04:05 som-snytt

One common issue is this error:

[error] -- [E040] Syntax Error: import-line.scala:2:0 ----------------------------------
[error] 2 |
[error]   |^
[error]   |an identifier expected, but eof found

which happens in intent, effpi, scalatest, maybe others.

Yet some other issues appear to be false positives resulting in failed compilation of the rewritten code.

A full writeup will probably come no sooner than tomorrow.

griggt avatar May 11 '22 04:05 griggt

The issues uncovered by running this through the community build generally fall into three categories:

  1. Failure of the rewriting process itself
  2. Rewritten code fails to compile with the same (Scala 3) compiler
  3. Rewritten code fails to cross-compile with some Scala 2.x compiler(s) supported by the project

More details on each category are split into the posts below.

griggt avatar May 12 '22 00:05 griggt

1. Failure of the rewriting process itself

This manifested like so:

[error] -- [E040] Syntax Error: import-line.scala:2:0 ----------------------------------
[error] 2 |
[error]   |^
[error]   |an identifier expected, but eof found

Observed in effpi, intent, scalatest, monocle.

Unrelated to this PR, but some projects failed to build by just adding the -rewrite option (without -Wunused:imports and -Yrewrite-imports). scalatest had a slew of errors related to indentation width and mixing tabs and spaces, and zio, while having no files apparently changed by rewriting gave this error:

[error] -- [E007] Type Mismatch Error: /home/dotty/dotty/community-build/community-projects/zio/test/shared/src/main/scala-3/zio/test/Macros.scala:198:40
[error] 198 |       '{TestArrow.succeed($expr).span($span)}
[error]     |                                        ^^^^
[error]     |Found:    SmartAssertMacros.this.given_Quotes.reflect.Term => quoted.Expr[(Int, Int)]
[error]     |Required: quoted.Expr[(Int, Int)]

Thus scalatest and zio were excluded from further study.

griggt avatar May 12 '22 00:05 griggt

2. Rewritten code fails to compile with the same (Scala 3) compiler

This behavior was observed in specs2, play-json, and protoquill.

specs2:

Several instances where import X.{given, *} was replaced by import X.given turned out to be problematic. A couple examples:

--- a/common/shared/src/main/scala/org/specs2/text/Sentences.scala
+++ b/common/shared/src/main/scala/org/specs2/text/Sentences.scala
@@ -2,7 +2,7 @@ package org.specs2
 package text

 import collection.BiMap
-import BiMap.{given, *}
+import BiMap.given

 /** This does some simple replacements in sentences to negate them.
   *
[error] -- [E008] Not Found Error: /home/dotty/dotty/community-build/community-projects/specs2/common/shared/src/main/scala/org/specs2/text/Sentences.scala:15:19
[error] 15 |      " contains " <-> " does not contain ",
[error]    |      ^^^^^^^^^^^^^^^^
[error]    |value <-> is not a member of String, but could be made available as an extension method.
[error]    |
[error]    |The following import might fix the problem:
[error]    |
[error]    |  import org.specs2.collection.BiMap.<->
--- a/matcher/shared/src/main/scala/org/specs2/matcher/EitherMatchers.scala
+++ b/matcher/shared/src/main/scala/org/specs2/matcher/EitherMatchers.scala
@@ -3,7 +3,7 @@ package matcher

 import language.adhocExtensions
 import control.*
-import ImplicitParameters.{given, *}
+import ImplicitParameters.given
 import describe.Diffable

 /** Matchers for the Either datatype
[error] -- [E006] Not Found Error: /home/dotty/dotty/community-build/community-projects/specs2/matcher/shared/src/main/scala/org/specs2/matcher/EitherMatchers.scala:14:63
[error] 14 |  def beRight[T](using p: ImplicitParam = implicitParameter) = use(p)(new RightMatcher[T])
[error]    |                                                               ^^^
[error]    |                                                          Not found: use

play-json:

--- a/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala
+++ b/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala
@@ -504,7 +504,6 @@ object MacroSpec {
   object FamilyCodec {
     implicit val simpleWrites: OWrites[Simple]     = Json.writes[Simple]
     implicit val optionalWrites: OWrites[Optional] = Json.writes[Optional]
-    import LoremCodec.loremWrites

     implicit val familyWrites: OWrites[Family] = Json.writes[Family] // Failing:
     /* java.lang.IllegalArgumentException:

[error] -- Error: /home/dotty/dotty/community-build/community-projects/play-json/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala:508:49
[error] 508 |    implicit val familyWrites: OWrites[Family] = Json.writes[Family] // Failing:
[error]     |                                                 ^^^^^^^^^^^^^^^^^^^
[error]     |No Json serializer found for type play.api.libs.json.MacroSpec.Lorem[Any]. Try to implement an implicit Writes or Format for this type..
[error]     |I found:
[error]     |
[error]     |    play.api.libs.json.Writes.iterableWrites2[A,
[error]     |      play.api.libs.json.MacroSpec.Lorem[Any]
[error]     |    ](/* missing */summon[play.api.libs.json.MacroSpec.Lorem[Any] <:< Iterable[A]],
[error]     |      ???
[error]     |    )
[error]     |
[error]     |But no implicit values were found that match type play.api.libs.json.MacroSpec.Lorem[Any] <:< Iterable[A].
[error]     |
[error]     |One of the following imports might fix the problem:
[error]     |
[error]     |  import play.api.libs.json.MacroSpec.LoremCodec.loremFormat
[error]     |  import play.api.libs.json.MacroSpec.LoremCodec.loremWrites
[error]     |
[error]     |---------------------------------------------------------------------------
[error]     |Inline stack trace
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from JsMacroImpl.scala:126
[error] 126 |  inline def summonWrites[A]: Writes[A] = summonInline[Writes[A]]
[error]     |                                          ^^^^^^^^^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from JsMacroImpl.scala:126
[error]  92 |          val xjs = summonWrites[t].writes(x.asInstanceOf[t])
[error]     |                    ^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from JsMacroImpl.scala:126
[error]  98 |        } else writeCasesL[A, ts](x, ord)(n + 1)
[error]     |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from JsMacroImpl.scala:126
[error]  58 |    writeCasesL[A, m.MirroredElemTypes](x, m.ordinal(x))(0)
[error]     |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from JsMacroImpl.scala:126
[error]  33 |      case m: Mirror.SumOf[A]     => writeCases[A](x)(using self, m)
[error]     |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from JsMacroImpl.scala:126
[error]  53 |  inline def writes[A](using m: Mirror.Of[A]): OWrites[A] = JsMacroImpl.writes[A]
[error]     |                                                            ^^^^^^^^^^^^^^^^^^^^^
[error]      ---------------------------------------------------------------------------

protoquill:

This is a macro heavy project, and rewriting removed a significant number of imports, which seems to have caused widespread breakage. I'll just post a few example errors here, this would be a good project to investigate.

The failures here actually occurred on the initial run, whereas those in specs2 and play-json were from a follow-on run of the community build after the first run with -Wunused-imports -Yrewrite-imports -rewrite had completed successfully

[error] -- [E049] Reference Error: /home/dotty/dotty/community-build/community-projects/protoquill/quill-sql/src/main/scala/io/getquill/StaticSplice.scala:32:25
[error] 32 |        module        <- Try(untypedModule.asInstanceOf[StaticSplice[T]]).toEither.mapLeft(_.getMessage)
[error]    |                         ^^^
[error]    | Reference to Try is ambiguous,
[error]    | it is both imported by name by import util.Try
[error]    | and imported subsequently by import (x$1.reflect : x$1.reflectModule).*
[error] -- [E049] Reference Error: /home/dotty/dotty/community-build/community-projects/protoquill/quill-sql/src/main/scala/io/getquill/parser/Parser.scala:474:28
[error] 474 |  with Parser.PrefilterType[Query[_]]
[error]     |                            ^^^^^
[error]     |Reference to Query is ambiguous,
[error]     |it is both imported by import io.getquill.ast.{Ident as AIdent, Insert as AInsert, Update as AUpdate, Delete as ADelete, *}
[error]     |and imported subsequently by import io.getquill.*
[error] -- [E007] Type Mismatch Error: /home/dotty/dotty/community-build/community-projects/protoquill/quill-sql/src/main/scala/io/getquill/StaticSplice.scala:32:55
[error] 32 |        module        <- Try(untypedModule.asInstanceOf[StaticSplice[T]]).toEither.mapLeft(_.getMessage)
[error]    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |Found:    io.getquill.StaticSplice[T]
[error]    |Required: x$1.reflect.Term
[error]    |
[error]    |One of the following imports might make progress towards fixing the problem:
[error]    |
[error]    |  import io.getquill.Dsl.autoQuote
[error]    |  import io.getquill.autoQuote
[error] -- Error: /home/dotty/dotty/community-build/community-projects/protoquill/quill-sql/src/test/scala/io/getquill/FlicersSpec.scala:42:22
[error]  42 |      val r = ctx.run(q)
[error]     |                      ^
[error]     |Exception occurred while executing macro expansion.
[error]     |scala.quoted.runtime.impl.ScopeException: Expression created in a splice was used outside of that splice.
[error]     |Created in: quill-sql/src/main/scala/io/getquill/metaprog/etc/MapFlicer.scala:54 at column 79
[error]     |Used in: quill-sql/src/main/scala/io/getquill/metaprog/etc/MapFlicer.scala:48 at column 31
[error]     |Expr: field.toString()
[error]     |
[error]     |
[error]     |Creation stack:
[error]     |	quill-sql/src/main/scala/io/getquill/metaprog/etc/MapFlicer.scala:54 at column 79
[error]     |	quill-sql/src/main/scala/io/getquill/metaprog/etc/MapFlicer.scala:20 at column 176
[error]     |
[error]     |
[error]     |Use stack:
[error]     |	quill-sql/src/main/scala/io/getquill/metaprog/etc/MapFlicer.scala:48 at column 31
[error]     |	quill-sql/src/main/scala/io/getquill/metaprog/etc/MapFlicer.scala:20 at column 176
[error]     |
[error]     |
[error]     |Hint: A common reason for this to happen is when a `def` that creates a `'{...}`
[error]     |      captures an outer instance of `Quotes`. If this `def` is called in a splice
[error]     |      it will not track the `Quotes` provided by that particular splice.
[error]     |      To fix it add a `given Quotes` to this `def`.
[error]     |
[error]     |	at scala.quoted.runtime.impl.ScopeException$.checkInCorrectScope(ScopeException.scala:35)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$.quotedExprToTree(PickledQuotes.scala:39)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:111)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1452)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:134)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform$$anonfun$1(Trees.scala:1513)
[error]     |	at scala.collection.immutable.List.mapConserve(List.scala:472)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1513)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1418)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:134)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$.spliceTerms(PickledQuotes.scala:151)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$.unpickleTerm(PickledQuotes.scala:87)
[error]     |	at scala.quoted.runtime.impl.QuotesImpl.unpickleExprV2(QuotesImpl.scala:3044)
[error]     |	at io.getquill.metaprog.etc.MapFlicerMacro.fieldInject$1(MapFlicer.scala:48)
[error]     |	at io.getquill.metaprog.etc.MapFlicerMacro.$anonfun$1$$anonfun$1(MapFlicer.scala:54)
[error]     |	at io.getquill.metaprog.etc.MapFlicerMacro.$anonfun$1$$anonfun$adapted$1(MapFlicer.scala:54)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:109)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1452)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:134)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1484)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:134)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform$$anonfun$1(Trees.scala:1513)
[error]     |	at scala.collection.immutable.List.mapConserve(List.scala:472)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1513)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transformStats(Trees.scala:1509)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transformBlock(Trees.scala:1511)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1432)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:134)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform$$anonfun$1(Trees.scala:1513)
[error]     |	at scala.collection.immutable.List.mapConserve(List.scala:472)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1513)
[error]     |	at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1418)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$$anon$1.transform(PickledQuotes.scala:134)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$.spliceTerms(PickledQuotes.scala:151)
[error]     |	at dotty.tools.dotc.quoted.PickledQuotes$.unpickleTerm(PickledQuotes.scala:87)
[error]     |	at scala.quoted.runtime.impl.QuotesImpl.unpickleExprV2(QuotesImpl.scala:3044)
[error]     |	at io.getquill.metaprog.etc.MapFlicerMacro.$anonfun$1(MapFlicer.scala:54)
[error]     |	at scala.collection.immutable.List.map(List.scala:246)
[error]     |	at io.getquill.metaprog.etc.MapFlicerMacro.buildClause(MapFlicer.scala:56)
[error]     |	at io.getquill.metaprog.etc.MapFlicerMacro.base(MapFlicer.scala:62)
[error]     |	at io.getquill.metaprog.etc.MapFlicer$.applyImpl(MapFlicer.scala:23)
[error]     |	at io.getquill.metaprog.etc.MapFlicer$.inline$applyImpl(MapFlicer.scala:21)
[error]     |
[error]     |---------------------------------------------------------------------------
[error]     |Inline stack trace
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from MapFlicer.scala:20
[error]  20 |Quotes): Expr[Boolean] = {
[error]     |                                                                                                                                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from MapFlicer.scala:20
[error] 117 |efault)
[error]     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]     |This location contains code that was inlined from MapFlicer.scala:20
[error]  40 |        query[PersonFlatOpt].filterByKeys(keys)
[error]     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]      ---------------------------------------------------------------------------

griggt avatar May 12 '22 00:05 griggt

3. Rewritten code fails to cross-compile with some Scala 2.x compiler(s)

This was observed in cats-effect-3, scala-parallel-collections, scalacheck, ScalaPB, jackson-module-scala, FingerTree, scala-stm.

cats-effect-3:

--- a/kernel/js/src/main/scala/cats/effect/kernel/AsyncPlatform.scala
+++ b/kernel/js/src/main/scala/cats/effect/kernel/AsyncPlatform.scala
@@ -16,7 +16,7 @@

 package cats.effect.kernel

-import scala.scalajs.js.{|, defined, Function1, JavaScriptException, Promise, Thenable}
+import scala.scalajs.js.{defined, Function1, JavaScriptException, Promise, Thenable}

 private[kernel] trait AsyncPlatform[F[_]] { this: Async[F] =>
[info] compiling 35 Scala sources to /home/dotty/dotty/community-build/community-projects/cats-effect-3/kernel/js/target/scala-2.13/classes
[error] /home/dotty/dotty/community-build/community-projects/cats-effect-3/kernel/js/src/main/scala/cats/effect/kernel/AsyncPlatform.scala:28:44: not found: type |
[error]         val onFulfilled: Function1[A, Unit | Thenable[Unit]] =
[error]                                            ^

This is an interesting case because scalajs.js.| is actually special-cased in the compiler (see #11671):

https://github.com/lampepfl/dotty/blob/7c446ce8b2bf7031a6cf223b5f6a2782c1543016/compiler/src/dotty/tools/dotc/typer/Typer.scala#L257-L265

scala-parallel-collections:

Here, import renaming was rewritten to use as. Not sure why this occurred.

--- a/junit/src/test/scala/scala/collection/NewBuilderTest.scala
+++ b/junit/src/test/scala/scala/collection/NewBuilderTest.scala
@@ -13,7 +13,7 @@
 package scala.collection

 import scala.{collection => sc}
-import scala.collection.{mutable => scm, immutable => sci, parallel => scp, concurrent => scc}
+import scala.collection.{mutable as scm, immutable as sci, parallel as scp}
 import scala.collection.parallel.{mutable => scpm, immutable => scpi}

 import org.junit.runner.RunWith

The remainder of the Scala 2 cross-compile failures fall into two cases:

  1. removal of some language feature import that is required by some Scala 2.x version(s) but not Scala 3

Rewritten akka failed to build on 2.13 because it needs language.existentials in places, scalacheck needs language.higherKinds, and ScalaPB needs the removed language.implicitConversions imports.

  1. removal of a wildcard import from some version compatibility module which is not needed on Scala 3

I'm not sure what we can reasonably do here. It might be nice to have some way to indicate "this import is actually necessary, analysis be damned".

griggt avatar May 12 '22 00:05 griggt

Thanks. I have another rewrite of -rewrite to push, where it just locates the Import trees in the compilationUnit. The unfortunate part is reconstructing the group from spans (the first one includes the keyword).

Every bit of insight about ergonomics is invaluable. They call it ergonomics because you conclude, Ergo, this feature is useless.

Maybe language features can go behind an additional -Y flag. They might represent "policy" even if unused. Probably they don't change often.

The macros failures suggest I missed something important.

som-snytt avatar May 12 '22 02:05 som-snytt

Rebased, but I haven't looked at the latest comments from May 11 or commit from May 6. It looks like the follow-up is for locating selectors to rewrite, handling language imports (including version), and cross-compilation (such as import x.y as z).

If a language flag is required under Scala 2 but not 3, I don't think it's a goal here to differentiate? or my previous suggestions was yet another -Y flag to say lint the language imports, too.

som-snytt avatar Jun 23 '22 15:06 som-snytt

Tweaked the rewrite facility. It might be useful for projects who can't depend on scalafix, not that you can't depend on scalafix. It surely needs tweaks for weird multiline imports.

som-snytt avatar Sep 21 '22 02:09 som-snytt

  • Dude, please get rid of the red ex, finally.

  • OK, I'll try that.

bad option '-Yerased-terms' was ignored

som-snytt avatar Sep 23 '22 00:09 som-snytt

Reopen to mark import used after implicit ranking, preparatory to seeing if it works the same way on Scala 2.

som-snytt avatar Nov 10 '22 19:11 som-snytt

Not sure how the build diff, reverted in the last commit, caused the settings error -Yerased-terms. I'll save that puzzle for some future three-day weekend.

som-snytt avatar Nov 11 '22 02:11 som-snytt

For those who is also wondering why this one is closed, I believe the issue is superseded with https://github.com/lampepfl/dotty/pull/16157

ivan-klass avatar Jan 24 '23 11:01 ivan-klass