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

Jump to definition doesn't work on Windows #688

Open
jiribenes opened this issue Nov 12, 2024 · 4 comments
Open

Jump to definition doesn't work on Windows #688

jiribenes opened this issue Nov 12, 2024 · 4 comments
Labels
area:lsp bug Something isn't working windows

Comments

@jiribenes
Copy link
Contributor

On Windows, jumping to definition in VSCode doesn't work correctly.

When it should jump to file:

C:/.../effekt/lib/foo.effekt

it instead tries to jump to file with a trailing slash:

C:/.../effekt/lib/foo.effekt/

Unfortunately, this tends to confuse VSCode so that it thinks it's an invalid folder.

I'm not sure it's a VSCode bug per se, that's why I'm filing the issue here (since I think it might be an LSP issue).
As a workaround, we could just always remove trailing slashes, I think? 🤔

@jiribenes jiribenes added bug Something isn't working area:lsp labels Nov 12, 2024
@b-studios
Copy link
Collaborator

b-studios commented Nov 20, 2024

Here is the kiama code that computes the source file location:

  def locationOfNode(node: N): Location = {
    (positions.getStart(node), positions.getFinish(node)) match {
      case (start @ Some(st), finish @ Some(_)) =>
        st.source match {
          case StringSource(_, name) =>
            val s = convertPosition(start)
            val f = convertPosition(finish)
            new Location(name, new LSPRange(s, f))
          case _ =>
            null
        }
      case _ =>
        null
    }
  }

https://github.com/effekt-lang/kiama/blob/ab6aef78fc4ce99a9ad2db8555718d00b03f7b8a/jvm/src/main/scala/kiama/util/Server.scala#L337C7-L351

It just returns the path and shouldn't mess with it.

The constructor of Location also just stores the uri.

The URI should be introduced here:

@JsonNotification("textDocument/didSave")
  def didSave(params: DidSaveTextDocumentParams): Unit = {
    process(params.getTextDocument.getUri, params.getText)
  }

https://github.com/effekt-lang/kiama/blob/ab6aef78fc4ce99a9ad2db8555718d00b03f7b8a/jvm/src/main/scala/kiama/util/Server.scala#L496-L499

which ends up invoking

def compileString(name: String, input: String, config: C): Unit = {
    compileSource(StringSource(input, name), config)
}

https://github.com/effekt-lang/kiama/blob/ab6aef78fc4ce99a9ad2db8555718d00b03f7b8a/jvm/src/main/scala/kiama/util/Compiler.scala#L112-L114

which in turn ends up calling

override def compileSource(source: Source, config: EffektConfig): Unit = try {

on the string source.

@b-studios
Copy link
Collaborator

b-studios commented Nov 20, 2024

Here is a gem, that I didn't know about:

def toURI(filename: String): String = {
if (filename startsWith "file://")
filename
else
if (filename startsWith "./")
"file://" ++ Filenames.cwd() ++ "/" ++ Filenames.dropPrefix(filename, ".")
else
s"file://$filename"
}

Not sure this is related to the bug, though. However, this should be a source of bugs on windows since it doesn't use path separator, but literal /, which might not work on Win.

@b-studios
Copy link
Collaborator

@jiribenes do you know whether this also happens when jumping to definition within the same file?

@TheDying0fLight
Copy link

@jiribenes do you know whether this also happens when jumping to definition within the same file?

Jumping to definitions within the same file works without a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:lsp bug Something isn't working windows
Projects
None yet
Development

No branches or pull requests

3 participants