Use cabal-install-parsers and Cabal parser

maksbotan commented 4 years ago

This is a draft PR to provoke discussion.

I came across http://hackage.haskell.org/package/cabal-install-parsers-0.3.0.1/docs/Cabal-Project.html, which was made for haskell-ci, and I thought that it could be a good idea to use this parser here instead of hand-written one.

What do you think?

Avi-D-coder commented 4 years ago

Thanks, I'm very much in favor of this. It was discussed in https://github.com/Avi-D-coder/implicit-hie/issues/4

I take a look tomorrow.

Avi-D-coder commented 4 years ago

@jneira, @fendor, if you could both take a look as well

Avi-D-coder commented 4 years ago

So this is the output of this pr on hls repo.

cradle:
  cabal:
    - path: "./src"
      component: "lib:haskell-language-server"

    - path: "./exe/Main.hs"
      component: "haskell-language-server:exe:haskell-language-server"

    - path: "./exe/Wrapper.hs"
      component: "haskell-language-server:exe:haskell-language-server-wrapper"

    - path: "./test/utils"
      component: "haskell-language-server:test:func-test"

    - path: "./test/functional"
      component: "haskell-language-server:test:func-test"

    - path: "ghcide/src"
      component: "lib:ghcide"

    - path: "ghcide/test/preprocessor/Main.hs"
      component: "ghcide:exe:ghcide-test-preprocessor"

    - path: "ghcide/exe/Main.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/test/cabal"
      component: "ghcide:test:ghcide-tests"

    - path: "ghcide/test/exe"
      component: "ghcide:test:ghcide-tests"

    - path: "ghcide/test/src"
      component: "ghcide:test:ghcide-tests"

master outputs this:

cradle:
  cabal:
    - path: "./src"
      component: "lib:haskell-language-server"

    - path: "./exe/Main.hs"
      component: "haskell-language-server:exe:haskell-language-server"

    - path: "./exe/Arguments.hs"
      component: "haskell-language-server:exe:haskell-language-server"

    - path: "./exe/Wrapper.hs"
      component: "haskell-language-server:exe:haskell-language-server-wrapper"

    - path: "./exe/Arguments.hs"
      component: "haskell-language-server:exe:haskell-language-server-wrapper"

    - path: "./test/functional"
      component: "haskell-language-server:test:func-test"

    - path: "ghcide/src"
      component: "lib:ghcide"

    - path: "ghcide/src-ghc86"
      component: "lib:ghcide"

    - path: "ghcide/src-ghc88"
      component: "lib:ghcide"

    - path: "ghcide/src-ghc810"
      component: "lib:ghcide"

    - path: "ghcide/test/preprocessor/Main.hs"
      component: "ghcide:exe:ghcide-test-preprocessor"

    - path: "ghcide/exe/Main.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/exe/Utils.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/exe/Arguments.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/exe/Paths_ghcide.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/test/cabal"
      component: "ghcide:test:ghcide-tests"

    - path: "ghcide/test/exe"
      component: "ghcide:test:ghcide-tests"

    - path: "ghcide/test/src"
      component: "ghcide:test:ghcide-tests"

maksbotan commented 4 years ago

Yes, I did not yet implement other-modules for exes, tests and benchmarks, there's TODO item for that in the code.

By the way, what do you do in the case there are multiple hs-source-dirs in the executable? Generate cartesian product of source dirs and modules?

Avi-D-coder commented 4 years ago

Maybe I have not experimented with that case.

jneira commented 4 years ago

Maybe a case not taken in account:

PS D:\dev\ws\haskell\cabal-test> gen-hie
gen-hie.exe: BadPackageLocation "."
PS D:\dev\ws\haskell\cabal-test> cat .\cabal.project
packages:  .

jneira commented 4 years ago

It seems it does not take in account conditionals (imo it is one of the keys reasons to use Cabal):

For a .cabal file with a lib stanza like (a very common pattern):

library
  exposed-modules:     Lib
  build-depends:       base >=4.12 && <4.15
                     , bytestring
  hs-source-dirs:      src
  default-language:    Haskell2010
  if os(windows)
    hs-source-dirs:    win-src
  else
    hs-source-dirs:    nix-src

the hie.yaml generated is):

cradle:
  cabal:
    - path: "./src"
      component: "lib:cabal-test"

    - path: "./app/Main.hs"
      component: "cabal-test:exe:cabal-test"

    - path: "./test"
      component: "cabal-test:test:cabal-test-test"

Not sure how could we handle conditionals:

include all paths and let the user remove the ones that breaks the ide
- with a comment warning them
include conditional paths but commented, to make easier select the good ones
???

maksbotan commented 4 years ago

Thanks for the comments, @jneira! Unfortunately, I did not have the time to look into it more on the weekend.

Re conditionals. As far as I understand, for every open file hie/hls would try to find matching prefix. Than I suppose that it's the right idea to generate all possible prefixes. In the normal scenario, user won't open files that belong to other conditionals, and therefore those prefixes will never be matched.

I believe that resolving of the conditionals does not belong to this project.

But of course, I may be wrong, I'm just a bystander :)

maksbotan commented 4 years ago

How about we come up with a definition of reliable first, then? :) I don't think I'm familiar with hie internals enough at the moment to know that...

jneira commented 4 years ago

Well, reliable can be pretty ambiguous term, fortunately we already have two alternatives that can be used a bounds of what can be a reliable way to get a hie-bios configuration:

the actual custom parser as lower bound
using cabal-helper a higher bound, see https://github.com/Avi-D-coder/implicit-hie/pull/11

We have take in account that one of the goals of the library is to being used as the implicit configuration with no hie.yaml (see https://github.com/haskell/haskell-language-server/pull/138). There would be no hie.yaml file to correct so imo it would need a configuration that takes in account common stanzas and conditionals.

More context (apart of the pr linked by @Avi-D-coder above):

https://github.com/mpickering/hie-bios/pull/178

What would be the improvements in the generated config using this pr instead the actual custom parser? does it handle common stanzas?

jneira commented 4 years ago

does it handle common stanzas?

It does, although using them within executable components generates a little bit strange file.

For this cabal file

common mycommon
  hs-source-dirs:      common-src

library
  import:               mycommon
  exposed-modules:     Lib
  build-depends:       base >=4.12 && <4.15
                     , bytestring
  hs-source-dirs:      src
  default-language:    Haskell2010
  if os(windows)
    hs-source-dirs:    win-src
  else
    hs-source-dirs:    nix-src

executable cabal-test
  import:              mycommon
  main-is:             Main.hs
  build-depends:       base >=4.12 && <4.15
                     , cabal-test
                     , bytestring
  hs-source-dirs:      app
  default-language:    Haskell2010

that is the output of this version:

cradle:
  cabal:
    - path: "./common-src"
      component: "lib:cabal-test"

    - path: "./src"
      component: "lib:cabal-test"

    - path: "./common-src/Main.hs"
      component: "cabal-test:exe:cabal-test"

    - path: "./app/Main.hs"
      component: "cabal-test:exe:cabal-test"

    - path: "./test"
      component: "cabal-test:test:cabal-test-test"

Observe the .common-src/Main.hs, that likely will not exist

Otoh, the actual lib version includes some condition handling!

cradle:
  cabal:
    - path: "./src"
      component: "lib:cabal-test"

    - path: "./win-src"
      component: "lib:cabal-test"

    - path: "./nix-src"
      component: "lib:cabal-test"

    - path: "./app/Main.hs"
      component: "cabal-test:exe:cabal-test"

    - path: "./test"
      component: "cabal-test:test:cabal-test-test"

So not handling them would be even a regression.

jneira commented 4 years ago

For completeness, this is the hie-yaml using cabal-helper (with #4):

cradle:
  cabal:
    - path: "common-src\"
      component: "lib:cabal-test"

    - path: "src\"
      component: "lib:cabal-test"

    - path: "win-src\"
      component: "lib:cabal-test"

    - path: "common-src\"
      component: "cabal-test:exe:cabal-test"

    - path: "app\"
      component: "cabal-test:exe:cabal-test"

    - path: "test\"
      component: "cabal-test:test:cabal-test-test"

As expected, it resolves the conditions with the actual flags values (i am on windows) and handles common stanzas the right way. But i think we could get a similar result using cabal-install-parsers and this pr as base.

maksbotan commented 4 years ago

What would be the improvements in the generated config using this pr instead the actual custom parser? does it handle common stanzas?

My primary motivation was to make implicit-hie "automatically" support all of cabal's syntax. I thought of this after making PR #12.

Otoh, the actual lib version includes some condition handling!

Right, I haven't yet done traversing of conditional tree. I probably should just use flattenPackageDescription as you (?) proposed in #4.

jneira commented 4 years ago

My primary motivation was to make implicit-hie "automatically" support all of cabal's syntax. I thought of this after making PR #12.

That is a really good point.

Right, I haven't yet done traversing of conditional tree. I probably should just use flattenPackageDescription as you (?) proposed in #4.

Well, it is a possible path; it resolves conditions without having to set flags values (you would need a full config execution to get them). Afaik it merges all branches so maybe it gives us what we need. Another approximation could be copy part of its logic, focusing in the elements of interest to compute the paths/component pair (hs-source-dir, *-modules, etc) and adapting to our needs.

Avi-D-coder commented 1 year ago

Superseded by https://github.com/Avi-D-coder/implicit-hie/pull/48

Avi-D-coder / implicit-hie

Use cabal-install-parsers and Cabal parser #13