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

fix: Hover shows de-aliased type members #6251

Closed
wants to merge 1 commit into from

Conversation

pityka
Copy link

@pityka pityka commented Mar 24, 2024

Previously metals showed generic type members for arguments of anonymous functions.

This commit recursively de-aliases type references in HoverProvider.

Resolves #6230 for scala 2.13 and scala 3.3.3

image

Previously metals shows generic type members for arguments of anonymous functions.

This commit recursively de-aliases type references in HoverProvider.
@rochala rochala self-requested a review March 25, 2024 08:35
@rochala rochala self-assigned this Mar 25, 2024
@kasiaMarek
Copy link
Contributor

It seems a few tests have failed. @pityka, don't be bothered by macOS-latest jdk-17 unit and Scala CLI integration, tests that failed there are flaky and have nothing to do with your changes.

Copy link
Collaborator

@rochala rochala left a comment

Choose a reason for hiding this comment

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

Hey, firstly, thanks for the contribution you've made.

Your solution is almost correct, but it caused a regression.
I think that the reason is in a recursive dealiasing. We should be more careful of what we're trying to dealias.
https://github.com/scalameta/metals/pull/6251/files#diff-5e56146f7a5bddf85757c9a6408980a990a426b172b4a43f14e49be6d5962571R251-R260
I left you a comment which should help you with finishing the PR.

If you want you can also take the opportunity to rename the method to deepDealias as metalsDealias is super bad name :D

@@ -224,8 +258,11 @@ class HoverTermSuite extends BaseHoverSuite {
| } yield x
|}
|""".stripMargin,
"""|x: Int
|""".stripMargin.hover
"""|implicit def intWrapper(x: Int): RichInt
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like it's a regression.

@@ -29,6 +29,36 @@ object HoverProvider:
driver: InteractiveDriver,
search: SymbolSearch,
)(implicit reportContext: ReportContext): ju.Optional[HoverSignature] =
def dealias(tpe: Type)(using Context): Type = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a method called metalsDealias (it should be named deepDealias) which does similar thing to yours.
The fix should be done directly in that method.
This is a bit more controlled of what we're doing.

I recommend implementing this like that:

  extension (tpe: Type)
    def deepDealias(using Context): Type =
      tpe.dealias match
        case app @ AppliedType(tycon, params) =>
          // we dealias applied type params by hand, because `dealias` doesn't do it
          AppliedType(tycon, params.map(_.metalsDealias))
        case aliasingBounds: AliasingBounds => 
          aliasingBounds.derivedAlias(aliasingBounds.alias.dealias)
        case TypeBounds(lo, hi) => 
          TypeBounds(lo.dealias, hi.dealias)
        case RefinedType(parent, name, refinedInfo) => 
          RefinedType(parent.dealias, name, refinedInfo.deepDealias)
        case dealised => dealised

Copy link
Author

Choose a reason for hiding this comment

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

Hm.. I am getting a strange error in one of the java import tests. I haven't seen this before. Is this something known? This is 3.3.3. Thanks!

exception caught when loading class BoundMethodHandle$Specializer$Factory: java.lang.AssertionError: assertion failed: failure to resolve inner class:
externalName = java.lang.invoke.ClassSpecializer$Factory,
outerName = java.lang.invoke.ClassSpecializer,
innerName = Factory
owner.fullName = java.lang.invoke.ClassSpecializer
while parsing /modules/java.base/java/lang/invoke/BoundMethodHandle$Specializer$Factory.class
Mar 27, 2024 11:13:19 PM scala.meta.internal.pc.CompilerAccess handleError
SEVERE: assertion failed: failure to resolve inner class:
externalName = java.lang.invoke.ClassSpecializer$Factory,
outerName = java.lang.invoke.ClassSpecializer,
innerName = Factory
owner.fullName = java.lang.invoke.ClassSpecializer
while parsing /modules/java.base/java/lang/invoke/BoundMethodHandle$Specializer$Factory.class
java.lang.AssertionError: assertion failed: failure to resolve inner class:
externalName = java.lang.invoke.ClassSpecializer$Factory,
outerName = java.lang.invoke.ClassSpecializer,
innerName = Factory
owner.fullName = java.lang.invoke.ClassSpecializer
while parsing /modules/java.base/java/lang/invoke/BoundMethodHandle$Specializer$Factory.class
at scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:8)
at dotty.tools.dotc.core.classfile.ClassfileParser$innerClasses$.classSymbol(ClassfileParser.scala:1124)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you getting this on this branch ? Maybe try cleaning the project sbt clean and try again ?

@pityka
Copy link
Author

pityka commented Apr 29, 2024

Thank you for fixing the issue in an other PR! Closing this.

@pityka pityka closed this Apr 29, 2024
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.

Metals does not infer concrete type of path dependent types
3 participants