fix: Regions based on indent #5260

jkciesluk · 2023-05-23T11:54:34Z

solves #5241
The problem was not connected to packages being in the same file, but to extensions being defined in package at the toplevel. This PR contains multiple fixes:

For val/def/extension/... defined inside package, their owner is module class <filename>$package$, not the package directly, so in inverseSemanticdbSymbol, we have to look inside module classes too.
Previously regions based on indent were working incorrectly if colon was not followed directly by a newline.
We were not always using correct region after escaping from indented region.

Previously, we were not always changing region based on indent correctly. Also, for vals/defs/... defined directly in package, we have to look inside module class.

jkciesluk · 2023-05-24T10:15:38Z

mtags/src/main/scala-3/scala/meta/internal/pc/SemanticdbSymbols.scala

@@ -64,7 +68,7 @@ object SemanticdbSymbols:
                    .alternatives
                    .iterator
                    .map(_.symbol)
-                    .filter(sym => symbolName(sym) == s)
+                    // .filter(sym => symbolName(sym) == s)


Turns out this line was actually needed for overloaded methods 😅
Basically, the problem is: For code like

// Filename: Main.scala package a: def fooBar(x: Int) = x + 1 def foobar(x: String) = x.length

Semanticdb contains names

a/Main$package.fooBar(). a/Main$package.fooBar(+1).

But MtagsIndexer creates names

a/fooBar(). a/fooBar(+1).

So I have to fix it in the indexer

jkciesluk

I added some comments explaining changes

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

jkciesluk · 2023-05-31T07:39:58Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

@@ -128,7 +135,8 @@ class ScalaToplevelMtags(
        if (dialect.allowSignificantIndentation) {
          data.token match {
            case WHITESPACE | COMMENT => region
-            case _ => exitIndented(region, indent)
+            case _ =>


We were not always changing owner correctly when escaping indent

jkciesluk · 2023-05-31T07:40:41Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

@@ -185,7 +192,7 @@ class ScalaToplevelMtags(
          withOwner(currRegion.owner) {
            term(name.name, name.pos, Kind.METHOD, EXTENSION)
          }
-          loop(indent, isAfterNewline = false, region, newExpectIgnoreBody)
+          loop(indent, isAfterNewline = false, currRegion, newExpectIgnoreBody)


We should always use currRegion instead of region, to account for regions based on indent

jkciesluk · 2023-05-31T07:42:07Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

      override def isExtension = extension
+
+      override val withTermOwner: String => InBrace = termOwner =>


Previously, withTermOwner(newOwner) had no effect, which was very confusing

jkciesluk · 2023-05-31T07:44:55Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

      def acceptMembers: Boolean =
        owner.endsWith("/")
-      val produceSourceToplevel: Boolean = false
+      val produceSourceToplevel: Boolean = termOwner.isPackage


After adding <filename>$package, termOwner is no longer a package, so this suffix is not added again, and its being removed when leaving this package. Previously this logic was based on global mutable variable, which wasn't changed after escaping a package

jkciesluk · 2023-05-31T07:45:31Z

tests/mtest/src/main/scala/tests/PCSuite.scala

@@ -87,8 +87,8 @@ trait PCSuite {

  private def addSourceToIndex(filename: String, code2: String): Unit = {
    val file = tmp.resolve(filename)
+    Files.createDirectories(file.toNIO.getParent)


For test CompletionFilenameSuite.paths

kasiaMarek

This is awesome, it seems you caught a few issues there.

I think that it could be also useful to add an analogue of ToplevelSuite for Mtags.toplevels (not in this PR but in general), to see if any of those cases are not handled properly.

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

tgodzik

Great work! 🚀

tgodzik · 2023-06-05T18:00:34Z

tests/cross/src/test/scala/tests/pc/CompletionWorkspaceSuite.scala

+       |package b:
+       |  def main: Unit = incre@@
+       |""".stripMargin,
+    """|increment3: Int


Any dea why this isn't ordered? Not important for this PR though

increment3 is a val, the other two are defs, I think this causes this ordering

tgodzik

Great work! 🚀

fix: Regions based on indent

6c08431

Previously, we were not always changing region based on indent correctly. Also, for vals/defs/... defined directly in package, we have to look inside module class.

jkciesluk commented May 24, 2023

View reviewed changes

fix: Fix incorrect filename in tests

62f08f6

jkciesluk force-pushed the toplevel-defs-completion branch 3 times, most recently from 98fd8f7 to ae15600 Compare May 31, 2023 08:19

jkciesluk commented May 31, 2023

View reviewed changes

jkciesluk force-pushed the toplevel-defs-completion branch from ae15600 to b8d51d9 Compare May 31, 2023 09:41

fix: Adding filename$package for toplevel methods

8d4fb6a

jkciesluk force-pushed the toplevel-defs-completion branch from b8d51d9 to 8d4fb6a Compare May 31, 2023 11:26

jkciesluk requested review from tgodzik and kasiaMarek May 31, 2023 12:38

kasiaMarek approved these changes Jun 1, 2023

View reviewed changes

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala Show resolved Hide resolved

tgodzik approved these changes Jun 5, 2023

View reviewed changes

tgodzik merged commit 2cd41a6 into scalameta:main Jun 5, 2023
21 of 23 checks passed

tgodzik mentioned this pull request Jun 14, 2023

Extension completions don't work when the packages are in the same file #5241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Regions based on indent #5260

fix: Regions based on indent #5260

jkciesluk commented May 23, 2023

jkciesluk May 24, 2023

jkciesluk left a comment

jkciesluk May 31, 2023

jkciesluk May 31, 2023

jkciesluk May 31, 2023

jkciesluk May 31, 2023

jkciesluk May 31, 2023

kasiaMarek left a comment

tgodzik left a comment

tgodzik Jun 5, 2023

jkciesluk Jun 5, 2023

tgodzik left a comment

		override def isExtension = extension

		override val withTermOwner: String => InBrace = termOwner =>

fix: Regions based on indent #5260

fix: Regions based on indent #5260

Conversation

jkciesluk commented May 23, 2023

Choose a reason for hiding this comment

jkciesluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiaMarek left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment