haskell / haskell-mode

Emacs mode for Haskell
http://haskell.github.io/haskell-mode/
GNU General Public License v3.0
1.33k stars 342 forks source link

Integrate haskell-flycheck into haskell-mode #538

Closed lukehoersten closed 9 years ago

lukehoersten commented 9 years ago

haskell-flycheck it's @chrisdone's flycheck implementation that uses a haskell-process as the backend.

Flycheck comes with a built-in flycheck-haskell that uses GHC. The Flycheck author, @lunaryorn, has the policy of not having Flycheck depend on external modes as it's untenable. Since haskell-flycheck is dependent on haskell-process within haskell-mode anyway, I think it makes sense to include @chrisdone's haskell-flycheck in this mode.

I'd be open to making the code changes myself and submitting a PR but I'm not sure the best approach to integrating something like this, especially since it's conditional on the availability of Flycheck.

swsnr commented 9 years ago

This was already discussed somewhere else (can't remember where, currently), and basically I won't merge any of @chrisdone's code unless he agrees personally—that's just a matter of fairness, imho—, and I won't merge it, if I have to maintain it on my own. It's too complicated for that, and I don't know Haskell Mode well enough, nor do I have the time to get to know it.

IOW, I'll only merge this checker, if @chrisdone agrees, and if he or someone else other than me agrees to maintain this checker as part of this repository.

lukehoersten commented 9 years ago

We discussed in a haskell-flycheck issue previously.

We don't have to merge it into flychecker. That was just one option. The other option is going into haskell-mode which I think we agreed on in our previous discussions. That works nicely because it keeps all the haskell-process stuff together.

Either way I want it to turn into something easy enough for people to get that it starts getting wider adoption. That's my whole goal here.

swsnr commented 9 years ago

@LukeHoersten Oh sorry, there's apparently a misunderstanding. flycheck-haskell is not built-in. It's a Flycheck extension, which isn't subject to any policies about Flycheck itself.

As a matter of fact, flycheck-haskell already depends on Haskell Mode already, albeit only for some minor stuff.

I understand, that you'd like to have a haskell-process based checker published, but the fact that @chrisdone hasn't published it yet makes me think that it's probably not ready yet for a public release.

Anyhow, I'm not at all opposed to merging everything in flycheck-haskell, as long as @chrisdone agrees, and someone else other than me actually maintains the checker.

lukehoersten commented 9 years ago

I do think it makes sense to preserve the current checker as an option (maybe even the default option) as well. I'm not so concerned with where the package goes, just as long as it's reachable form package.el.

swsnr commented 9 years ago

@LukeHoersten The current checker will stay anyway, since it's part of Flycheck itself, and still has a purpose, e.g. for small haskell sources, which don't have a cabal file. Flycheck Haskell currently just parses Cabal files and sets up Flycheck accordingly. It doesn't have any checkers on its own.

But again, the decision whether to make the haskell-process based checker available via package.el is not ours.

lukehoersten commented 9 years ago

Be patient and give him a chance to respond. To be clear: I'm not disputing that the decision to merge is up to @chrisdone.

In the mean time, I don't want to confuse the discussion about how we would integrate the checkers with whether @chrisdone is OK with it. @chrisdone has mentioned earlier that integration specifics is his main concern:

chrisdone commented on Feb 5 I'm okay with merging this into flycheck-haskell if it unifies a common thing and avoids confusion. I'm concerned about pissing off your users by swapping out the backend with something that requires starting a session and ghci (things that people regularly complain about as being such a chore).

So this brings me back to my last point: how will this work? We have a few options:

  1. Merge flycheck-haskell and haskell-flycheck into the flycheck-haskell package.el then auto-detect haskell-process if a session is started or just require the user to configure themselves.
  2. Merge haskell-flycheck into haskell-mode to consolidate dependencies and default to the haskell-process checker.
  3. Keep haskell-flycheck and flycheck-haskell separate package.el packages and the use of either would auto-enable the corresponding checker.

The focus here is on how does the user specify which checker should be used. If you're using haskell-process already, it seems unlikely the user would want to use a separate GHC process for flychecking.

swsnr commented 9 years ago

@LukeHoersten In my opinion it makes no chance for us to discuss anything as long as he hasn't responded.

lukehoersten commented 9 years ago

From our previous discussion:

chrisdone commented on Feb 5 I'm okay with merging this into flycheck-haskell if it unifies a common thing and avoids confusion. I'm concerned about pissing off your users by swapping out the backend with something that requires starting a session and ghci (things that people regularly complain about as being such a chore).

swsnr commented 9 years ago

@LukeHoersten I read “concerned” first, whereas you apparently read “I'm okay” :)

But I guess we can take that as agreement of @chrisdone, so please feel free (you or @chrisdone, whoever wants) to open a pull request that adds his Cabal Repl checker to this repository. If @chrisdone wants, I can give him write access, but I'd like to review the checker before merging it into this package and thus released it to a greater audience.

If another way of integration is desired, e.g. moving everything into Haskell Mode or so, please tell me. I don't cling to this repo, so if you see a better place for this code, I'd be ok to move it somewhere else.

lukehoersten commented 9 years ago

@lunaryorn, Chris is concerned about changing the default checker but he's OK with merging the packages.

Preserving the default means we need to work out the technical details about how the two checkers will interact and become enabled. Until the technical details are worked out, we will not merge anything. Unless you are ready to contribute to this technical discussion I'm afraid we're stalled.

gracjan commented 9 years ago

@lunaryorn: I know that you were in the middle of rewrite of generalized checker. How far are you with this?

Currently haskell-mode has a lot of ways to check Haskell code, including the new ide-backend. There is quite a lot of cleanup needed in haskell-mode before we can handle yet one checker.

@LukeHoersten: I knew that flychecker-haskell uses haskell-cabal.el for a reason but I did not know that it uses haskell-process.el for anything. How does it use it?

gracjan commented 9 years ago

Note that we still have remnants of flymake in the code, those are reported to not always work as in #384 or #130.

lukehoersten commented 9 years ago

@gracjan:

I don't believe that haskell-flycheck depends on anything but haskell-mode (for haskell-process) and flycheck. haskell-process is what it uses for cabal integration (via ghci etc).

My comment above enumerates some of the possibilities and potential tradeoffs of merging haskell-flycheck into haskell-mode (because of it's haskell-process dep) or lumping it in with the flycheck-haskell package as just sort of an "extras" package for haskell emacs flycheck stuff.

My personal preference is to merge haskell-flycheck into the flycheck-haskell package and then just give users a way to switch over the haskell-process-checker if they're using haskell-process already or something like that (the mechanics still need to be worked out). We'd leave the default as the ghc-checker in this case.

gracjan commented 9 years ago

I did not know that haskell-flycheck is something different than flycheck-haskell. It that case at least half of my comment does not apply :)

lukehoersten commented 9 years ago

In hind sight haskell-flycheck should really be called flycheck-haskell-process and flycheck-haskell should be called flycheck-haskell-cabal or something like that. flycheck proper already has a checker called ghc-checker. This ambiguity is exactly what I'm trying to resolve here while simultaneously simplifying configuration and access to these projects.

NOTE: I'm not really proposing those name changes, just using them for illustrative purposes.

swsnr commented 9 years ago

@gracjan I'm not rewriting the built-in GHC syntax checker. I wonder where you got this idea from. I had the idea to use cabal build for syntax checking, in this package, but it seems redundant now in light of a haskell-process based checker.

@LukeHoersten We are not stalled then. I'll contribute to the technical aspects of the syntax checker implementation, with regards to how we can seamlessly switch between GHC and haskell-process based checking depending on whether a running Cabal REPL is available for the current buffer. Flycheck already provides the necessary infrastructure, and if haskell-process does, too, we shouldn't have any problems.

I'd just like to ask you to open a PR with the new checker first. It's just easier to discuss technical aspects with the code at hand :)

chrisdone commented 9 years ago

To be clear, I wrote haskell-flycheck as an experiment, I'm not convinced it's a good idea, as it has subtle interactions with e.g. the REPL context. Although it seems to work.

Relatedly, ide-backend being open sourced changes the situation. Perhaps in a month we'll have a flycheck checker for ide-backend-client, so this discussion could be for naught. I don't know.

gracjan commented 9 years ago

Good to know, thanks for the update.

swsnr commented 9 years ago

@chrisdone Since you've already implemented it, it's worth a try imho. If it's too flaky we can always return to the simple GHC checker. Thanks to Flycheck's automatic checker selection it should be mostly transparent to the user.

I'm not sure whether ide-backend changes the game currently. If I remember the announcement post aright, it doesn't yet expose compilation errors… if it did, thought, it'd be trivial to call out to die-backend to check the current buffer.

chrisdone commented 9 years ago

Since you've already implemented it, it's worth a try imho. If it's too flaky we can always return to the simple GHC checker. Thanks to Flycheck's automatic checker selection it should be mostly transparent to the user.

Fair enough. If it's not disrupting existing users of Flycheck-haskell it seems fine to me.

I'm not sure whether ide-backend changes the game currently. If I remember the announcement post aright, it doesn't yet expose compilation errors… if it did, thought, it'd be trivial to call out to die-backend to check the current buffer.

It does do that, I just didn't do anything interesting with the output in my emacs minor mode yet. It'll just be a case of calling getSourceErrors from the backend and asynchronously waiting on a list of errors, similar to what I'm doing with this haskell-flycheck mode. It should be a trivial checker to write, but I didn't get round to it yet.

chrisdone commented 9 years ago

@lunaryorn Example, if interested: https://github.com/chrisdone/ide-backend-mode/blob/master/ide-backend-mode.el#L432 Here I'm just message'ing them but of course that could easily be pushed into a list via flycheck's async API.

lukehoersten commented 9 years ago

Does everyone generally agree that we should be using ide-backend as the incremental compile (flycheck), code completion, and code navigation backend?

This means we now have two processes running during dev work: ghci for repl and ide-backend for everything else. This isn't necessarily bad, just clarifying? A goal of mine is to keep the number of external dependencies to a minimum in my dev environment just to keep things simple and fast.

If that's the case, @chrisdone would you just prefer waiting until you've integrated ide-backend with flycheck, autocomplete or company, and some code nav system instead of releasing haskell-flycheck in some fashion? What's the ETA on the backend integration?

chrisdone commented 9 years ago

I don't think anyone yet is using ide-backend, but it's a possible future contender as it's a high-quality implementation worked on by Well-Typed.

This means we now have two processes running during dev work: ghci for repl and ide-backend for everything else.

Actually ide-backend supports running statements (REPL stuff) and debugger stuff, although this was never exposed by the public online FP Complete IDE. The functionality is there. So really it would just be "the backend" for everything, ideally speaking. Similar to the interactive-haskell-mode minor mode, ide-backend-mode provides some keybindings that know about having a "cabal project", loading and queuing commands and such. Others may dislike the fact you have to choose an interaction minor mode, but such infrastructure was anticipating future potential "better" modes.

If that's the case, @chrisdone would you just prefer waiting until you've integrated ide-backend with flycheck, autocomplete or company, and some code nav system instead of releasing haskell-flycheck in some fashion? What's the ETA on the backend integration?

That's what I'm thinking. Here is the client: https://github.com/chrisdone/ide-backend-client and here is the mode: https://github.com/chrisdone/ide-backend-mode but it's pre-alpha. I don't have an ETA but it's less than an hour's work to get the flycheck checker defined. I don't know company-mode as a library, but it'd probably be an hour to integrate autocomplete.el, so assuming the diff between company-mode and autocomplete.el is small then it should also be easy.

The instigator for me will be when I get some spare time I'll start trying to use ide-backend-mode for my real work projects and at that time it'll be a forcing function for fleshing out the rest. I'll probably move both projects under the haskell organization at that point.

lukehoersten commented 9 years ago

Makes sense. One of the things I liked about using ghci (or ghc itself) as the backend for flycheck and code nav is that it's kept up to date with ghc by core devs. If FPco decides to move on from ide-backend (hypothetically) then we stop getting support for new lang extensions etc. What does everyone think about this? GHC and GHCi move in lock-step.

chrisdone commented 9 years ago

Technically the code nav and span type info come from ghci-ng which is maintained by me (which is why it doesn't support GHC 7.10 yet). GHCi proper is more limited, and harder to contribute to. If FP Complete don't upgrade ide-backend to support 7.12 then someone (e.g. me) will.

lukehoersten commented 9 years ago

Conceivably though if you got your ghci-ng patches into ghci proper it would be carried along with future work?

chrisdone commented 9 years ago

It is conceivable.

swsnr commented 9 years ago

@LukeHoersten ide-backend won't go away even if FPComplete stops to support it. We can still keep developing it in the community. We need something like it anyway to support editors.

I'm afraid, though, that you two lost me. I know nothing about ide-backend beyond what was said in the blog post, and certainly not enough to make a decision about what we should use for Flycheck.

That said, I'll happily merge any checker which you see fit for this repo and keep maintaining as good as I can, and I'll give you any help that you need when it comes to the technical aspects of writing a specific checker for Flycheck. But I'm afraid I can't contribute anything reasonable to this discussion anymore :)

lukehoersten commented 9 years ago

Since @chrisdone is the primary developer of both haskell-process and ide-backend, we're going to defer any more work on flycheck integrations in the hopes that ide-backend-mode supersedes haskell-process/ghci-ng-based integrations.

swsnr commented 9 years ago

@LukeHoersten So we'll let this matter rest until ide-backend-mode is ready?

lukehoersten commented 9 years ago

I think so, yes. We'll see how far Chris gets with the implementation. On Thu, Apr 23, 2015 at 1:37 AM Sebastian Wiesner notifications@github.com wrote:

@LukeHoersten https://github.com/LukeHoersten So we'll let this matter rest until ide-backend-mode is ready?

— Reply to this email directly or view it on GitHub https://github.com/haskell/haskell-mode/issues/538#issuecomment-95462041 .