Prepend hs-source-dir to match-component #6622

[fendor]

Why this change?
The function matchFile expects arguments of a form such as:

matchFile [("app/Main.hs", ()), ("test/Test.hs", ())]  "app/Main.hs"

but the old implementation produces the following:

[("Main.hs", ()), ("Test.hs", ())]

since it doesnt prepend in the hsSourceDirs for each other-module.

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• If the change is docs-only, [ci skip] is used to avoid triggering the build bots.
• [ ] Add tests for ambiguous filepath targets

Please also shortly describe how you tested your change. Bonus points for added tests!

closes #6622

[mpickering]

Is this feature of the repl documented in the manual? Now might be a good time to do so.

[fendor]

I am happy to update any documentation for cabal repl. Where should the documentation be added?

[phadej]

re docs, there are sections for v2 commands. For example https://cabal.readthedocs.io/en/latest/nix-local-build.html#cabal-v2-repl

There are also TargetSelector tests in cabal-install/tests/IntegrationTests2.hs. Please add one testing new scenario.

[fendor]

This should be updated, since cabal build File.hs works.

[fendor]

The windows error seems to be that the following works on windows:

$ cabal repl p:app\Main.hs

but this doesnt:

$ cabal repl p:app/Main.hs

However, both

$ cabal repl app/Main.hs

$ cabal repl app\Main.hs

work.

Is it ok to fix that in this PR, too?

EDIT: fixed it.

[fendor]

So, currently there is an internal error when the file target is ambiguous, e.g. target is part of two executables, therefore hard to actually test for. The bug-fix for that is relatively simple, however, I feel like this PR is already bigger than planned.
Should I push the bug-fix here, or rather create another issue and another PR, so at least the very helpful changes in this PR are merged?

[phadej]

I'm on the fence with this. Something tells me that cabal repl Foo.hs should not pick the component Foo.hs is included but build all of the project (or none if outsde) and load Foo.hs.

#6149

[fendor]

Shouldn't v2-build just build both?

Yeah, but it fails. I assume it is just some minor bug, somewhere in target matching.

I'm on the fence with this. Something tells me that cabal repl Foo.hs should not pick the component Foo.hs is included but build all of the project (or none if outsde) and load Foo.hs.

What is all of the project in the case of a library? Wouldnt "library" be the component to build?
Or do you mean additionally, we should load the module Foo into the ghci session?
Afaict, we could add :add Foo to the ghci-script passed to ghci to achieve this.

However, please note, that not behaviour is introduced by this patch. This patch only fixes two bugs regarding finding the component associated with the module.

[phadej]

I imagine use case:

cd some-library            # go to library sources
vim ExampleBug.hs          # quickly hack some example file
cabal repl ExampleBug.hs   # load it into repl

This is very common workflow for myself, but I have to rely on local
environment files now.

Yet, I don't understand what is the motivation to be able to select
a component by an arbitrary source-file. Module name is somewhat in unclear,
also main-is: EntryPoint.hs. But arbitrary source file: not really.

OTOH out-of-package source file and in package source files should
be disjoint, so there is no ambiguity.

[mpickering]

@phadej The motivation to select a component by a source file is to implement an IDE which can create the right session given any FilePath.

This patch just fixes the existing behavior as far as I understand.

[fendor]

This is very common workflow for myself, but I have to rely on local
environment files now.

Would this be accomplished by #6149? Or rather, could we accomplish it that way?

[phadej]

@mpickering I see. That's enough for me for a motivation.

[fendor]

So, is there anything blocking this for getting merged? I would be happy if this bug-fix can make it into the next release.

[fendor]

Is it ok if I merge this? Ideally before #6774 is merged so this PR does not need to be rebased.

[m-renaud]

Assuming no major delays here I can hold off until this PR is merged.

[fendor]

I am just waiting for approval (or requesting changes). Dont want to go over anyone's head.

[phadej]

what happens if executable name is HLint (note case), and there is HLint module in a library.

Is that ambiguous, how to resolve ambiguity?

[phadej]

I see

                                  ":pkg:q:lib:q:file:Q.y"
                                  ":pkg:p:exe:ppexe:file:app/Main.hs"

selectors in text but have no idea what they mean (by looking at the documentation changes).

[phadej]

Should we also add a test that Main.hs should be ambiguous?

[fendor]

Yeah, an ambiguous target selector makes sense, going to update it asap!

[fendor]

I am sorry, I am not sure I get what you are saying?
Do you mean to add a test for the case that Main.hs is ambiguous? Since it should be a filepath, I dont think it can be ambiguous?

[fendor]

what happens if executable name is HLint (note case), and there is HLint module in a library.

I dont know a lot about the module syntax, but the current (3.2.0.0) behaviour is that the executable is chosen. If this is not clear enough, we can remove the module syntax from the documentation.

selectors in text but have no idea what they mean (by looking at the documentation changes).

That is mainly copied from the other test-cases. I read the latter as the most verbose version of a file-target syntax while the former is the same for .y files (which are generated from happy, I think?)

[fendor]

what happens if executable name is HLint (note case), and there is HLint module in a library.

There exists a test for it already:

    -- local components shadow modules and files
    reportSubCase "unambiguous: cwd comp vs module, file"
    assertUnambiguous "Foo"
      (mkTargetComponent "bar" (CExeName "Foo"))
      [ mkpkg "bar" [mkexe "Foo"]
      , mkpkg "other" [ mkexe "other"  `withModules` ["Foo"]
                      , mkexe "other2" `withCFiles`  ["Foo"] ]
      ]

[fendor]

Merged via #6826