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

REPL tab completion: skip deprecated things until double-tab #7869

Closed
wants to merge 1 commit into from

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Mar 13, 2019

motivation: it's a good change in general, I believe.

but also, this is motivated by all the new deprecations in 2.13. lots of people are trying out the new collections API; some of them will be doing it in the REPL; let's not confuse those people and make our API look bad by offering them deprecated completions

this is still a draft because although a lot of things work already, there are a couple commented-out test cases that fail that I'd like to attempt to fix as well (UPDATE: one of them was scala/bug#10152 rather than something I did wrong)

also I might still decide to target 2.12.x instead. I can't add isDeprecated to api.Symbol there, but that's nbd nah

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Mar 13, 2019
@SethTisue
Copy link
Member Author

@som-snytt @retronym any thoughts on this, as I finish it up?

@SethTisue
Copy link
Member Author

@olafurpg do your presentation-compiler plans for Metals involve the REPL tab completion code, or do you (will you) be using other mechanisms to do completion?

@SethTisue SethTisue changed the base branch from 2.13.x to 2.12.x March 18, 2019 12:29
@olafurpg
Copy link
Member

@SethTisue we currently use interactive.Global.completionsAt in Metals. Is that affected by this PR? I had at one point copy-pasted the implementation of completionsAt to customize its behavior but managed to avoid it.

@SethTisue SethTisue changed the base branch from 2.12.x to 2.13.x March 18, 2019 12:36
@SethTisue
Copy link
Member Author

SethTisue commented Mar 18, 2019

we currently use interactive.Global.completionsAt in Metals. Is that affected by this PR?

yes, and there's also @retronym's #7833

if REPL tab completion improvements help Metals too, that's good to know, that increases my motivation (& everyone's motivation) to perhaps make further improvements

@olafurpg
Copy link
Member

I would like to build on top of completionsAt as much as possible and so far it's been working great (and consistently across 2.11/2.12 🙏). The customizations we have made in Metals are mostly adding more items to the completionsAt result, for example

It's normal that those completions are not covered by the completionsAt API because they require the ability to transform the original source file (insert imports, convert string literal).

@olafurpg
Copy link
Member

olafurpg commented Mar 18, 2019

Since we use completionsAt, all improvements to that method will benefit Metals (+ ScalaFiddle and Ammonite) so please keep them coming 😄 We actually have a few improvements in Metals that might be worth contributing

  • type members of classes that extend scala.Dynamic are not included in completionsAt by default, we work around it in Metals with a custom fallback traverser.
  • scope completions sometimes break inside local blocks when compiler plugins like kind-projector are enabled, we work around it in Metals by overriding interactive.Global.addContext() to only record contexts during the typer and namer phases

@szeiger szeiger modified the milestones: 2.13.1, 2.13.2 Aug 30, 2019
@SethTisue SethTisue modified the milestones: 2.13.2, 2.13.3 Feb 6, 2020
@SethTisue SethTisue added the tool:REPL Changes in the Scala REPL Interpreter label Feb 22, 2020
@SethTisue SethTisue force-pushed the tab-completion-deprecated branch 2 times, most recently from c8f7d81 to 9c7431f Compare February 22, 2020 08:38
@SethTisue
Copy link
Member Author

so the changes I made here actually don't affect completionsAt at all, they only affect code downstream from that, in PresentationCompilation. @olafurpg if you wanted to detect deprecated things in Metal and handle them differently, we would have to refactor (in a separate PR)

@SethTisue SethTisue marked this pull request as ready for review February 22, 2020 08:45
@SethTisue
Copy link
Member Author

I found that sometimes but not always (not sure what determines it), the compiler creates a symbol for a companion object that doesn't exist and enters it as a member, which then turns up in tab completion. an example is scala.collection.DefaultMap. it has no companion, yet:

scala 2.13.1> :power
scala 2.13.1> definitions.ScalaPackage.info.member(TermName("collection"))
res0: $r.intp.global.Symbol = package collection
scala 2.13.1> .info.member(TermName("DefaultMap"))
res1: $r.intp.global.Symbol = object DefaultMap
scala 2.13.1> res1.exists
res2: Boolean = false

it would be better if we never offered these nonexistent companions as completions at all, but I kind of half-assed it in this PR by treating them as if they were deprecated: they are omitted on single-tab but then included again after double-tab.

I accomplish that by checking sym.exists. (thankfully, this returns false on these nonexistent companions.) I'm a bit concerned that check could be too broad. might it inappropriately exclude anything?

@olafurpg
Copy link
Member

We want to get completions for deprecated symbols since LSP has a Boolean to mark that a completion item is deprecated and it renders like this in vscode

Ideally, we could call “completionsAt” with some configuration object to prevent regressions caused by changes like this. It’s not only Metals that uses this method, it’s also used by other tools like ScalaFiddle, Ammonite and more.

@SethTisue
Copy link
Member Author

SethTisue commented Feb 22, 2020

@olafurpg okay, I'll keep that in mind if I decide to do any followup PRs in this area. this PR is safe.

@som-snytt
Copy link
Contributor

strike-thru rendering for deprecations is an old ask for REPL.

scala/bug#4229

@SethTisue
Copy link
Member Author

after the JLine 3 upgrade lands, we'll have the option of offering completions in named groups. so one of the groups could be "deprecated"

@SethTisue
Copy link
Member Author

I see no reason to keep this open in the PR queue. I'll reopen it when the time comes

@SethTisue SethTisue closed this Mar 8, 2020
@SethTisue SethTisue removed this from the 2.13.3 milestone Mar 8, 2020
@SethTisue
Copy link
Member Author

after the JLine 3 upgrade lands, we'll have the option of offering completions in named groups. so one of the groups could be "deprecated"

another possibility would be to set the descr field of org.jline.reader.Candidate to "deprecated". this would show up in the UI as e.g.:

Screen Shot 2020-04-02 at 6 21 03 PM

@SethTisue
Copy link
Member Author

Since 2.13.3, we have the version with the "(deprecated)" notices. Ideally, deprecated methods would be presented last, in their own group. That may yet happen under scala/bug#12272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool:REPL Changes in the Scala REPL Interpreter
Projects
None yet
5 participants