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

Evolving the API #9

Closed
ndmitchell opened this issue Jun 28, 2019 · 8 comments · Fixed by #29
Closed

Evolving the API #9

ndmitchell opened this issue Jun 28, 2019 · 8 comments · Fixed by #29

Comments

@ndmitchell
Copy link
Contributor

hie-bios is great, but I think the current API doesn't quite address our needs. In hie-core we're driving it in slightly odd ways. After brainstorming internally, our thoughts around an API are:

-- | Takes a directory name, corresponding to the opened directory in something like `VS Code`.
--   If there is a `hie.yaml` it reads that, otherwise it sniffs out cabal/stack etc.
--   Errors are returned as exceptions
loadCradle :: FilePath -> IO Cradle

-- | Given a specific file, which is expected to be within the initial directory passed to loadCrade,
--   find the GHC flags (packages, extensions etc) required to load that file.
--   It is expected that in 99% of the cases the file will not be used, and everything within
--   a directory will return the same set of flags. If two files return different sets of flags,
--   the extension may choose to fail to load the second one.
--   The time required to execute cradleFlags should be minimal - it should not call
--   out to cabal.exe each time, but should do that once and cache the result.
cradleFlags :: FilePath -> IO [String]

-- | For a Cradle, what are the target files that are relevant for the project. For cabal,
--   that would be the main files for the executables and all listed modules.
--   Any refactoring or errors would be expected to report on all cradle targets.
cradleTargets :: Cradle -> IO [FilePath]

The main changes from the existing API are that we cradleFlags to be vastly cheaper, caching its work so that we can call it on every key stroke. We also introduce cradleTargets, which is essential for a bunch of refactoring use cases, and also more generally for rapid feedback.

Assuming that plan seems reasonable, we're happy to help with some of the heavy lifting to make it work. As part of the refactoring, I'd also suggest separating hie-bios from hie-ghc-helpers - so hie-bios no longer depends on the GHC API or includes those helper functions.

CC @cocreature @mpickering @alanz

@ndmitchell ndmitchell changed the title Designing an API Evolving the API Jun 28, 2019
@mpickering
Copy link
Collaborator

I have been thinking about your suggestions. Thanks for posting. Firstly, I don't want hie-bios to be in the business of managing any cache, it makes the library more opinionated and clients are capable of managing it themselves.

In the current module, you call loadCradle and then the client is supposed to either

  1. Inspect the targets that were passed with the options to learn about the extent of a component.
  2. Load the component using the GHC API and then consult the module graph to learn the extent of a component.

Then if you require information about a component in future, you first consult your cache to see if you already have a suitable session for the path you are trying to load and reuse it rather than call loadCradle again with a new path. cradleTargets can also be implemented directly in this manner.

For cradleTargets do you expect it to act across multiple components?

So I think that you can already get your desired behaviour with the existing API (at least this is how I manage it on my fork of h-i-e)

The part about separating out the ghc specific helpers into a separate package I agree with, and would like to do so.

@ndmitchell
Copy link
Contributor Author

Thanks, I think I'm seeing the design - whatever conclusions we come to, I'll volunteer to write the "semantics of cradles" documentation somewhere. To keep things concrete, let's imagine the .cabal file:

name: lib
library
  exposed-modules:
    Public1
    Public2
  other-modules:
    Private1

executable
   main-is: Main
   depends: lib

How are the clients mean to know that if lib.cabal changes then they should request fresh flags? I don't see any way to anything but the Cradle knowing that information. However, I'm also content that it's a bit more of a corner case, and if it wasn't managed properly it wouldn't be the end of the world.

If I ask for Public1 through findCradle, do you expect it to return a set of flags that also cause Public2 to be loaded? And thus am I expected to infer that because Public1 also caused Public2 to enter the module graph, then it's the case that Public2 must take the same flags?

If I ask for Main will I get an environment that also works for Public1 and Public2? And conversely, will Public1 know about Main? If they do know about each other, does that mean you flatten components, and how would you cope if the executable listed different extensions? If they don't know about each other, how do I edit the executable and the library simultaneously? How can I apply refactorings between them.

If the code is broken when I start, say with a parse error, how can I then find the extend of the component? Or is that not supported?

@mpickering
Copy link
Collaborator

I think the answer to these questions is that it depends on the build system.

If you are not using a build system then if one of your modules has a parse error then there is no way to know the complete module graph. If you are have a project which has a .cabal file then the cradle can report the entire component when you query for a file without having to parse any .hs files. In h-i-e I eagerly load a whole component on initial load so that the subsequent refreshes are faster because of the cached .hi files.

The concept of a "component" is also an artificial one created by a build tool. Your tricky questions about how components should be handled by how the build tool wants to map files to components. The responsibility for creating the right environment for a file to be loaded into belongs to the build tool. Cabal has been famously reticent to add multi-component targets because it is not predictable how combining things like ghc-options should work.

I think you are right that the cradle needs to report something about what should require it to be reloaded. Just as in ghcid you can specify the --restart flag to say when a restart should happen. Supporting this with the explicit configuration is easy but I'm unsure about how to handle this with the implicit configurations. It would have to be something that build tools reported but I dont't think they are as setup to provide this information currently.

@ndmitchell
Copy link
Contributor Author

Agree it depends on the build system, but I think it would be good to pin down what Cabal should do, since what I'm really trying to figure out is what the semantics of cradles are using Cabal as an example. In particular, you seem to be assuming that Public1.hs and Public2.hs have the same set of flags, but I see no reason to expect that with your definition of the semantics of cradles, and given the lack of caching, actually checking is probably prohibitive.

@mpickering
Copy link
Collaborator

I am having a hard time coming up with anything precise to say about this. You would expect all files in a component to be in the same cradle but is that anything other than an expectation?

@ndmitchell
Copy link
Contributor Author

ndmitchell commented Sep 8, 2019

After discussions:

-- | Given root/foo/bar.hs, return root/hie.yaml, or wherever the yaml file was found
findCradle :: FilePath -> IO (Maybe FilePath)

-- | Given root/hie.yaml load the Cradle
loadCradle :: FilePath -> IO Cradle

-- | Given root/foo/bar.hs (because I couldn't find a hie.yaml) just guess
loadImplicitCradle :: FilePath -> IO Cradle

We should also add cradleDependencies :: [FilePath] being the files in a cradle whose changing should result in the Cradle being rerun.

CC @fendor

@mpickering
Copy link
Collaborator

How is the findCradle command going to work if there is no config file?

@ndmitchell
Copy link
Contributor Author

@mpickering if there is no config file it returns Nothing and you would call loadImplicitCradle. I'm not convinced loadImplicitCradle is right, but it's close enough that we can work with it for now.

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 a pull request may close this issue.

2 participants