flycheck-ocaml breaks under company-mode and auto-complete-mode

[drvink]

flycheck-ocaml breaks if company-mode or auto-complete-mode (or both) are enabled. Strange parts of the buffer will get highlighted as errors; in particular, messages like these are common:

Merlin was already processing ((tell start at ((assoc) (line . 35) (col . 31)))) while ((tell start at ((assoc) (line . 35) (col . 31)))) was attempted

With auto-completion-mode specifically, Emacs will lock up after showing a completion listing.

I'm seeing this on Emacs 24.5.1 with the latest flycheck, merlin, company, and auto-complete.

[swsnr]

@drvink I remember a similar error and lock ups while writing this extension, but I thought that this issue was fixed in Merlin. Apparently, it's not, or it re-appeared.

Unfortunately, I'm not doing any OCaml of late, and I lack the time to investigate and fix this issue. Pull requests or patches welcome.

Note, though, that this might not be an issue in this extension specifically. It just uses the Merlin API to check the buffer, so the issue might actually be in Merlin. Consider reporting it to Merlin instead.

[let-def]

I gave a more detailed answer on merlin issue. I know how to fix that, there is small change needed in the use of merlin API, but as I am not using flycheck I can't test.

Also, I started cleaning up the emacs code, which will introduce another round of api breakage in some future release, but as I know flycheck is relying on some part of the code, I will make sure not to break it again.

[swsnr]

@def-lkb Break whatever you want to break :) As long as I get a notice, I'm fine with adapting to a new API. I'll look at your response over at Merlin, and make the necessary changes to Flycheck.

[swsnr]

@def-lkb Ok, I've read your response, and I'll happily transition to the “asynchronous interface”. If you could just give me a hint on what this interface actually is :)

I thought I was using this interface already: Flycheck calls merlin-send-command-async to check the current buffer. Isn't that the asynchronous interface?

Or is the problem the synchronous call to merlin-sync-to-point? If so, what would I use instead? The idea of this call is to make sure that Merlin has the latest “version” of the buffer so that the errors are accurate, but it's probably not the right approach. I remember that this call has caused issues in the past, too. I think I even reported an issue to Merlin back when I wrote this extension…

[let-def]

Yep, your mode is already doing things in the correct way. The problem is indeed the synchronous call to merlin-sync-to-point. I am not sure how to work around that, doing async things in emacs is a bit fragile. It would need some notion of transactions, either in emacs or in merlin, to make sure consecutive async commands are executed on the same buffer, with a consistent state.

In the meantime, I would suggest not doing anything when merlin is busy: the errors won't get updated for this round, but they will get once completion is finished. Would you agree?

By the way, there is no need to capture the buffer, merlin async handler makes sure the closure is invoked with the appropriate buffer in scope.

[swsnr]

In the meantime, I would suggest not doing anything when merlin is busy: the errors won't get updated for this round, but they will get once completion is finished. Would you agree?

Yes, sure. I think that'd be sufficient, since a completion usually changes the buffer contents, which quickly triggers the next syntax check anyway.

How can I check whether merlin is busy?

[let-def]

This should do it:

(unless (merlin--process-busy)
   ...)

The name is private (as indicated by the "--"), but that's part of the API revamp, I'll keep you posted if this was to change.

[swsnr]

@def-lkb Like this? Or should I guard the merlin-send-command-async as well?

The current patch has the advantage that Flycheck still gets the old errors from Merlin, and won't accidentally report the buffer as clean even though it isn't. It'd provide a more consistent user experience.

[let-def]

The problem is that without the sync, merlin-send-command-async might have a partial view the buffer (the one left by completion), so it might even be worse.

The right thing to do is to cache the errors, update the cache when merlin is not busy, reuse the cache otherwise.

[swsnr]

@def-lkb Okay, will change it accordingly.

[swsnr]

@def-lkb So like this?

[let-def]

looks good to me!

[drvink]

I'm still seeing the issue. I can provide my .emacs if it'll help.

[swsnr]

Are sure that you are using the very latest versions of Flycheck OCaml and Merlin?

[drvink]

flycheck-ocaml: 20150603.758 merlin: 20150528.759

These are the most recent, right?

[swsnr]

@drvink Yes, I think so. The issue is apparently not fixed then. Still, what I said three days ago still holds. I lack the time—and, honestly, the motivation—to investigate and fix this issue.

You may try to enable the debugger (M-x toggle-debug-on-error) and paste the resulting backtrace here (if it's longer, please make a separate Gist for it). If the backtrace points to something obvious, I'll try to fix it, but if it doesn't—which is what I fear—I'm afraid you're on your own. Sorry.

[let-def]

I can't reproduce with the latest version.

[drvink]

Here's the backtrace.

[let-def]

Thanks that's helpful!

[drvink]

BTW, auto-complete-mode works fine for me now (no freezes, completion works fine), so the issue is only now when using company-mode.

[let-def]

The only way I can imagine to fix that is that rather than flycheck checking whether merlin is busy, it's merlin telling flycheck that synchronization has failed, and that it cannot get meaningful errors this time.

[swsnr]

@def-lkb I'm not sure. Doesn't the fact that auto-complete works whereas company does not rather indicate an issue with Merlin's support for Company?

[let-def]

It's a race condition in simulation of concurrency inside emacs. Even if auto-complete works (doesn't complain), it doesn't mean its correct.

[let-def]

The fundamental flaw is trying to simulate a synchronous model within emacs. But if we weren't able to answer synchronously, lots of commands would be useless. I propose is "best effort" approach: emacs mode tries to simulate synchronicity, if it is not possible, it offers the developer the opportunity to handle failure.

[let-def]

The change would be along the lines of:

(let ((merlin-command-priority 0)) ; be really nice, let any other mode cancel the command if needed
  (condition-case nil
      (progn
        ;; query errors
        )
    (merlin-canceled
      ;; return cache
      )))

[let-def]

This is a patched version https://github.com/def-lkb/flycheck-ocaml that @drvink uses with success so far. I will make a pull request soon.

[swsnr]

@def-lkb Would be great, thanks!

[ddovod]

Hello guys. I have the same issue and would like to ask you, when the patched version of flycheck-ocaml will appear? Thank you a lot!

[swsnr]

@ddovod It did already appear.

[let-def]

@ddovod You need a git version of Merlin though, also @drvink reported that it is not always fixed, I didn't have time to take a look at that yet.

[ddovod]

Ou, i missed that. Okay, I will try it today's evening. Thanks guys!

[swsnr]

With #5 being merged this should be fixed for very recent versions of Merlin. I'm closing this now.

[let-def]

Yep sorry for not responding on the pull request, I am rushing on other stuff right now.

[swsnr]

@def-lkb That's fine :) Thanks for this change.

By the way, you might be interested in https://github.com/flycheck/flycheck/issues/774 which aims at providing a better API for modes like merlin that have a better understanding of when to check files. Essentially it would remove the need to define a syntax checker and let Merlin directly add annotations to the buffer through Flycheck.