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.

Sign up for GitHub

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

Jump to bottom

improvement: Use PC for synthetic decorations #5585

Merged
merged 6 commits into from
Nov 16, 2023
Merged

improvement: Use PC for synthetic decorations #5585

merged 6 commits into from
Nov 16, 2023

Conversation

jkciesluk
Copy link
Member

No description provided.

@jkciesluk jkciesluk marked this pull request as ready for review August 28, 2023 16:44
tgodzik
tgodzik requested changes Sep 29, 2023
View reviewed changes
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.

Great work! Sorry that it took mi so long to review

tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala Outdated Show resolved Hide resolved
tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala Show resolved Hide resolved
tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala Show resolved Hide resolved
tests/unit/src/test/resources/decorations3/example/Miscellaneous.scala Outdated Show resolved Hide resolved
tests/unit/src/test/resources/decorations3/example/GivenAlias.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/SyntheticDecorationsProvider.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/SyntheticDecorationsProvider.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/PcSyntheticDecorationsProvider.scala Outdated Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/SyntheticDecorationsSuite.scala Outdated Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/SyntheticDecorationsSuite.scala Outdated Show resolved Hide resolved
@jkciesluk jkciesluk force-pushed the pc-decoration branch 6 times, most recently from 31e5af9 to 4929fb0 Compare October 27, 2023 12:22
@jkciesluk jkciesluk requested a review from kasiaMarek October 31, 2023 08:44
kasiaMarek
kasiaMarek reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@kasiaMarek kasiaMarek left a comment

Choose a reason for hiding this comment

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

Generally, I have only minor comment, otherwise looks good. I'm guessing some unhandled cases will still show up but that's fine. One case is that 1 :: List(1) turns into 1 :: List[Int](1)[Int] for Scala 2.

mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java Outdated Show resolved Hide resolved
mtags-interfaces/src/main/java/scala/meta/pc/SyntheticOptions.java Outdated Show resolved Hide resolved
tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/TestingClient.scala Show resolved Hide resolved
tests/slow/src/test/scala/tests/feature/SyntheticDecorationsScala3ExpectSuite.scala Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/decorations/SyntheticHoverProvider.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/PcSyntheticDecorationProvider.scala Outdated Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/SyntheticDecorationsSuite.scala Outdated

// TODO: Examinate implicit conversion here and unignore
check(
"wrong-given-conversion".ignore,
Copy link
Contributor

@kasiaMarek kasiaMarek Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Similar case with case class A(x: Int, g: Int)(implicit y: String) ((y)(y) case class A(x: Int, g: Int)(implicit y: String)), my bet would be that you have to check if the function for which you add implicit params is not synthetic/has zero extent. Though I guess it make sense to show implicit params for implicit conversation, if both are visible...

Copy link
Member Author

Choose a reason for hiding this comment

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

my bet would be that you have to check if the function for which you add implicit params is not synthetic/has zero extent

For now I think we can do this and leave show implicit params for implicit conversation as a follow up

@jkciesluk
Copy link
Member Author

One case is that 1 :: List(1) turns into 1 :: ListInt[Int] for Scala 2.

Fixed!

@jkciesluk jkciesluk force-pushed the pc-decoration branch 2 times, most recently from 4218089 to 8ef3139 Compare November 6, 2023 08:45
tgodzik
tgodzik reviewed Nov 9, 2023
View reviewed changes
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! Just a few last questions and nitpicks

metals/src/main/scala/scala/meta/internal/decorations/SyntheticHoverProvider.scala
val position = params.getPosition
val line = position.getLine()
val isInlineDecorationProvider = clientConfig.isInlineDecorationProvider()
val newHover = currentDocument(path) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

The hover is coming from Semanticdb, but the decoration itself from presentation compiler, will that not be a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

Decorations from PC are at least as good as those from semantic db, so there shouldn’t be a case that there is a decoration from semanticdb and not from PC. It's possible that there will no hover on some decorations (ie. given arguments), because they are not in semanticdb synthetics. We could try to move hover to PC, but since we want to use inlay hints either way I don't think it's worth it?

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala Outdated Show resolved Hide resolved
mtags-shared/src/main/scala/scala/meta/internal/metals/CompilerSyntheticDecorationsParams.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/MetalsGlobal.scala Outdated Show resolved Hide resolved
tests/unit/src/test/scala/tests/decorations/SyntheticDecorationsLspSuite.scala
"""|object Main{
| val head: Double :: tail: List[Double] = List[Double](0.1, 0.2, 0.3)
| val List[Int](l1: Int, l2: Int) = List[Int](12, 13)
| println("Hello!")
| val abc: Int = 123
| val tupleBound @ (one: String, two: String) = ("1", "2")
| val tupleBound: (String, String) @ (one: String, two: String) = ("1", "2")
Copy link
Contributor

@tgodzik tgodzik Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally this should not be added, but we can remove it in a later PR, maybe an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

done: #5835

@jkciesluk jkciesluk requested a review from tgodzik November 13, 2023 13:41
tgodzik
tgodzik approved these changes Nov 14, 2023
View reviewed changes
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.

LGTM! O rerun the failed jobs, I don't think the failures are related. Feel free to merge

@jkciesluk jkciesluk merged commit c13c8b3 into scalameta:main Nov 16, 2023
24 checks passed
@jkciesluk jkciesluk mentioned this pull request Nov 17, 2023
Merged
tgodzik added a commit to scala/scala3 that referenced this pull request Nov 20, 2023
@tgodzik
presentation-compiler: Add synthetic decorations (#18951)
12ec5f9 
connected to scalameta/metals#5585
@kasiaMarek kasiaMarek mentioned this pull request Nov 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasiaMarek kasiaMarek kasiaMarek approved these changes

@tgodzik tgodzik tgodzik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jkciesluk @tgodzik @kasiaMarek