Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: Named args completions with default values #5657

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

jkciesluk
Copy link
Member

No description provided.

@@ -237,15 +237,22 @@ object NamedArgCompletions:
.getOrElse(baseArgs)
.filterNot(isUselessLiteral)

def isDefaultParam(t: Tree): Boolean = t match
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be wrong, but looking at the implementation maybe this could be marked as a @tailrec function? 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, thanks!

)

check(
"default-args5",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before 3.3.1, the path in this case looks way different -
After 3.3.1:

  Ident(name = CURSOR),
  Apply(
    fun = Apply(fun = Ident(name = deployment), args = List(Literal(const = ( = "str")))),
    args = List(Ident(name = CURSOR))
  ),
  ValDef(
    name = abc,
    tpt = TypeTree[dotty.tools.dotc.core.Types$UnspecifiedErrorType$@2de07d9c],
    preRhs = Apply(
      fun = Apply(fun = Ident(name = deployment), args = List(Literal(const = ( = "str")))),
      args = List(Ident(name = CURSOR))
    )
  )

Before:

  Ident(name = CURSOR),
  ValDef(
    name = DerivedName(underlying = opt, info = NumberedInfo(num = 1)),
    tpt = TypeTree[dotty.tools.dotc.core.Types$PreviousErrorType@44aa396b],
    preRhs = Ident(name = CURSOR)
  ),
  Block(
    stats = List(
      ValDef(
        name = DerivedName(underlying = opt, info = NumberedInfo(num = 1)),
        tpt = TypeTree[dotty.tools.dotc.core.Types$PreviousErrorType@44aa396b],
        preRhs = Ident(name = CURSOR)
      )
    ),
    expr = Block(
      stats = List(
        DefDef(
          name = $anonfun,
          paramss = List(
            List(
              ValDef(
                name = fst,
                tpt = TypeTree[TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class scala)),object Predef),type String)],
                preRhs = Thicket(trees = List())
              ),
              ValDef(
                name = snd,
                tpt = TypeTree[TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),class Int)],
                preRhs = Thicket(trees = List())
              )
            )
          ),
          tpt = TypeTree[AppliedType(TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),class Option),List(TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),class Int)))],
          preRhs = Apply(
            fun = Apply(
              fun = Apply(fun = Ident(name = deployment), args = List(Literal(const = ( = "str")))),
              args = List(Ident(name = DerivedName(underlying = opt, info = NumberedInfo(num = 1))))
            ),
            args = List(Ident(name = fst), Ident(name = snd))
          )
        )
      ),
      expr = Closure(env = List(), meth = Ident(name = $anonfun), tpt = Thicket(trees = List()))
    )
  )

I will investigate further what can be done for older version

@@ -19,7 +19,12 @@ trait ArgCompletions { this: MetalsGlobal =>
val editRange: l.Range =
pos.withStart(ident.pos.start).withEnd(pos.start).toLsp
val funPos = apply.fun.pos
val method: Tree = typedTreeAt(funPos)
val method: Tree = typedTreeAt(funPos) match {
case Apply(Block(defParams, app: Apply), _)
Copy link
Contributor

@filipwiech filipwiech Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: from my perspective a small comment explaining this case would be really welcome (like the very nice remarks in the isDefaultArg function). 🙂

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, could you also take a look if the signature help in those cases work correctly?

@jkciesluk
Copy link
Member Author

Looks good, could you also take a look if the signature help in those cases work correctly?

I've fixed it for Scala 2, for Scala 3 it worked fine.
There is one more bug in Scala 3 which should be fixed by scala/scala3#18594

@tgodzik
Copy link
Contributor

tgodzik commented Sep 26, 2023

Looks good, could you also take a look if the signature help in those cases work correctly?

I've fixed it for Scala 2, for Scala 3 it worked fine. There is one more bug in Scala 3 which should be fixed by lampepfl/dotty#18594

Ok, let's rebase then. I broke the CI before 😓

@@ -925,4 +925,144 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite {
|""".stripMargin,
)

// NOTE:
// Unignore after merging https://github.com/lampepfl/dotty/pull/18594
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been already merged a while ago, so maybe the test could be enabled now. 🙂

tgodzik added a commit to scala/scala3 that referenced this pull request Oct 6, 2023
@jkciesluk jkciesluk merged commit 2c75be6 into scalameta:main Oct 9, 2023
23 of 24 checks passed
@jkciesluk jkciesluk deleted the named-def-arg-issue branch October 9, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants