implicit-hie icon indicating copy to clipboard operation
implicit-hie copied to clipboard

Use cabal-install-parsers and Cabal parser

Open maksbotan opened this issue 5 years ago • 14 comments
trafficstars

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?

maksbotan avatar Jun 05 '20 22:06 maksbotan

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 avatar Jun 05 '20 23:06 Avi-D-coder

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

Avi-D-coder avatar Jun 05 '20 23:06 Avi-D-coder

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"

Avi-D-coder avatar Jun 06 '20 19:06 Avi-D-coder

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?

maksbotan avatar Jun 06 '20 19:06 maksbotan

Maybe I have not experimented with that case.

Avi-D-coder avatar Jun 06 '20 20:06 Avi-D-coder

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 avatar Jun 07 '20 20:06 jneira

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
  • ???

jneira avatar Jun 07 '20 20:06 jneira

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 avatar Jun 07 '20 21:06 maksbotan

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...

maksbotan avatar Jun 07 '20 21:06 maksbotan

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 avatar Jun 08 '20 06:06 jneira

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 avatar Jun 08 '20 06:06 jneira

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.

jneira avatar Jun 08 '20 06:06 jneira

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.

maksbotan avatar Jun 08 '20 08:06 maksbotan

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.

jneira avatar Jun 08 '20 08:06 jneira

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

Avi-D-coder avatar Oct 19 '22 05:10 Avi-D-coder