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

Generate FileTarget for all possible targetLocations #3893

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Dec 9, 2023

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
Copy link
Collaborator

@michaelpj michaelpj left a comment

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
Copy link
Collaborator Author

fendor commented Dec 9, 2023

Test failures seem genuine :(

@wz1000
Copy link
Collaborator

wz1000 commented Dec 11, 2023

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

@fendor
Copy link
Collaborator Author

fendor commented Dec 11, 2023

Not yet, but will look into it!

@fendor
Copy link
Collaborator Author

fendor commented Dec 13, 2023

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
Copy link
Collaborator

wz1000 commented Dec 13, 2023

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
Copy link
Collaborator

wz1000 commented Dec 13, 2023

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

@fendor
Copy link
Collaborator Author

fendor commented Dec 13, 2023

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
Copy link
Collaborator

wz1000 commented Dec 13, 2023

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
Copy link
Collaborator Author

fendor commented Dec 13, 2023

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

@wz1000
Copy link
Collaborator

wz1000 commented Dec 14, 2023

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
Copy link
Collaborator Author

fendor commented Dec 14, 2023

@wz1000 Looks good now?

@fendor fendor merged commit 66cf400 into haskell:master Dec 14, 2023
39 checks passed
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