Construct arg completion regression in 2.13.16

[SethTisue]

caught by the Metals test suite at e.g. https://scala-ci.typesafe.com/job/scala-2.13.x-jdk17-integrate-community-build/2396/artifact/logs/metals-build.log

[metals] [info] tests.pc.CompletionArgSuite.contructor-param_2.13.16-bin-50d025b started
[metals] [error] ==> X tests.pc.CompletionArgSuite.contructor-param_2.13.16-bin-50d025b  0.208s munit.ComparisonFailException: tests/cross/src/test/scala/tests/pc/CompletionArgSuite.scala:622
[metals] [error] 621:
[metals] [error] 622:  check(
[metals] [error] 623:    "contructor-param",
[metals] [error] Obtained empty output!
[metals] [error] => Expected:
[metals] [error] xxx = : Int

according to git bisect with dbuild, the culprit PR is scala/scala#10740

I've also reproduced the problem outside of dbuild by locally publishing scalameta 4.12.1:

git checkout v4.12.1
sbt
set every version := "4.12.1"
++2.13.16-bin-50d025b!
semanticdbCompilerPlugin/publishLocal

after which, in the Metals repo, one can

++2.13.16-bin-50d025b!
cross/testOnly *CompletionArgSuite -- *contructor-param*

and see the failure

[SethTisue]

but one wants to add debugging output to the compiler, so that's a third repo, and this three-repo style of debugging gets tedious fast

so then I thought, perhaps I could replicate the test in the scala/scala repo and debug it there

there is indeed infrastructure for writing completion tests there, in test/files/presentation. you put special markers in a source file, for example /*!*/, to test a certain kind of completion at that point

however, the styles of completion supported (in nsc/interactive/CoreTestDefs.scala) seem to be limited to testing askScopeCompletionAt and askTypeCompletionAt and askAllSources

I don't see any way of testing the method that is actually being called here, which is the case Ident(name) => case in completionsAt (in nsc.interactive.Global)

      case Ident(name) =>
        val allMembers = scopeMembers(pos)
        val positionDelta: Int = pos.start - focus1.pos.start
        val subName = name.subName(0, positionDelta)
        CompletionResult.ScopeMembers(positionDelta, scopeMemberFlatten(allMembers), subName, forImport = false)

though this code it does call scopeMembers which is also called by askScopeCompletionAt, so maybe there's some hope there?

and now I'm not sure whether to:

1. go ahead and add more infrastructure (a new style of /* */ market that calls completionsAt) so that we can write a nice partest in the existing style
2. bypass partest and write a plain JUnit test for this
3. go back to the three-repo approach

[SethTisue]

though this code it does call scopeMembers which is also called by askScopeCompletionAt, so maybe there's some hope there?

Even at the v2.13.15 tag, this test:

package test

class Foo(xxx: Int)
object Foo {
  def apply(yyy: Int) = new Foo(yyy)
}

object Main {
  new Foo(/*_*/)
  Foo(/*_*/)
}

doesn't make Foo's constructor apply method show up in the .check file as I'd hoped:

reload: Completions.scala

askScopeCompletion at Completions.scala(9,10)
================================================================================
[response] askScopeCompletion at (9,10)
retrieved 4 members
class Foo extends AnyRef
def <init>(): test.Main.type
object Foo
object Main
================================================================================

askScopeCompletion at Completions.scala(10,6)
================================================================================
[response] askScopeCompletion at (10,6)
retrieved 4 members
class Foo extends AnyRef
def <init>(): test.Main.type
object Foo
object Main
================================================================================

which makes me wonder how this even works in Metals. In my trawl through Metals<->mtags<->scalameta it appeared to me that interactive.Global#completionsAt was doing all the work, but that could be a mistaken impression I got because I was going too fast, hoping to find a short path to a solution.

Note that the Metals test case doesn't work in the Scala REPL, which could be indirect evidence that Metals is adding its own functionality here.

[SethTisue]

Investigating from the Metals side a bit, the 2023 PR where these tests landed in the first place was scalameta/metals#5174 (by @kasiaMarek )

and a look at https://github.com/scalameta/metals/blob/main/mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala does indeed show that there is a lot of Metals-specific logic here

so I think I should abandon the hypothesis that completionsAt regressed and try to figure out what we could have changed (yes, probably positions!) on our side that could be leading the logic on the Metals side astray (perhaps in checkIfArgsMatch?)

(and now I wish I'd started at that PR in the first place)

an additional bit of archaeology is that the support seems to have first landed in scalameta/metals@0543a45

[lrytz]

@SethTisue thanks for digging.

I added some printlns here https://github.com/scalameta/metals/blob/v1.4.1/mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala#L21-L28

And compared 2.13.15 vs a current snapshot.

The apply AST is new Foo(<x_CURSOR_: error>), the apply.fun is new Foo.

The difference is in apply.fun.pos, on 2.13.15 it's RangePosition(A.scala, 79, 75, 82). That's a weird (buggy?) position because the point is outside the range. typedTreeAt(funPos) returns the Ident(Foo).

With a current snapshot the position is RangePosition(A.scala, 75, 75, 82). That looks like an improvement. typedTreeAt(funPos) returns new Foo, a New AST.

So probably the right fix is this?

--- a/mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala
+++ b/mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala
@@ -23,6 +23,7 @@ trait ArgCompletions { this: MetalsGlobal =>
       case Apply(Block(defParams, app: Apply), _)
           if defParams.forall(p => p.isInstanceOf[ValDef]) =>
         app
+      case New(c) => c
       case t => t
     }
     val methodSym = method.symbol

That makes the test pass.

[SethTisue]

nice, thanks! for community build purposes, I can apply that patch in a fork

wdyt @kasiaMarek @tgodzik ?

[SethTisue]

patched: https://github.com/scalacommunitybuild/metals/commits/community-build-2.13/

[tgodzik]

Sorry for the late reaction! I am porting it also here scalameta/metals#7047