Fix standard library symbols completion (solves #9760) #9805

prolativ · 2020-09-16T08:35:17Z

Symbols which are available in scope by default like println or Runnable were not suggested by LSP completion feature.
Some symbols like Seq or BigDecimal were suggested neither by LSP nor in REPL.
This PR solves both these issues.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

tgodzik

Looks good! It would be good to merge it also to 0.27.0. It's broken in 0.27.0-RC1, what is the rule with fixing release candidates?

smarter · 2020-09-18T16:13:58Z

I haven't checked but I assume this regressed in 6990aab, /cc @TheElectronWill

odersky · 2020-09-19T08:03:54Z

compiler/src/dotty/tools/dotc/core/Definitions.scala

@@ -1284,7 +1284,9 @@ class Definitions {

  private val ScalaImportFns: List[RootRef] =
    JavaImportFns :+
-    RootRef(() => ScalaPackageVal.termRef)
+    RootRef(() => ScalaPackageVal.termRef) :+
+    RootRef(() => ScalaPackageObject.termRef)


This just makes compilation slower. An import from ScalaPackageVal will automatically import members of scala.package. On the other hand, now we have one more import to search for every root reference.

What breaks if we do not do this change?

Without this change the PR doesn't fix anything. But I agree that we shouldn't need it.

As stated in the PR description, this PR fixes 2 issues (caused by 2 separate things in the code base but influencing users in a similar way):

Completion of symbols from scala package object, like Seq or BigDecimal

Completion of other predefined symbols (from Predef and top level definitions from scala package), like println

The latter issue is indeed a regression introduced in the PR that @smarter mentioned and it occurs only in LSP. The former issue must be older, even though nobody else seems to have spotted it before, it affects both LSP and REPL and is fixed specifically by this line (although this itself wouldn't fix LSP without the other fix).
I'm not really sure how the problem could be solved without adding this line. Interestingly this only changes the behaviour of symbols completion. Symbols like Seq or BigDecimal cause no problems in compilation and symbols completion should probably have exactly the same view of symbols in scope as the compiler.

Ah right, I forgot the other issue. Adding root imports to the context like you did is indeed the right solution for this second issue.

odersky

If we can make it work without the change in ScalaImportFns the rest LGTM

TheElectronWill

Actually, just adding RootRef(() => ScalaPackageObject.termRef) to ScalaImportFns solves the issue (for the REPL, at least). And adding scala root imports to the rootContext (to end up in a state similar to before #9394) doesn't. So the real issue seems to be somewhere else 🤔

prolativ · 2020-09-23T11:01:18Z

@odersky @smarter as after the discussion it looks like adding that import function is the proper solution, can we merge this PR? Or is the compilation slowdown so significant that we need to find some hack for that as a workaround?

smarter

I think the addition of ScalaPackageObject in the root import is a workaround for an underlying but that needs to be fixed: when importing a package, we should also get completions for members of the package object. For example:

import scala.collection._
val foo: TraversableOnc<TAB>

This should find TraversableOnce since it's defined in https://github.com/scala/scala/blob/d003bf5159acaca883f90e1be822bef9316639de/src/library/scala/collection/package.scala#L23, but it doesn't find anything. If I explicitly import the package object it works:

import scala.collection.`package`.
val foo: TraversableOnc<TAB>

So the completion logic needs to be fixed to handle package objects correctly.

smarter · 2020-09-24T13:45:31Z

compiler/src/dotty/tools/dotc/interactive/Completion.scala

@@ -264,7 +264,12 @@ object Completion {
    private def accessibleMembers(site: Type)(using Context): Seq[Symbol] = site match {
      case site: NamedType if site.symbol.is(Package) =>
        // Don't look inside package members -- it's too expensive.


It looks like we were intentionally not taking package object members into account, @odersky do you remember the context of this comment? (added in 518e9fd)

I would like to get this PR finally merged but I'm not sure what I should do to push it forward. If the comment is still valid, I can revert some of the changes and leave only the fix for the regression but from user's perspective completion not working for symbols like List or Iterator might be quite confusing and irritating.

We might do it on the Metals side, but it will require a bit more work still.

Just asked Martin IRL about this and he thinks it's fine to make this change. So LGTM from me, but please remove that comment since it's no longer accurate.

smarter

Thank you!

dottybot reviewed Sep 16, 2020

View reviewed changes

tgodzik approved these changes Sep 16, 2020

View reviewed changes

tgodzik requested a review from smarter September 16, 2020 09:39

smarter assigned odersky Sep 18, 2020

smarter requested a review from odersky September 18, 2020 15:50

smarter requested a review from TheElectronWill September 18, 2020 16:14

odersky reviewed Sep 19, 2020

View reviewed changes

odersky requested changes Sep 19, 2020

View reviewed changes

TheElectronWill reviewed Sep 19, 2020

View reviewed changes

smarter requested changes Sep 23, 2020

View reviewed changes

prolativ force-pushed the fix-standard-library-symbols-completion branch 2 times, most recently from 2d7881d to f1cb964 Compare September 24, 2020 12:10

smarter reviewed Sep 24, 2020

View reviewed changes

smarter added this to the 3.0.0-M1 milestone Sep 30, 2020

Fix standard library symbols completion (solves scala#9760)

138c742

prolativ force-pushed the fix-standard-library-symbols-completion branch from f1cb964 to 138c742 Compare October 1, 2020 09:49

smarter approved these changes Oct 1, 2020

View reviewed changes

smarter merged commit 19cf871 into scala:master Oct 1, 2020

prolativ mentioned this pull request Nov 10, 2020

No completions from standard library symbols #9760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix standard library symbols completion (solves #9760) #9805

Fix standard library symbols completion (solves #9760) #9805

prolativ commented Sep 16, 2020 •

edited

Loading

dottybot left a comment

tgodzik left a comment

smarter commented Sep 18, 2020

odersky Sep 19, 2020

TheElectronWill Sep 19, 2020

prolativ Sep 21, 2020 •

edited

Loading

TheElectronWill Sep 21, 2020 •

edited

Loading

odersky left a comment

TheElectronWill left a comment •

edited

Loading

prolativ commented Sep 23, 2020

smarter left a comment

smarter Sep 24, 2020

prolativ Sep 30, 2020

tgodzik Sep 30, 2020

smarter Sep 30, 2020

prolativ Oct 1, 2020

smarter left a comment

Fix standard library symbols completion (solves #9760) #9805

Fix standard library symbols completion (solves #9760) #9805

Conversation

prolativ commented Sep 16, 2020 • edited Loading

dottybot left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

smarter commented Sep 18, 2020

odersky Sep 19, 2020

Choose a reason for hiding this comment

TheElectronWill Sep 19, 2020

Choose a reason for hiding this comment

prolativ Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

TheElectronWill Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

TheElectronWill left a comment • edited Loading

Choose a reason for hiding this comment

prolativ commented Sep 23, 2020

smarter left a comment

Choose a reason for hiding this comment

smarter Sep 24, 2020

Choose a reason for hiding this comment

prolativ Sep 30, 2020

Choose a reason for hiding this comment

tgodzik Sep 30, 2020

Choose a reason for hiding this comment

smarter Sep 30, 2020

Choose a reason for hiding this comment

prolativ Oct 1, 2020

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

prolativ commented Sep 16, 2020 •

edited

Loading

prolativ Sep 21, 2020 •

edited

Loading

TheElectronWill Sep 21, 2020 •

edited

Loading

TheElectronWill left a comment •

edited

Loading