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

Support scaladoc auto completion on type `/**` #1250

Merged
merged 15 commits into from Jan 13, 2020

Conversation

@tanishiking
Copy link
Member

tanishiking commented Jan 4, 2020

Closes #1252

This PR enables the metals server to auto-complete scaladoc for method definition and class definition on type /**

vscode-scaladoc

Implement Overview

  • Enable to fire textDocument/completion on type * by setting triggerCharacters on initialize.
  • If the line only have a /**, try to complete scaladoc
    • Close the comment by inserting */ behind the cursor before adding the source to the compilation unit, so that metals can traverse whole AST in the source.
    • Find an associated member definition by traverse the AST for the source, and collect all MemberDef that is defined below the /**., and get the closest MemberDef that is defined below the /** as an associated definition.
    • Generate scaladoc based on the definition.

Questions

CursorPosition

Outdated :)

~Currently, after the completionEdit, the cursor will move to behind the first parameter like,

/**@@
def test(x: Int, y: Int): Int = ???

to

/**
  *
  * @param x | <- move the cursor to here like tsserver
  * @param y
  * @return
  */
def test(x: Int, y: Int): Int = ???

Is it ok to move the cursor here?

/**
  * | <- maybe we should move cursor here instead? like intellij-scala-plugin
  *
  * @param x
  * @param y
  * @return
  */
def test(x: Int, y: Int): Int = ???
/**
  * | <- move cursor here instead? like intellij-scala-plugin, is it ok?
  *
  * @param x
  * @param y
  * @return
  */
def test(x: Int, y: Int): Int = ???

Scaladoc or Javadoc?

Now, this PR add scaladoc with Javadoc style with gutter asterisks aligned in column three like:

/**
  * | <- move cursor here
  *
  * @param x
  * @return
  */
def method(x: Int): Int = ???

https://docs.scala-lang.org/style/scaladoc.html

But, IMO it's common to write scaladoc with Javadoc style, with gutter asterisks aligned in column two:

/**
 * | <- move cursor here
 *
 * @param x
 * @return
 */
def method(x: Int): Int = ???

Should we complete the doc that format instead?


  • I haven't checked the behavior on Windows yet, and I'm going to confirm the behavior later (after setting up the windows working environment) and check it with other language server client than VSCode
@@ -494,7 +494,7 @@ class MetalsLanguageServer(
capabilities.setCompletionProvider(
new CompletionOptions(
config.compilers.isCompletionItemResolve,
List(".").asJava
List(".", "*").asJava

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 4, 2020

Author Member

Enable to fire textDocument/completion on type *

.text()
.charAt(i - 1) == '*' && params.text().charAt(i - 2) == '/' =>
// Insert potentially missing `*/` to avoid comment out all codes after the "/**".
CURSOR + "*/"

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 4, 2020

Author Member

If we don't close the comment here, the whole code below the /** will be commented out, and we cannot traverse the AST (retrieved from a compilation unit) below the comment

case moduledef @ ModuleDef(_, _, _) => process(moduledef)
case pkgdef @ PackageDef(_, _) => process(pkgdef)
case valdef @ ValDef(_, _, _, _) => process(valdef)
case _ if treePos(t).includes(pos) => super.traverse(t)

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 4, 2020

Author Member

What we construct scaladoc for is just ClassDef and DefDef, but collecting all the MemberDef to avoid the following pattern.

/**@@
type Foo = Int // If we don't collect TypeDef

def foo(x: Int): Int = ???

to

/**
  *
  * @param x
  * @return
  */
type Foo = Int // If we don't collect TypeDef

def foo(x: Int): Int = ???

instead of

/**
  *
  */
@tanishiking tanishiking force-pushed the tanishiking:scaladoc-template-completion branch from 3bafed9 to 25641a7 Jan 4, 2020
traverse(root)
defs.sortBy(_.pos.point).headOption
}
override def traverse(t: Tree): Unit = {

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 4, 2020

Author Member

Since just traversing the AST of the source to find the associated definition, if the source has long lines, it takes a bit time before returning the completion response...

Copy link
Member

gabro left a comment

Amazing job @tanishiking! 👏

I've given a first pass and left a few comments here and there.

I'll leave @olafurpg comment on the general approach, since he's more expert than me on completions and possible performance bottlenecks.

|""".stripMargin,
"""|object A {
| /**
| *

This comment has been minimized.

Copy link
@gabro

gabro Jan 4, 2020

Member

The official docs also suggest the @constructor tag for classes. What do you think?

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 5, 2020

Author Member

I think that's a good idea👍 I'm going to implement it later!

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 9, 2020

Author Member

done in f4e93f4

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Jan 4, 2020

About your questions, I think it makes sense to use Scaladoc style and I would personally expect the cursor to move before the @param block, so that I can start typing a description.

Copy link
Member

olafurpg left a comment

Thank you for this contribution! This feature looks great. I haven't reviewed in depth yet but wanted to leave a few quick comments :)

}
}

class ConstructorFinder(clazz: ClassDef) extends Traverser {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

Can we move all the scaladoc-related logic into a separate file? For example ScaladocCompletions.scala. The Completions.scala file is already too large.

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 9, 2020

Author Member

Done in b6de5d9 👍
Is it ok to split those files in this way?

If it's ok, I'd like to split other modules into another file in another PR (like MatchKeyword Completion related logics into MatchKeywordCompletion.scala)(in this case, maybe we should dig a directory like completions and put those files in pc/completions/ScaladocCompletion.scala)

as mentioned in #672

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Jan 5, 2020

FYI, I've just used this feature to author the Scaladoc in this PR 😄

tanishiking added 4 commits Jan 5, 2020
This PR fixes to cases below

Case1 nested trait

before this commit, since trait doesn't have a constructor, constructor
finder would pick a constructor of the class defined inside the trait.

```scala
/**@@
trait X {
  case class A(x: Int) {}
}
```

```scala
/**
  * $0
  *
  * @param x
  */
trait X {
  case class A(x: Int) {}
}
```

To avoid this, if the associated class has a trait/abstract/interface
modifiers, do not try to find a constructor from its body.

---

Case2

Before this commit, some times the scaladoc completion for the class
that has a class inside its body is a bit unreliable.
Since constructor finder wes trying to find a constructor by traverse
the associated def's body, if the body has multiple constructors,
ConstructorFinder doesn't necessarily return its own constructor (maybe
it return the constructor for the class defined inside the body
instead).

```scala
/**@@
case class X(x: Int) {
  case class Y(y: Int) {}
}
```

```scala
/**
  * $0
  *
  * @param y <- instead of x
  */
```scala
/**@@
case class X(x: Int) {
  case class Y(y: Int) {}
}
```
@tanishiking

This comment has been minimized.

Copy link
Member Author

tanishiking commented Jan 5, 2020

Thank you so much for feedbacks! @olafurpg @gabro
It's still working in progress, but I'm going to address all feedbacks in a few days :)

tanishiking added 7 commits Jan 6, 2020
Since Completions.scala has already really big,
try to split scaladoc related logic into another file.
… params

Instead of finding the constructor by traversing the AST.
```scala
/**@@
def test[T: Ordering](t: T) = ???
```

shouldn't have something like `* @param evidence$1` and it should be

```scala
/**
  *
  *
  * @param t
  * @return
  */
def test[T: Ordering](t: T) = ???
```
Copy link
Member

gabro left a comment

Great work! I think we're really close to merging this :-)

I've left a couple of minor comments

pos.isDefined &&
text.charAt(pos.point - 3) == '/' &&
text.charAt(pos.point - 2) == '*' &&
text.charAt(pos.point - 1) == '*'
Comment on lines 147 to 150

This comment has been minimized.

Copy link
@gabro

gabro Jan 9, 2020

Member

maybe you can do

text.slice(pos.point - 3, 3) == "/**"

(I would double check the indexes :P )

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 10, 2020

Member

The current solution is fine.

@@ -197,4 +199,88 @@ object CompletionScaladocSuite extends BaseCompletionSuite {
|}
|""".stripMargin
)

checkEdit(
"scaladoc-defdef-returns-unit",

This comment has been minimized.

Copy link
@gabro

gabro Jan 9, 2020

Member

since this is the CompletionScaladocSuite I think you can avoid the scaladoc- prefix in all tests

// FIXME: All synthetic params should be filtered out by checking SYNTHETIC flag.
// evidence params for classs constructor look like they don't have SYNTHETIC flag.
!sym.isSynthetic &&
!sym.name.startsWith(nme.EVIDENCE_PARAM_PREFIX)
Comment on lines 128 to 131

This comment has been minimized.

Copy link
@gabro

gabro Jan 9, 2020

Member

if you're happy with the workaround, I would change the FIXME into NOTE. We tend to avoid FIXME comments as much as possible (with a few exceptions that point to specific GitHub issues)

@@ -336,7 +336,7 @@ trait Completions { this: MetalsGlobal =>
// variable but it avoids repeating traversals from the compiler
// implementation of `completionsAt(pos)`.
var lastVisistedParentTrees: List[Tree] = Nil
sealed abstract class CompletionPosition {
abstract class CompletionPosition {

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 9, 2020

Author Member

Removed a sealed modifier to split Scaladoc related logic into another file.
I think we cannot avoid removing this for splitting each CompletionPosition logics into other files, and IMO it's ok with removing this modifier because we don't use exhaustive match for CompletionPosition 🤔

// FIXME: All synthetic params should be filtered out by checking SYNTHETIC flag.
// evidence params for classs constructor look like they don't have SYNTHETIC flag.
!sym.isSynthetic &&
!sym.name.startsWith(nme.EVIDENCE_PARAM_PREFIX)

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 9, 2020

Author Member

I believed that we can filter out all synthetic parameter like evidence$1: Ordering[T] which is appended here https://github.com/scala/scala/blob/ba9701059216c629410f4f23a2175d20ad62484b/src/compiler/scala/tools/nsc/ast/parser/TreeBuilder.scala#L135
But only with isSynthetic filter, I couldn't filter out evidence$1 from params and this test failed 🤔

I think I have to dig deeper into this phenomenon, but I just patched it with filtering by EVIDENCE_PARAM_PREFIX this for now, as a workaround.

This comment has been minimized.

Copy link
@tanishiking

tanishiking Jan 11, 2020

Author Member

I'm still not 100% sure, but maybe this happens because the scalac type checker doesn't keep tree modifiers in sync with symbol flags for a class constructor. (actually, there's only IMPLICIT flag remains on the symbol of evidence on class constructor).

Maybe I can create an issue on scala/bug and fix (or maybe feature?) this, but let me keep this workaround and note this behavior on a comment for now :)


  • when a symbol for a method signature is created, modifiers that are defined as ValueParameterFlags survive during type checking.
    • Assigning modifiers to value parameters here
  • but, it looks like type-checker doesn't use ValueParameterFlags as modifiers mask for a class constructor. Just assigning inConstructorFlags maybe, and all modifiers from the tree don't remain in symbol definition.

FYI: The PR that makes evidence (for method definition symbol) SYNTHETIC is here scala/scala#2132

@gabro
gabro approved these changes Jan 11, 2020
Copy link
Member

gabro left a comment

LGTM! Amazing job 👏👏

* Rename "ScaladocCompletion" to "ScaladocCompletions" for consistency
  with the rest of the codebase.
* Mix ScaladocCompletions into MetalsGlobal instead of Completions.
* Remove "constructor" annotation for class docstrings, these are not
  generated by IntelliJ.
* Add test case for curried class definitions.
Copy link
Member

olafurpg left a comment

I took the liberty to push a few minor changes. I have not seen the `@constructorand annotation before and it's not used by IntelliJ so I removed it.
Screenshot 2020-01-13 at 07 33 38

Please let us know if you would like to keep the constructor annotation.

LGTM 👍 Great work @tanishiking !

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Jan 13, 2020

The @constructor tag was my idea (I've read it in the guide), but I have very little experience with Scaladoc. If it's not commonly used, then 👍 for not adding it.

@tgodzik tgodzik merged commit 97bc14d into scalameta:master Jan 13, 2020
12 checks passed
12 checks passed
windows-latest jdk-11 unit tests
Details
macOS-latest jdk-11 unit tests
Details
ubuntu-latest jdk-8 unit tests
Details
ubuntu-latest jdk-11 unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@tanishiking

This comment has been minimized.

Copy link
Member Author

tanishiking commented Jan 13, 2020

Thank you so much for the review and finishing up this PR @olafurpg @gabro !
I'm so excited to use this feature in the latest release 😄

@tanishiking tanishiking deleted the tanishiking:scaladoc-template-completion branch Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.