nwolverson/vscode-ide-purescript

Go to definition feature got broken in `v0.26.5`

Closed this issue · 32 comments

KristianBalaj commented

The feature works only partially, it works for some imported types/functions, etc. but for some others it doesn't.

I don't see any specific pattern in the definitions for which the go to works or doesn't.

Also, no erroneous output log for the Purescript IDE extension can be seen.

Reverting back to the v0.26.3 makes the Go to definition feature work properly again.

wclr commented

There should be some pattern anyway. Maybe only local defs/not exported or something?

KristianBalaj commented

Haven't noticed anything about local defs, also haven't noticed any relation with not exported ones, the same behaviour can be noticed even when using explicit exports.

A few things I've noticed:
Also, the go to is sometimes broken for definitions in current project (different files of the project) but also for the dependencies definitions. Sometimes from a single module A, importing 3 different functions f1, f2, f3, for 2 of them the go to works and for the third one the go to doesn't work (module using explicit exports)...

wclr commented

It should be somehow reproducible. Are you using fastRebuild or diagnosticsOnType, fullBuildOnSave?

KristianBalaj commented

Using only fastRebuild from those 3 options.

Also, I've tried to do project clean and build it from scratch but the behaviour was the same.

natefaubion commented

FWIW, I'm seeing the same issue.

natefaubion commented

It seems to be very inconsistent. I honestly can't pin down exactly why it will find some things but not others. With verbose logging I can see the request for textDocument/definition and for some it returns nothing and some it returns the full document text. All sorts of references appear to work inconsistently within the same module: qualified names, unqualified names, constructors, local definitions. I don't think it's a purs-ide issue because everything that can return type information does and it all compiles fine with fast rebuild. I'll see definitions with type on hover, but can't go to them.

I'd say the only thing I can find that is consistent is that it has trouble finding the same definitions between window reloads, so it at least appears to be deterministically inconsistent.

wclr commented

@nwolverson
There seems to be only one commit that may relate to this problem. nwolverson/purescript-language-server@eb12800

It happened that I'm still on the previous version of LS (0.17.3, though with the latest changes of mine) and hadn't synced with upstream for a while (now I guess because master was renamed to main 😳 and I rebased to master), so I do not experience the issues described.

nwolverson commented

I'm seeing the issue personally, it is rather irritating but I've not had a moment. Thanks for digging up that commit, I'm sure there was a reason this needed reworked, I think possibly moving away from legacy api support that wasn't available in another editor. I'll look into it as soon as I get a chance

nwolverson commented

The linked commit is not to do with definition, so I think hopefully not relevant.

In fact I can reproduce the issue with 0.17.3 (as used by ide-purescript 0.26.3)

Initially looks to be a bad response from purs ide, we changed to using the type command with a dependencies filter since 0.17.2 (little over a year ago), which is coming back with "definedAt": null.

Editing and saving the module containing the definition, and purs ide now returns a definedAt field.

jquesada2016 commented

I am running into this as well. I am on spago 0.93.41

wclr commented

The linked commit is not to do with definition, so I think hopefully not relevant.
In fact I can reproduce the issue with 0.17.3 (as used by ide-purescript 0.26.3)

Why do people say it happens after the latest an updates and reverting to v0.26.3 fixes it? Btw even if to use v0.26.3, it has dependency "purescript-language-server": "^0.17.3 so it would anyway use the latest LS version available?

nwolverson commented

@wclr given the issue appears inconsistent I would not trust a report of not seeing the bug on reverting unless it is accompanied by a precise set of reproduction steps. I certainly saw the issue with 0.17.3, so clearly someone is mistaken to an extent. It would be nice if it is me, because otherwise I think I'm looking at purs changes

I didn't have long to look at this but I was unable to reproduce in a standalone test project, only an actual work project that makes it hard to actually get anywhere

To verify the actual purescript-language-server running, the Output panel for IDE PureScript will log this right near the top.

wclr commented

So far I belive I never experience the issue with jumping to definitions. I know there are some issues with how purs-ide works, esp. how it handles rebuilds, etc.

I'm currently using my updated version of the compiler including some purs-ide changes. Specifically I changed the behaviour of the initial loading of modules: instead of just loading built results from the output, it runs a full build (make) for the supplied modules (and loads the failed from the disk). It seems like it helped to get rid of some issues that happened sometimes when ide started (missing and stale modules, etc). I'm not sure if it has an impact on the issue discussed.

Editing and saving the module containing the definition, and purs ide now returns a definedAt field.

Maybe this is handled by the initial full make that is performed in my purs-ide version?

nwolverson commented

Didn't get around to this for a long time after failing to reproduce it in a test, while it has been slowing me down on a daily basis.

It turns out the problem is the change to spawn/exec with shell for the windows node-security issue #223, which means the globs that should be passed as literals to purs ide server are suddenly being interpreted by the shell. And thus things are rather inconsistent depending on whether your shell has globstar enabled, perhaps path length issues, etc

Should be fixed in v0.26.6 (lsp 0.18.3). I've simply reverted to not using the shell option other than on windows, I think this should still play out ok on windows as the globs will be uninterpreted there, let me know how it goes.

natefaubion commented

This is still broken for me on the latest for anything except src. I can get types only from purs-ide, but no docs, and no go to definition. I'm using spago@next, so I wonder if the format of spago sources changed? The whole glob thing sounds like a potential culprit still.

natefaubion commented

It doesn't look like it's logging the params for purs ide even with "verbose" logging, so it's hard for me to tell what exactly is going on in that regards. I'm happy to try any suggestions to further narrow it down.

nwolverson commented

You're not on windows @natefaubion ? I use spago@next and this fixed things fully - to check the purs ide args I just look in ps for the command line, the difference is pretty easy to spot

wclr commented

I'm on Windows. I just tried to launch a project with new spago (which is linked as npm package from the local dev repo), and was not getting anything except src/** sources.

It turned out that executing "spago sources" returned empty stdout and the the error message (in stderr) . It was trying to launch the found bin: C:\Program Files\nodejs\spago.CMD and failed with the error: 'C:\Program' is not recognized as an internal or external command, operable program or batch file.

I wrapped the bin with quotes and now it works.

natefaubion commented

I'm not on windows (I'm on osx), but I am able to reproduce it working and not working reliably.

  • I have purs@0.15.15 (and spago@next) installed locally for a project via npm, and I have npm bins enabled in settings for the language setting. I have no global purs installation in my path.
  • I can run a full build, I get types on hover, I get immediate feedback on save. I just don't get docs or go to definition.
  • Quit vscode, amend ~/.profile to add a global purs@0.15.7 install to my path, source. Relaunch with code .
  • Now everything works (docs, go to). Log indicates it found two purs bins in the path, picking the first. Can confirm it is still launching local node_modules purs@.15.15 bin.
  • Quit everything again, remove global install from path, source, relaunch, and I'm back to the previous behavior of docs and go to not working.

Note I'm not running any additional builds in between. I can confirm the build products are all 0.15.15. I'm not sure why having a global install in the path that it's not using would change why it's working or not, but at least I'm back in business for now.

natefaubion commented

If I use ps to look at the process call, then when it works I see

./node_modules/.bin/purs ide server -p 15663 --output-directory output

But when it doesn't work, I see:

./node_modules/.bin/purs ide server -p 15979 --output-directory output/ src/**/*.purs

So it seems the calling convention is different between the cases. And I suppose the globs are just unnecessary?

Hmm, going through the process again, and it worked but now I see all the globs. So I dunno what the deal is there. It may just be how it decided to truncate it in my shell.

natefaubion commented

OK, piping ps through cat avoids truncation. Can confirm that it works when all globs are passed in. When it doesn't work it's definitely only passing in src globs. Not sure why it would do that based on whether there are multiple purs's in the path, but that seems to be the most reliable way to reproduce.

wclr commented

When it doesn't work it's definitely only passing in src globs.

This probably means that it can't get needed result from "spago sources", "src/**/*.purs" LS adds by default.

What is the path to spago executable? Maybe there is a similar issue that I had on windows.

natefaubion commented

I don’t have a global spago, so it’s going to be the node_modules bin.

natefaubion commented

With spago@next, if you call spago sources without a purs in your path (even though it doesn't call it for the command), it will return an error and no source paths. So my working assumption is that specifically for spago sources, the language server is not calling it with npm bins in the path, though it adds it for other commands, such as a full build.

wclr commented

So, does it it work for you with purs installed globally?

natefaubion commented

It works for me as long as there is a purs in my global path somewhere due to a spago@next check. I don't think it matters what this purs is. But I would expect the language server to add npm purs to the path even in the spago sources call.

wclr commented

Right, this should be fixed to make new spago work in this scenario.

natefaubion commented

I think it's reasonable to assume that if addNpmPaths is enabled, npm paths are enabled for all process calls. It's not obvious to me this is a spago issue. Spago's check is a reasonable sanity check.

  • 👍1
nwolverson commented

No problem adding the npm paths here and everywhere, but spago's check is also suprising, I wouldn't have even considered the possibility purs was required to print out a list of globs. I would probably suggest that a warning on stderr while still printing the deps would be preferable.

The real footgun here was the fact that the error is not logged at all from the language server. Well spotted

natefaubion commented

My guess is that spago does this as a coarse-grained preflight check for all commands. Whether or not that's appropriate is debatable, certainly.

wclr commented

@nwolverson if you will be fixing this, also check if it is ok to wrap bin here in quotes, it seems to be required on Windows, as I described the issue above.