search

scalameta / metals

Scala language server with rich IDE features 🚀
https://scalameta.org/metals/
Apache License 2.0
2.07k stars 323 forks source link

Test failures in Scala 2.13 community build (changes in 2.13.12) #5438

Closed SethTisue closed 12 months ago

SethTisue commented 1 year ago

recently at e.g. https://scala-ci.typesafe.com/job/scala-2.13.x-jdk17-integrate-community-build/1254/artifact/logs/metals-build.log we're seeing:

[metals] [error] Failed tests:
[metals] [error]    tests.typedef.TypeDefinitionSuite
[metals] [error]    tests.pc.PcPrepareRenameSuite
[metals] [error]    tests.pc.InsertInferredTypeSuite
[metals] [error]    tests.pc.PcDefinitionSuite
[metals] [error]    tests.hover.HoverNegativeSuite
[metals] [error]    tests.pc.PcRenameSuite
[metals] [error]    tests.highlight.TypeDocumentHighlightSuite
[metals] [error]    tests.pc.CompletionDocSuite
[metals] [error]    tests.highlight.SyntheticsDocumentHighlightSuite
[metals] [error]    tests.tokens.SemanticTokensSuite
[metals] [error]    tests.pc.SignatureHelpDocSuite
[metals] [error]    tests.highlight.DocumentHighlightSuite

can I ask y'all to look at the failures and see whether you think they are due to a regression in Scala, or whether it's just changes that Metals needs to adjust to?

the Metals commit we're at is 56386a37138b145e41b042c760d3b0f1581087b3

which 2.13.12 PR might be the cause is unclear to me. Metals wasn't being run for a while while a Scalafmt issue was resolved. could be any of https://github.com/scala/scala/milestone/101?closed=1

@som-snytt any guesses? maybe https://github.com/scala/scala/pull/10375 ?

som-snytt commented 1 year ago

@SethTisue you can use the search terms is:pr author:som-snytt good deed is:closed to find out which went unpunished.

SethTisue commented 1 year ago

@som-snytt To be clear, I think the ball is in Metals' court on this — I hope you aren't off digging already. Unless you really want to :-)

som-snytt commented 1 year ago

A grave digger can't take a vacation because he's buried in work.

jkciesluk commented 1 year ago

Yes, I believe this is caused by https://github.com/scala/scala/pull/10375 The issue is that in Metals, we have our own namePos defined as extension method here https://github.com/scalameta/metals/blob/c2642c636a4163fcd306f93676915b28d9c38ab1/mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala#L702C7-L702C7, which is now shadowed by namePos method added in https://github.com/scala/scala/pull/10375/files#diff-b57d4b9dbee46888ae26125fbf2515d5b3b6a6268b3a1b19f631e8a2c24c8c63R295 We can just change the name of our extension method and everything should work, but I think there is an issue in Scala:

def <<method>>(a: Int): Int = ??? // namePos is correct
<<val someVal = 123>> // namePos includes whole ValDef

Maybe NamePos attachment isn't added to every ValDef (and maybe also TypeDef)?

SethTisue commented 1 year ago

We can just change the name of our extension method

Yes please, and I can pick up the change in the community build even without a release.

Maybe NamePos attachment isn't added to every ValDef (and maybe also TypeDef)?

@som-snytt wdyt?

som-snytt commented 1 year ago

@jkciesluk @SethTisue thanks for the heads up, I will PR a follow-up in time for 2.13.12. ~Until then, I recommend using only def, and maybe class if necessary.~

Also thanks for making the adjustment downstream.

Edit: deleted my joking advice, as the follow-up PR was not in time for 2.13.12.

SethTisue commented 12 months ago

It seems this was fixed.