Avoid unnecessary Target canonicalisation in Session setup #2359
Conversation
In particular, this test checks whether modules that are actually symbolic lins can be found by ghcide. This is known to be broken, as Session.hs canonicalises Targets, e.g. saves the location of the symbolic link. When we later try to load that module, we can't find it, as it won't be part of the known targets since it is not canonicalized.
Canonicalising Targets makes it harder later to actually find the targets during import analysis, as ghcide only looks for modules in the import paths and checks for existence in the known target Map. However, import analysis doesn't canonicalise target candidates, thus the lookup in the known target Map will always fail. We no longer canonicalise Targets, so import analysis will succeed loading modules that are actually symbolic links.
| return [TargetDetails (TargetModule mod) env dep locs] | ||
| -- For a 'TargetFile' we consider all the possible module names | ||
| fromTargetId _ _ (GHC.TargetFile f _) env deps = do | ||
| nf <- toNormalizedFilePath' <$> canonicalizePath f | ||
| let nf = toNormalizedFilePath' f |
There was a problem hiding this comment.
| let nf = toNormalizedFilePath' f | |
| nf <- toNormalizedFilePath' <$> makeAbsolute f |
This might be better to make sure the paths are absolute but let's see what CI says
|
Did you check the git history to make sure we are not missing anything? This seems like a risky change |
|
The canonicalisation seems to be from the first implementation done by mpickering. One of the earliest commits I can find in this repository is f1b36ba#diff-d7140bbfca1b7aaafd8c82d1d6aa21a3f941eb27ba8f08e4d9fb13766b21b418R316 I think this was mainly done to avoid possible issues with relative and absolute paths. In Alternatively, if you feel uncomfortable with this solution, we can canonicalize the candidates of the imports at https://github.com/haskell/haskell-language-server/blob/master/ghcide/src/Development/IDE/Import/FindImports.hs#L71 |
|
After looking some more around, I found: which we need to patch as well for the same reasons.Any other occurrence of I think we should use |
|
I am weirded out that this commit fixed anything |
|
@jneira are windows tests flaky? |
always 😆 a little bit more flaky than ubuntu/macos ones |
Lately ghcide tests are more flaky in ubuntu 🤔 |
|
This is weird. CI is apparently slightly flaky. I am not fully on-board with changes from b790142 as I did not check thoroughly what they influence. Should we just ignore it, or should I revert this commit since I didnt check it? |
makeAbsolute does a normalisation? or it keeps . and .. f.e.? (one of the call sites talk about remove ..) |
fendor
force-pushed
the
target-canonicalization
branch
from
b790142
to
d42d632
Compare
There was a problem hiding this comment.
Looks good, it includes the appropiate regression test and ci is green. Great work as usual.
I would wait to next release to dogfooding it if you dont mind
Would like to know @pepeiborra thoughts though
|
we will need a new release with the fixes for the early bugs found in 1.5.0, will merge afterwards |
jneira
added
merge me
the ghcide build fails for win and ghc-8.8 with segfaults
) * Add test-case for projects that use symbolic links In particular, this test checks whether modules that are actually symbolic lins can be found by ghcide. This is known to be broken, as Session.hs canonicalises Targets, e.g. saves the location of the symbolic link. When we later try to load that module, we can't find it, as it won't be part of the known targets since it is not canonicalized. * Dont canonicalise Targets during session setup Canonicalising Targets makes it harder later to actually find the targets during import analysis, as ghcide only looks for modules in the import paths and checks for existence in the known target Map. However, import analysis doesn't canonicalise target candidates, thus the lookup in the known target Map will always fail. We no longer canonicalise Targets, so import analysis will succeed loading modules that are actually symbolic links. * Prefer makeAbsolute over canonicalizePath * Use makeAbsolute to read HIE files from disk * Restore repeated builds the ghcide build fails for win and ghc-8.8 with segfaults Co-authored-by: Javier Neira <atreyu.bbb@gmail.com> Co-authored-by: Pepe Iborra <pepeiborra@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Closes #2358