Fix other file goto definition

[nlander]

In my work on gotoDefinition for dependencies I found a bug that predates my code. In the first few seconds of startup for big projects, gotoDefinition uses the persistent rule for GetHieAst and gets its PositionMapping from that. This PositionMapping gets used on any location a definition is found, even if that definition is in a different file. This doesn't usually cause a problem, except when the definition in the other file is found on a line that is greater than the number of lines in the file we are coming from. In this case, the PositionMapping will end up nullifying the result returned by AtPoint.gotoDefinition.

If you want to see this bug in action, try opening ghcide/src/Development/IDE/LSP/Outline.hs and try to immediately do gotoDefinition on extract_cons on line 115.

This code adds some tests for gotoDefinition and fixes the problem by checking if the file where the definition is found is the same as the file we are calling gotoDefinition from. If not, we get the PositionMapping for the file we are going to and use that instead.

[fendor]

Nice catch, LGTM!

[michaelpj]

Great investigation!

[michaelpj]

I wonder if we have this problem anywhere else? I could imagine that e.g. DocumentSymbols and so on might produce locations in other files, which could have the same issues.

[michaelpj]

Wait, it would have to be workspace symbols, or maybe call hierarchy.

[michaelpj]

e.g.

haskell-language-server/ghcide/src/Development/IDE/Core/Actions.hs

Line 121 in f20562f

res <- liftIO $ withHieDb (\hieDb -> HieDb.searchDef hieDb $ T.unpack query)

@wz1000 I'm guessing that these hiedb operations will return the positions in the file from some past point in time? Or will it do the right thing if the target file is open in the editor and has been altered by the user?

[wz1000]

e.g.

haskell-language-server/ghcide/src/Development/IDE/Core/Actions.hs

Line 121 in f20562f

res <- liftIO $ withHieDb (\hieDb -> HieDb.searchDef hieDb $ T.unpack query)

@wz1000 I'm guessing that these hiedb operations will return the positions in the file from some past point in time? Or will it do the right thing if the target file is open in the editor and has been altered by the user?

we index files asynchronously whenever they are changed, so if that is completed it will do the right thing (as long as the file compiles).

[nlander]

Thanks all! Anything else that needs to happen before it gets the mergeme label?

[michaelpj]

so if that is completed it will do the right thing

And if it isn't it won't! It seems like in principle we probably ought to be computing position mappings for things from hiedb also, tbh.

[wz1000]

And if it isn't it won't! It seems like in principle we probably ought to be computing position mappings for things from hiedb also, tbh.

yes, but then we would need to know which version of the file was indexed in hiedb.

Even if we kept track of file versions (source hashes?) in hiedb, this would still not be complete and potentially add a lot of latency to these requests. If you open and edit a file, but the initial load hasn't completed, we will still have a location from the hiedb indexed during the previous HLS session. But we have no idea how to map positions between the current file and version of the file indexed in the database. We could do some combination of the following things:

1. Check if we have a mapping in memory corresponding to the source hash in hiedb, and if so use that.
2. If not, we could read the source from the .hie file and use that to construct a mapping, but the .hie file on disk may not be the correct version (because the last HLS session was killed before indexing could complete, or because we overwrote the .hie file in this session because we are also compiling/indexing everything asynchronously).

2 seems not quite worth it and it would also be susceptible to race conditions.