-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Use presentation compiler for REPL tab completion #4725
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
Conversation
Fix cross talk between candidates in ImplicitComputation#findAll. Haoyi reported that he couldn't get the presentation compiler to offer completion suggestions for extension methods on primitive arrays. Turns out that an error in typechecking the previous candidate had not been cleared, and this was taken to mean that `arrayIntOps` was itself ineligible. After this patch: ``` scala> Array(1, 2, 3) reverse reverseIterator reverse reverseMap reversed scala> Array(1,2,3).reverse //print scala.Predef.intArrayOps(scala.Array.apply(1, 2, 3)).reverse scala> Array(1, 2, 3) to toString toCollection toSeq toIterator to toMap toSet toList toArray toBuffer toStream toIterable toTraversable toVector toIndexedSeq scala> Array(1, 2, 3) toSeq override def toSeq: Seq[Int] ```
The classic banner is available under -Dscala.repl.power.banner=classic. ``` scala> :power Power mode enabled. :phase is at typer. import scala.tools.nsc._, intp.global._, definitions._ Try :help or completions for vals._ and power._ ```
Transcript paste mode invites the user to keep typing like regular paste mode, but really you must enter more transcript. This matters if the script ends in the middle of incomplete code that the user wants to complete by hand. Previously, ``` scala> scala> def f() = { // Detected repl transcript paste: ctrl-D to finish. // Replaying 1 commands from transcript. scala> def f() = { scala> scala> def f() = { // Detected repl transcript paste: ctrl-D to finish. | } // Replaying 1 commands from transcript. scala> def f() = { } f: ()Unit ``` Now, ``` scala> scala> def f() = { // Detected repl transcript. Paste more, or ctrl-D to finish. // Replaying 1 commands from transcript. scala> def f() = { | 42 | } f: ()Int scala> f() res0: Int = 42 ```
The presentation compiler currently demands that all interaction is performed by asynchronous submission of work items, which are queued and executed on the presentation compiler thread. This is fairly inconvenient if you are a known-single-threaded client that is trying to use the compiler from your own thread. This commit adds an option to disable "assertCorrectThread" to better support this use case.
Conveniences added: - The client need not determine ahead of time whether it wants scope completions of type member completions, this is inferred from the tree at the cursor - Computes the delta from the cursor to point where a suggested completion item should be written. This includes finding the position of the name within a Select node, which is tricky. - Includes a matcher that matches results base on prefix, accessibility and suitability in the syntactic location. Type members are not offered in term positions, and term members only offered in type position if they could for a prefix for a type. E.g. `type T = glob<TAB>` should offer `global` to help writing `global.Tree`.
In preparation for use of the presentation compiler to drive REPL tab completion, add a dependency in the build. As far as the outside world is concerned, these are already comingled in scala-compiler.jar, and this patch doesn't change that.
val and var members of classes are split into getter/setter and field symbols. However, we can't call `defString` on any of these and see what existed in source code. Example: ``` scala> class X { protected var x: Int = 0 } defined class X scala> val xField = typeOf[X].member(TermName("x ")) xField: $r.intp.global.Symbol = variable x scala> xField.defString res10: String = private[this] var x: Int scala> xField.getterIn(xField.owner).defString res11: String = protected def x: Int scala> xField.setterIn(xField.owner).defString res12: String = protected def x_=(x$1: Int): Unit ``` This commit introduces a new method on `Symbol` that pieces together the information from these symbols to create an artificial symbol that has the `defString` we're after: ``` scala> xField.sugaredSymbolOrSelf.defString res14: String = protected var x: Int ```
The old implementation is still avaiable under a flag, but we'll remove it in due course. Design goal: - Push as much code in src/interactive as possible to enable reuse outside of the REPL - Don't entangle the REPL completion with JLine. The enclosed test case drives the REPL and autocompletion programatically. - Don't hard code UI choices, like how to render symbols or how to filter candidates. When completion is requested, we wrap the entered code into the same "interpreter wrapper" synthetic code as is done for regular execution. We then start a throwaway instance of the presentation compiler, which takes this as its one and only source file, and has a classpath formed from the REPL's classpath and the REPL's output directory (by default, this is in memory). We can then typecheck the tree, and find the position in the synthetic source corresponding to the cursor location. This is enough to use the new completion APIs in the presentation compiler to prepare a list of candidates. We go to extra lengths to allow completion of partially typed identifiers that appear to be keywords, e.g `global.def` should offer `definitions`. Two secret handshakes are included; move the the end of the line, type `// print<TAB>` and you'll see the post-typer tree. `// typeAt 4 6<TAB>` shows the type of the range position within the buffer. The enclosed unit test exercises most of the new functionality.
These commits, slated for 2.12, remove the old implementation and experiment with colorized completion suggestions: f279894...d05b097 We should also upgrade JLIne 2.13.1 when it is ready (scala/scala-dev#44) |
Looks like I've missed the |
Sorry I've been squeezed for time by life + work, the classic rock + hard place respectively. I think I included that SI-1931 commit b/c what it touches and likely conflicts, ugh. Including test output. I tried & failed one day to normalize the line numbering; I didn't try hard. I see some of the previous commits were squashed? I'm not partial to history that way, but hard to track at a distance. You've got to raise the barrel a bit and account for wind, then maybe you hit the target? etc. Soon we can use "resugar" without quotes. I mean resugar. I hope someday you can comment on an ammonite issue, "fixed in scala repl." Which I guess needs a name now, someone should create an issue. |
// Needed to import `xxx` during line 2 of: | ||
// scala> val xxx = "" | ||
// scala> def foo: x<TAB> | ||
importVars += stripped.toTermName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment has to precede the if statement. I noticed it, too, but hoped the typesetter would catch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. The last two lines were actually not needed.
Move misplaced and malindented comment and remove two lines of unneeded code.
@som-snytt I usually err on the side of submitting a history of commits that tells the story sub-feature-wise. In this case, given that the actual history was so meandering, and that all of the code here is new, I was a bit more squashy than usual. |
Oh, and I'm pretty sure that ammonite (and Scala IDE) will need to play catch up with |
Added a few commits as a result of @SethTisue's feedback; results are sorted, and we hide inherited members of the interpreter wrappers. |
This makes life easier for clients of these APIs, we use this to avoid passing this around in the wrapper result `TypeMembers`.
Trying harder to keep the synthetic interpretter wrapper classes behind the curtain
1d13b9a
to
3f1352f
Compare
I'm not going to include this in this PR, but I couldn't resist trying out some of this hole driven programming that the cool kids are raving about:
|
Another teaser: we can hijack keybindings in
Could use this for a more convenient version of |
This is just too useful to leave on the cutting room floor. ``` scala> classOf[String].enclo<TAB> scala> classOf[String].getEnclosing getEnclosingClass getEnclosingConstructor getEnclosingMethod scala> classOf[String].simpl<TAB> scala> classOf[String].getSimpleName type X = global.TTWD<TAB> scala> type X = global.TypeTreeWithDeferredRefCheck ``` I revised the API of `matchingResults` as it was clunky to reuse the filtering on accessibility and term/type-ness while providing a custom name matcher.
I've added a commit with a cleaned up version of the camel case completion. |
/rebuild |
the camel case stuff is great, and worked on everything I tried it on |
This isn't your fault, but right arrow sorts to the bottom. VBAR too. As opposed to category symbols coming first. Because we love symbols and encourage their use. OK, I see how the getCamel thing works, neat. I kind of think this should work: Basically, all my woes are due to caps; I was going to add ignorecase eventually. I have one machine where it's enabled in shell, another not because I'm lazy to look up the syntax, so I suffer. Yes, I suffer, but I appreciate it all the more when it works. So I'd want not only I guess we'd want You probably noticed it offers I don't suppose you'll be offering XML completion? The new name for Typesafe could be
It's biased to beans: More reasonably But should |
I've dropped the |
Just saw @lihaoyi's discussion about memory usage over at com-lihaoyi/Ammonite#216. It might be worth taking a look at the impact on memory usage of this PR. |
Regarding memory, we create and discard the presentation compiler at each press of TAB. This is in addition to the resident compiler that is held for the duration of the REPL session. I think this is fairly low risk: for applications that are embedding the interpreter and that might be memory sensitive, no cost is incurred (they don't trigger autocompletion). For interactive users with JLine, the short lifetime should avoid the extra objects surviving to the old generation. |
FWIW my memory usage conclusion wasn't wrt total memory held, but memory retention behavior. You guys set a max of 256mb somewhere in your she'll script, which works great, but I wanted to leave Ammonite's open ended because I've used it for some relatively large non-memory small-dara work. The interesting conclusion is that G1 keeps the heap smallish when the working set is smallish, whereas the default GC let's it grow to the max heap size, e.g. 4gb on my laptop, even if the working set never goes above 100-200mb. Ammonite keeps the pressy around but initializes it lazily, so it isn't part of that trial either =) |
thanks for checking the heap usage. i went through all the commits, it looks excellent. like everybody else, i can't wait to have it in :) |
LGTM |
/synch - maybe that helps with the cla status? -- yep it did -- but it also made scala-jenkins remove the "reviewed" label -- and it's quite stubborn about it. |
@lrytz I've removed the unused parameter you flagged and made another minor refactoring in the last commit. |
LGTM - Let's Get This Merged :) |
🍰 ! |
💦 what an effort! 🐫 crosses the REPL waste and also handles 🐫 case. 🕦 is when he was up checking the heap. |
The commit for SI-1931 went in under #4554 |
@@ -288,6 +290,8 @@ trait CompilerControl { self: Global => | |||
accessible: Boolean, | |||
inherited: Boolean, | |||
viaView: Symbol) extends Member { | |||
// should be a case class parameter, but added as a var instead to preserve compatibility with the IDE | |||
var prefix: Type = NoType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think source-compatibility should be ok... Do we pattern match on this? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think you do. We'd need to phase that out first
On Thu, 24 Sep 2015 at 6:05 PM, Iulian Dragos notifications@github.com
wrote:
In src/interactive/scala/tools/nsc/interactive/CompilerControl.scala
#4725 (comment):@@ -288,6 +290,8 @@ trait CompilerControl { self: Global =>
accessible: Boolean,
inherited: Boolean,
viaView: Symbol) extends Member {
- // should be a case class parameter, but added as a var instead to preserve compatibility with the IDE
- var prefix: Type = NoType
I think source-compatibility should be ok... Do we pattern match on this?
:)—
Reply to this email directly or view it on GitHub
https://github.com/scala/scala/pull/4725/files#r40294033.
This brings tab-completion in the REPL in line with the experience in IDEs and ENSIME-powered editors.
class C { def f(l: List[Int]) = l.<TAB>
(l: List[Int]).rro<TAB>
, it expands to(l: List[Int]).reduceRightOption
classOf[String].typ<TAB>
to getgetAnnotationsByType
,getComponentType
and othersdef f(s: Str<TAB>
//print
: tryfor (x <- 1 to 10) println(x) //print<TAB>
List(1,2,3).part<TAB>
, which completes toList(1,2,3).partition
; press tab again to displaydef partition(p: Int => Boolean): (List[Int], List[Int])
get
: try(d: java.util.Date).day<TAB>
!Review by @som-snytt