Skip to content

Generate FileTarget for all possible targetLocations#3893

Merged
fendor merged 2 commits into
haskell:masterfrom
fendor:fix/boot-modules
Dec 14, 2023
Merged

Generate FileTarget for all possible targetLocations#3893
fendor merged 2 commits into
haskell:masterfrom
fendor:fix/boot-modules

Conversation

@fendor

@fendor fendor commented Dec 9, 2023

Copy link
Copy Markdown
Collaborator

If a target file has multiple possible locations, then we
assume they are all separate file targets.
This happens with '.hs-boot' files if they are in the root directory of the project.
GHC reports options such as '-i. A' as 'TargetFile A.hs' instead of 'TargetModule A'.
In 'fromTargetId', we dutifully look for '.hs-boot' files and add them to the
targetLocations of the TargetDetails. Then we add everything to the 'knownTargetsVar'.
However, when we look for a 'Foo.hs-boot' file in 'FindImports.hs', we look for either

  • TargetFile Foo.hs-boot
  • TargetModule Foo

If we don't generate a TargetFile for each potential location, we will only have
'TargetFile Foo.hs' in the 'knownTargetsVar', thus not find 'TargetFile Foo.hs-boot'
and also not find 'TargetModule Foo'.

It took a while, but this fixes #3794

@fendor
fendor requested a review from pepeiborra as a code owner December 9, 2023 10:14
@fendor
fendor requested review from michaelpj and wz1000 and removed request for pepeiborra December 9, 2023 10:14

@michaelpj michaelpj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I entirely understand, but seems plausible!

@fendor

fendor commented Dec 9, 2023

Copy link
Copy Markdown
Collaborator Author

Test failures seem genuine :(

@wz1000

wz1000 commented Dec 11, 2023

Copy link
Copy Markdown
Collaborator

Patch seems fine to me. Did you check locally to see why tests are failing?

@fendor

fendor commented Dec 11, 2023

Copy link
Copy Markdown
Collaborator Author

Not yet, but will look into it!

@fendor

fendor commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator Author

The existence check seems to be removing valid file targets. I.e., there are many file targets that don't exist on disk. Probably related to the VFS? I am confused why it works with Module Targets in that case, since we always filter those.

@wz1000

wz1000 commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

The existence check seems to be removing valid file targets. I.e., there are many file targets that don't exist on disk. Probably related to the VFS? I am confused why it works with Module Targets in that case, since we always filter those.

Many files from the testsuite only exist in the VFS, and those files are always FileTargets. I guess we should also check for existence in the VFS.

@wz1000

wz1000 commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

We could also ensure that explicit file targets aren't filtered out.

@fendor

fendor commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator Author

We could also ensure that explicit file targets aren't filtered out.

What are explicit file targets?
For now, I've just decided that all file target locations are valid FileTargets.

@wz1000

wz1000 commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

For now, I've just decided that all file target locations are valid FileTargets.

This is incorrect. We might end up trying to typecheck non-existent hs-boot files, or files in the wrong include directory

What are explicit file targets?

The FilePath argument to TargetFile :: FilePath -> Target is the explicit target. The [FilePath] in the targetLocations still needs to be filtered. We just need to ensure that the explicit target is included in the final result:

case targetTarget of
  TargetFile f -> do
    found <- filterM (IO.doesFileExist . fromNormalizedFilePath) targetLocations
    return [(targetTarget, nubOrd (f:found))]

@fendor

fendor commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator Author

Ah, I see!
Ok, the code presented is still not correct with respect to files that only exist in the VFS, right?

@wz1000

wz1000 commented Dec 14, 2023

Copy link
Copy Markdown
Collaborator

No, but the problematic situation will only be when a file is listed as a Target but not saved to disk, i.e. the user has edited the cabal file to include the module, but has not saved the module yet.

If a target file has multiple possible locations, then we
assume they are all separate file targets.
This happens with '.hs-boot' files if they are in the root directory of the project.
GHC reports options such as '-i. A' as 'TargetFile A.hs' instead of 'TargetModule A'.
In 'fromTargetId', we dutifully look for '.hs-boot' files and add them to the
targetLocations of the TargetDetails. Then we add everything to the 'knownTargetsVar'.
However, when we look for a 'Foo.hs-boot' file in 'FindImports.hs', we look for either

 * TargetFile Foo.hs-boot
 * TargetModule Foo

If we don't generate a TargetFile for each potential location, we will only have
'TargetFile Foo.hs' in the 'knownTargetsVar', thus not find 'TargetFile Foo.hs-boot'
and also not find 'TargetModule Foo'.
@fendor

fendor commented Dec 14, 2023

Copy link
Copy Markdown
Collaborator Author

@wz1000 Looks good now?

Comment thread ghcide/session-loader/Development/IDE/Session.hs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HLS fails to find modules with .hs-boot files in the root project directory

3 participants