chore: migrate Metals to 2.13 #3631

ckipp01 · 2022-02-13T14:03:49Z

No description provided.

ckipp01 · 2022-02-13T20:04:12Z

tests/unit/src/main/scala/scala/meta/internal/metals/WorkspaceSources.scala

 /**
 * Finds sources in the workspace when there is no available build tool.
 */
 final class WorkspaceSources(workspace: AbsolutePath) {
-  def all: Traversable[AbsolutePath] = {
+  def all: Generator[AbsolutePath] = {


I had some issues getting walkFileTree to work as an Interable in the same way that it is now. Then it sort of dawned on me that all here is only ever called in one place, inside of indexWorkspace which itself is only ever called in WorkspaceFuzzBench.scala and BaseWorkspaceSymboleSuite.scala to index a workspace with a zip it seems. I assumed we have the custome File Visitor to be able to skip dirs that start with . and not go down their subtrees, but seeing that this is only being used in tests it seemed way way easier to instead just use the built in listRecursive on AbsolutePath and just deal with the Generator that is returned. Is this ok or am I missing something?

ckipp01 · 2022-02-17T16:49:41Z

Alright, we are finally all ✅, but I did cheat a bit and a couple things are ignored. I'll outline them below and continue to look at them, but I think others can now take a look over this.

DefinitionCrossLspSuite.uderscore - This if failing in CI but I can't mimic this locally. I need to add some debugging in here for CI and try to figure out what's going on
DefinitionLspSuite.annotations - I need to replace this. circe-derication seems to work a bit different now and doesn't really show what we were testing before. I need to replace this with something else
WorkspaceSymbolLspSuite.dependencies - I'm having a ton of issues with this test. Between local and CI the order of some of the Options are different. NOTE: I ended up just marking this as .flaky. I can't figure it out. I don't get why the order is changing here as I can consistently get it to pass locally, but then not in CI. Also the order I get locally in a project testing is different than what passes the test as well. They are all the same results, just the ones that are on the same line don't seem to be consistent.

One other thing we need to figure out is when do we want to pull the trigger on this? If we do it right in the middle (like now) it'll be a mess for client that need to resolve the metals artifact. It might be best to do a release, and then right after the release, merge this so that anything 0.11.2 or great can use 2.13 and everything else 2.12. wdyt? If that's fine, then I'd suggest we do a release soon so we don't have to keep this up to date with every pr, especially seeing that there are some other larger prs open.

One other thing for maybe a follow-up pr. We default to Seq a lot. Should we be? Should we maybe instead be using Vector?

tgodzik · 2022-02-17T16:52:26Z

One other thing for maybe a follow-up pr. We default to Seq a lot. Should we be? Should we maybe instead be using Vector?

Maybe? That depends on the situation I guess, small lists are also well optimized.

Also, we can do a release soon, maybe with the bump to Bloop 1.4.13

ckipp01 · 2022-03-08T17:14:02Z

Seeing that we just released, what do you all think of trying to get this merged in right away. That will make it much easier for clients since anything > 0.11.2 can look for 2.13 whereas everything else can be on 2.12. Doing it right after a release will result in the smoothest transition.

There are basically 2 tests that I can't figure out that are unmarked above. One seems to be related to ordering and the other one I can reproduce locally. Other than that, I believe everything should be good to go.

tgodzik

Wow! This was a lot of great work! I left some comments, but nothing blocking, but I would do a follow up issue with things we still needed to improve, but this PR is already huge, so we can work on that later.

tests/slow/src/test/scala/tests/feature/DefinitionCrossLspSuite.scala

tgodzik · 2022-03-08T18:07:41Z

tests/unit/src/test/resources/semanticdb/example/Definitions.scala

  )
-  println/*scala.Predef.println(+1).*/(MacroAnnotation/*example.MacroAnnotation.*/.decodeMacroAnnotation/*example.MacroAnnotation.decodeMacroAnnotation.*/)
-  println/*scala.Predef.println(+1).*/(MacroAnnotation/*example.MacroAnnotation.*/.encodeMacroAnnotation/*example.MacroAnnotation.encodeMacroAnnotation.*/)
+  println/*scala.Predef.println(+1).*/(deriveDecoder/*io.circe.derivation.package.deriveDecoder().*//*local0*/[MacroAnnotation/*example.MacroAnnotation#*/])


Is /*local0*/ expect actually, is this coming from an implicit parameter?

Yea this is another one I had a tricky time with since the way circe derivation was working here in 2.13 was way different so I did what I thought would test the same thing, but I'm actually a bit unsure.

tgodzik · 2022-03-08T18:10:13Z

tests/unit/src/test/scala/tests/DefinitionLspSuite.scala

          |    ]
          |  }
          |}
          |/a/src/main/scala/a/User.scala
          |package a
-          |import io.circe.derivation.JsonCodec
-          |@JsonCodec case class User(name: String)


Why is the code changed here?

So the @JsonCodec wasn't working the same way anymore and not actually showing an annotation the way we were testing before. So in order to still have this test show a working goto definition with an annotation I switched this to use Alex's dataclass library so we can use @data and see that the @data creates an apply that we can do a goto definition on.

tests/unit/src/test/scala/tests/WorkspaceSymbolLspSuite.scala

tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala

tgodzik · 2022-03-08T18:17:04Z

tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala

        s"""|**Synthetics**:
            |
-            |([fallbackStringCanBuildFrom](command:metals.goto?$fallbackParams)[[Int](command:metals.goto?$intParams)])
+            |([Char](command:metals.goto?$orderingParams))
+            |[[Char](command:metals.goto?$charParams)]


Why is this on separate lines?

To be honest I'm not really sure.

tests/unit/src/test/scala/tests/decorations/SyntheticHoverLspSuite.scala

ckipp01 · 2022-03-08T18:30:56Z

Wow! This was a lot of great work! I left some comments, but nothing blocking, but I would do a follow up issue with things we still needed to improve, but this PR is already huge, so we can work on that later.

Sounds good! I can for sure follow this up with a couple tickets and missing points. I'm actually on holiday at the moment, so I might not be as responsive the next couple days until I get back, but wanted to at least bring attention to this right away since it'll be the easiest if we merge it as soon as we can. If you don't see any blockers feel free to merge, or if you do and still want to merge quickly, feel free to push on top of this. If not, I'll address them in a couple days and get to your questions.

kpodsiad

Let's merge this!

Duhemm · 2022-04-18T15:43:32Z

It looks like this PR introduced a regression in the indexing performance on Linux for large repositories 🤔

With org.scalameta:metals_2.12:0.11.2+1-444043ef-SNAPSHOT, my workspace indexes in 4 seconds
With org.scalameta:metals_2.13:0.11.2+3-105f3501-SNAPSHOT, my workspace indexes in 50 minutes 🤷‍♂️

I'm not sure why that is yet, I'll add more details once I figure it out!

tgodzik · 2022-04-19T09:07:14Z

Is that just on linux? The biggest change would probably be the Scala collections, so maybe anything related to that?

Duhemm · 2022-04-19T09:38:11Z

Yes it's just on Linux. I've removed as many redundant Scala collections ops as I could (I'll open a PR), but that didn't help at all. I'm still investigating...

Duhemm · 2022-04-19T14:06:54Z

I finally figured it out, and it was entirely caused by our environment 😮‍💨 🤦

Sorry for the noise and thanks for the great work 🚀

ckipp01 · 2022-04-19T14:25:39Z

I've removed as many redundant Scala collections ops as I could (I'll open a PR)

😅 please feel free to still pr the removal of the redundant Scala ops if you still have the changes

ckipp01 mentioned this pull request Feb 13, 2022

[WIP] Migrate to 2.13 #3475

Closed

ckipp01 force-pushed the migrate213 branch from 8951cf1 to f01087c Compare February 13, 2022 14:11

ckipp01 marked this pull request as draft February 13, 2022 14:34

ckipp01 force-pushed the migrate213 branch from f01087c to ef0186d Compare February 13, 2022 19:57

ckipp01 commented Feb 13, 2022

View reviewed changes

ckipp01 force-pushed the migrate213 branch 12 times, most recently from ae1762e to c8328c9 Compare February 17, 2022 15:47

ckipp01 marked this pull request as ready for review February 17, 2022 16:49

ckipp01 force-pushed the migrate213 branch 7 times, most recently from c877514 to 5c9b899 Compare February 18, 2022 08:21

ckipp01 force-pushed the migrate213 branch 2 times, most recently from 8173f1f to b3e1636 Compare March 8, 2022 17:11

ckipp01 requested review from tgodzik, dos65, tanishiking and kpodsiad March 8, 2022 17:14

tgodzik approved these changes Mar 8, 2022

View reviewed changes

ckipp01 mentioned this pull request Mar 8, 2022

Ignored DefinitionCrossLspSuite test fails in CI but not locally. #3688

Closed

ckipp01 force-pushed the migrate213 branch from b3e1636 to 97391ca Compare March 8, 2022 18:58

chore: migrate Metals to 2.13

35c84d4

ckipp01 force-pushed the migrate213 branch from 97391ca to 35c84d4 Compare March 8, 2022 19:05

kpodsiad approved these changes Mar 9, 2022

View reviewed changes

tgodzik merged commit 105f350 into scalameta:main Mar 9, 2022

ckipp01 deleted the migrate213 branch March 9, 2022 14:49

ckipp01 mentioned this pull request Mar 9, 2022

feat: support new 2.13 artifacts scalameta/nvim-metals#346

Merged

tanishiking mentioned this pull request Mar 10, 2022

feat: support new 2.13 artifacts scalameta/metals-languageclient#429

Merged

tgodzik mentioned this pull request Mar 11, 2022

Migrate codebase to 2.13 #1974

Closed

ayoub-benali mentioned this pull request Apr 19, 2022

support new 2.13 artifacts scalameta/metals-sublime#87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate Metals to 2.13 #3631

chore: migrate Metals to 2.13 #3631

ckipp01 commented Feb 13, 2022

ckipp01 Feb 13, 2022 •

edited

Loading

ckipp01 commented Feb 17, 2022 •

edited

Loading

tgodzik commented Feb 17, 2022 •

edited

Loading

ckipp01 commented Mar 8, 2022 •

edited

Loading

tgodzik left a comment

tgodzik Mar 8, 2022

ckipp01 Mar 8, 2022

tgodzik Mar 8, 2022

ckipp01 Mar 8, 2022

tgodzik Mar 8, 2022

ckipp01 Mar 8, 2022

ckipp01 commented Mar 8, 2022 •

edited

Loading

kpodsiad left a comment

Duhemm commented Apr 18, 2022

tgodzik commented Apr 19, 2022

Duhemm commented Apr 19, 2022

Duhemm commented Apr 19, 2022

ckipp01 commented Apr 19, 2022

chore: migrate Metals to 2.13 #3631

chore: migrate Metals to 2.13 #3631

Conversation

ckipp01 commented Feb 13, 2022

ckipp01 Feb 13, 2022 • edited Loading

Choose a reason for hiding this comment

ckipp01 commented Feb 17, 2022 • edited Loading

tgodzik commented Feb 17, 2022 • edited Loading

ckipp01 commented Mar 8, 2022 • edited Loading

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik Mar 8, 2022

Choose a reason for hiding this comment

ckipp01 Mar 8, 2022

Choose a reason for hiding this comment

tgodzik Mar 8, 2022

Choose a reason for hiding this comment

ckipp01 Mar 8, 2022

Choose a reason for hiding this comment

tgodzik Mar 8, 2022

Choose a reason for hiding this comment

ckipp01 Mar 8, 2022

Choose a reason for hiding this comment

ckipp01 commented Mar 8, 2022 • edited Loading

kpodsiad left a comment

Choose a reason for hiding this comment

Duhemm commented Apr 18, 2022

tgodzik commented Apr 19, 2022

Duhemm commented Apr 19, 2022

Duhemm commented Apr 19, 2022

ckipp01 commented Apr 19, 2022

ckipp01 Feb 13, 2022 •

edited

Loading

ckipp01 commented Feb 17, 2022 •

edited

Loading

tgodzik commented Feb 17, 2022 •

edited

Loading

ckipp01 commented Mar 8, 2022 •

edited

Loading

ckipp01 commented Mar 8, 2022 •

edited

Loading