Metals: Off by one error in position-to-offset conversion

[laughedelic]

https://github.com/scalameta/language-server/blob/87452e193986b784d7e0bad1fc2387997fa42e92/metaserver/src/main/scala/scala/meta/languageserver/Positions.scala#L13

Happens on the last newline in the file consistently. You can see that the position 12:0 in the log is correct, so I think it's just an off-by-one error in this conversion.

P.S. It's not related to #116 and all related changes, it existed before.

[olafurpg]

Nice catch! I think we can remove all of the offset conversion implementations in favor of the existing implementation in scala.meta https://github.com/scalameta/scalameta/blob/11a66cb6476d62783bcad27845a2db2a9a5a0128/langmeta/langmeta/shared/src/main/scala/org/langmeta/internal/inputs/InternalInput.scala#L27-L36

I don't know why those methods are package private, they really should be public. I opened https://github.com/scalameta/scalameta/issues/1196 to track this

[xeno-by]

I don't know why those methods are package private

No particular reason other than the desire to limit public APIs. Now that there is a good usecase for those methods, I don't mind making them public.

[olafurpg]

@laughedelic I am unable to reproduce the error after https://github.com/scalameta/language-server/pull/116/commits/bf31517531126fde18d6515006656a16ac8bcf9a Can you confirm?

[laughedelic]

@olafurpg now with exactly same reproduction scenario I get

01:27:55.460 ERROR l.c.LanguageServer$$anon$1 - assertion failed: file:///Users/laughedelic/dev/laughedelic/language-server/test-workspace/src/test/scala/example/UserTest.scala: 259 >= 259
java.lang.AssertionError: assertion failed: file:///Users/laughedelic/dev/laughedelic/language-server/test-workspace/src/test/scala/example/UserTest.scala: 259 >= 259
    at scala.reflect.internal.util.SourceFile.position(SourceFile.scala:26)
    at scala.tools.nsc.CompilationUnits$CompilationUnit.position(CompilationUnits.scala:111)
    at scala.meta.languageserver.providers.HoverProvider$.hover(HoverProvider.scala:22)
    at scala.meta.languageserver.ScalametaLanguageServer.$anonfun$hover$1(ScalametaLanguageServer.scala:256)
    at monix.eval.internal.TaskRunLoop$.monix$eval$internal$TaskRunLoop$$loop$1(TaskRunLoop.scala:187)
    at monix.eval.internal.TaskRunLoop$RestartCallback$1.onSuccess(TaskRunLoop.scala:119)
    at monix.eval.Task$.$anonfun$forkedUnit$2(Task.scala:1463)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

[olafurpg]

That looks very much like an off-by-one 😄

I am unable to reproduce that in vscode, but it seems vscode doesn't send a request when I hover at the end of the file. I tried installing ide-scala but the server doesn't start, will try again another time.

[laughedelic]

it seems vscode doesn't send a request when I hover at the end of the file

It can be editor specific, but I think it's not a generally wrong behaviour, so in any case we should handle any valid positions in the request.

I tried installing ide-scala but the server doesn't start, will try again another time.

If you just installed a version from the store, it's outdated, sorry 😅 I will publish a new one shortly (I was waiting for the right moment to bundle it with a relatively stable (published) version of the server, but it doesn't really matter). If you'll be trying it out and it something doesn't work, I'll be glad to help 😉

[olafurpg]

I was able to reproduce the error in atom @laughedelic

14:34:56.529 ERROR l.c.LanguageServer$$anon$1 - assertion failed: file:///Users/ollie/dev/language-server/test-workspace/src/test/scala/example/UserTest.scala: 260 >= 260
java.lang.AssertionError: assertion failed: file:///Users/ollie/dev/language-server/test-workspace/src/test/scala/example/UserTest.scala: 260 >= 260
    at scala.reflect.internal.util.SourceFile.position(SourceFile.scala:26)
    at scala.tools.nsc.CompilationUnits$CompilationUnit.position(CompilationUnits.scala:111)
    at scala.meta.languageserver.providers.HoverProvider$.hover(HoverProvider.scala:22)
    at scala.meta.languageserver.ScalametaLanguageServer.$anonfun$hover$1(ScalametaLanguageServer.scala:272)
    at monix.eval.internal.TaskRunLoop$.monix$eval$internal$TaskRunLoop$$loop$1(TaskRunLoop.scala:187)
    at monix.eval.internal.TaskRunLoop$RestartCallback$1.onSuccess(TaskRunLoop.scala:119)
    at monix.eval.Task$.$anonfun$forkedUnit$2(Task.scala:1463)

There indeed still seems to be an off-by-one error.

[gabro]

@olafurpg @laughedelic is this still open?

[olafurpg]

I believe it's still open, nothing has changed at least since last time I tried in atom.

[melekhove]

This issue seems to be obsolete. At least the following test passes.

--- a/metals/src/test/scala/tests/search/SymbolIndexTest.scala
+++ b/metals/src/test/scala/tests/search/SymbolIndexTest.scala
@@ -269,6 +269,11 @@ object SymbolIndexTest extends MegaSuite with LazyLogging {
         "_root_.a.User#"
       )
     }
+
+    "last-line" - {
+      assert(index.findSymbol(path.UserTestUri, 12, 0).isEmpty)
+    }
+
   }

[olafurpg]

Thank you @melekhove for reproducing. Additionally, we have added a new scala.meta.Position.Range constructor for line/column that handles oversized column numbers by narrowing them to the maximum column width of that line

input: inputs.Input.String = String("")

@ val pos = Position.Range(input, 0, 0, 0, 12) // end column == 12
pos: inputs.Position.Range = Range(String(""), 0, 0)