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

Use presentation compiler for type at point information #21

Merged
merged 4 commits into from
Nov 10, 2017
Merged

Conversation

gabro
Copy link
Member

@gabro gabro commented Nov 9, 2017

Just an experiment for now. Some things are better (e.g. generics are correctly inferred), but some just got weird, and that's probably me knowing nothing about the compiler's api.

val r = new Response[compiler.Tree]
val results = compiler.askTypeAt(position, r)
val typedTree = r.get.left.get
typedTree.tpe.toString
Copy link
Member Author

@gabro gabro Nov 9, 2017

Choose a reason for hiding this comment

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

This is the relevant bit.

In this example:

val user = User(name = "foo", age = 42)
List(user).map(x => x.name)

hovering on:

  • user --> <notype>
  • User --> (name: String, age: Int)a.User
  • List --> ([A](xs: A*)List[A]
  • map --> [B, That](f: a.User => B)(implicit bf: scala.collection.generic.CanBuildFrom[List[a.User],B,That])That
  • x --> <notype>

essentially everything got worse apart from map but I think I may be using the typedTree wrong

@@ -71,6 +72,22 @@ class Compiler(
.distinct
}
}

def typeAt(path: AbsolutePath, line: Int, column: Int): Option[String] = {
Copy link
Member

Choose a reason for hiding this comment

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

How would you say the types compare to denotation.info? One thing we can do with denotation.info later down the road is to improve the pretty-printing so that some types are printed like mutable.Set and renamings in the source file are respected.

Copy link
Member Author

Choose a reason for hiding this comment

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

See my other comment, it's pretty bad as of now.
The reason is that I'm taking the .tpe from the typed tree without analyzing which tree it is.
If I start discriminating on the trees it gets much better (e.g. if it's a val declaration, take the tpe of the rhs)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just pushed a new commit, as an example of what I mean.

Copy link
Member

Choose a reason for hiding this comment

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

Does this work for unsaved buffers that don't fully typecheck?

@gabro
Copy link
Member Author

gabro commented Nov 9, 2017

Here's an example (after the typeOfTree thingy)

I'm really not sure of what should happen on synthetic .apply (like List and User in the example). Also I'm not happy with user.name (when hovering on user it says a.Foo.user.type)

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

If this works good and handles open buffers then this is a nice improvements 👍

r
}

private def typeOfTree(c: Global)(t: c.Tree): Option[String] = {
Copy link
Member

Choose a reason for hiding this comment

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

Does g.showCode(Tree) give any better results? I recall it handling some cases a bit better while implementing the semanticdb-scalac type printer https://github.com/scalameta/scalameta/blob/c0b7b7df565115b39e0c1f6b6d39d307bef0b4ed/scalameta/semanticdb-scalac/src/main/scala/scala/meta/internal/semanticdb/PrinterOps.scala#L350-L458

@gabro gabro mentioned this pull request Nov 10, 2017
@gabro
Copy link
Member Author

gabro commented Nov 10, 2017

Thanks to a suggestion by @olafurpg, using .widen made everything significantly better!

case None => Hover(Nil, None)
case Some((pos, denotation)) =>
case Some(tpeName) =>
Copy link
Member

Choose a reason for hiding this comment

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

Is it 100% sure that tpeName is never empty? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried hovering on everything and it never showed empty boxes 💃

Of course I may be wrong ^^

Copy link
Member

Choose a reason for hiding this comment

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

I would still add a check here, if it's not an a priori known fact that tpeName cannot be empty.

Another question here: why did you remove the range parameter?

Copy link
Member Author

@gabro gabro Nov 10, 2017

Choose a reason for hiding this comment

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

I didn't find an easy way to retrieve that from the PC. I admit I didn't look hard for it, because vscode seems to handle it pretty well even without it.

Copy link
Member Author

Choose a reason for hiding this comment

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

As per the check, it'd be weird that tree.tpe.widen would return an empty string. I suppose if it ever comes up we can handle it pretty easily :)

Copy link
Member

Choose a reason for hiding this comment

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

Works great! 💯 So cool!
And yes, I thought that without range symbols won't be highlighted correctly, but both VS Code and Atom handle it very well.

@gabro gabro merged commit 65831cb into master Nov 10, 2017
@gabro gabro deleted the hover-pc branch November 10, 2017 15:05
This pull request was closed.
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