New syntax checker chaining system

[swsnr]

See #836 for a motivation. To summarize, the current system is cumbersome to maintain and hard to understand. This in-progress PR intends to provide a new simpler system that addresses these shortcomings.

I'm opening it early to collect feedback on the approach and the implementation. Inviting @flycheck/core-developers, @flycheck/maintainers, @mgudemann and @purcell.

Connects to #836.

Design

Instead of :next-checkers we now have three properties to control when a checker is selected:

• :stage: The kind of syntax checker, e.g. syntax, type, lint, style, test, which serves to order checker execution.
• :conflicts-with: A list of syntax checkers this syntax checker conflicts with, i.e. may not be used together with.
• :maximum-level: The maximum error level in the current buffer this syntax checker should still run at.

I hope that the meaning of these properties is fairly obvious: Flycheck runs all syntax checkers in order of their :stage (e.g. syntax checkers before type checkers, and so on). To find all checkers it goes through flycheck-checkers and collects all checkers that support the current buffer, and then evicts conflicting checkers from first to last.

Finally, after it has assembled the chain, it runs all syntax checkers one by one, and skips over any checker if the highest error level in the current buffer is greater (by priority) than the :maximum-level.

Pros

• The new system is easier to extend. You just need to declare a proper :stage and the new checker seamlessly plugs into existing ones. There's no need for flycheck-add-next-checker anymore.
• It's also easier to understand, because there's no ambiguity in the meaning anymore. With :next-checkers it was never obvious whether Flycheck would try to run all or just the first.
• It scales a lot better: For large chains, e.g. Go, it's much less maintenance work, because adding a new checker doesn't imply updating :next-checkers of all other Go checkers anymore.

Cons

• With the new system selecting individual checkers doesn't make much sense anymore, so flycheck-select-checker and related functionality is gone. We'll recommend buffer-local bindings of flycheck-checkers as alternative, which is actually simpler, too. But there'll not be an interactive command anymore.
• It's a breaking change. That'll bring some inconvenience to extension authors, but we'll update all official extensions. Others are on their own anyway, and I'd rather break things once than try and maintain a stub for :next-checkers indefinitely.

Status

• Collect a syntax checker chain
• Declare chains in all syntax checkers
• Run a syntax checker chain
• Rename :kind to :stage
• Filter remaining checkers by maximum level
• Remove old next-checkers functionality
• Remove flycheck-select-checker and friends
• Add support for buffer local bindings of flycheck-checkers
• Document the new chaining

Todo after merging

• Update official Flycheck extensions that use :next-checkers.

[purcell]

Sounds really good to me!

[Simplify]

@lunaryorn I like it, but can think of few edge cases and have a few questions/suggestions:

• Is it possible for user to change modes for specific checker in this new system? Will flycheck-add-mode still be available? For example ESLint is JavaScript checker, but if you install ruleset with npm and add path to those rules to ESLint config file, you can use ESLint as checker for TypeScript.
• Half a year ago I made flycheck-css-colorguard that detects similar colors in CSS. I have hard time to sort it as syntax, type, lint, style or test. maybe add other between style and test or after test?
• is there a way for user to change :maximum-level per buffer or "project"? Without this setting all third party checkers may use :error as :maximum-level just to get their checkers to always display errors/warnings and that may stop execution of some built-in checkers.
• Ability to change :maximum-level may be important for JavaScript projects. Often you make bunch of js files that at the end get combined in one single js file. In some of those files JS checkers can display errors because of missing function declaration in other file of because it can't find library that you use (often jQuery included in HTML header). That may stop execution of other JS checker that have maximum-level as :warning. In my case, I always want to run jscs after eslint because that checker has best support for checking 'JSDoc' documentation in the code, that no other JS checker can match. Currently jscs is getting merged into eslint, but that process is not over yet.
• My understanding after reading New system for chaining checkers #836 is that you don't want users to change :maximum-level and :conflicts-with. I think that list of checkers has still to be user preference. It's nice to be opinionated and have best defaults, but ability to change those for your needs is also important. For example eslint and jscs are very similar, they are even merging now, but eslint is better are ES6 support and jscs is better at jsdoc. It would be bad if they :conflicts-with each other, for example. Also, some JS checkers support extensions/rulesets that change behavior/quality of that checker in some area. In that case you may want to use two checkers with extensions in different checking areas that :conflicts-with each other.

[Simplify]

One more question: Will you use :kind only for initial sorting or also for checker execution? I mean, that lint checker with :maximum-level set to :warning always run, even if syntax checker find :error in the code?

[swsnr]

@Simplify

• Sure, we'll keep flycheck-add-mode.
• I don't know what colorguard does, but presumably it'd be lint or style. Depends on when you'd like the syntax checker to run. Presumably it shouldn't run when a buffer has invalid CSS syntax, so you'd put it after syntax to make sure that a syntax error inhibits a colorguard check. An other kind is not a good idea.
• No, it won't. The maximum level is an inherent property of a syntax checker: You can reasonably lint a buffer that has a syntax error. I'm not sure what risk you're seeing here. If 3rd party syntax checker deliberately report wrong error levels or use a maximum level that's a bug in these syntax checkers.
• I disagree. I think if you've implicit globals in your Javascript project you should rather configure eslint to know about these globals than try to work around spurious errors with Flycheck.
• You can always use flycheck-disabled-checkers to disable specific syntax checkers and thus prevent them from being used. :conflicts-with is intended for case where it really makes not sense to use both, e.g. you'd not use jshint and eslint, or Clang and GCC, or ruby and rubocop in the same buffer.
• Yes, :kind will obviously affect execution order. If a syntax checker reports an error of error level it'll inhibit a lint checker with a maximum-level of warning. I mean that's the whole point of having kind and maximum-level in the first place, and there's reason behind: Many linters don't handle syntax error gracefully, or may report a lot of spurious errors when applied on a syntactically invalid buffer.

TL;DR: flycheck-add-mode will stay. kind, conflicts-with and maximum-level won't be customisable, because I don't see why they should. Use flycheck-disabled-checkers or a buffer-local flycheck-checkers to influence syntax checker selection.

[mkcode]

@lunaryorn - Continuing our discussion from #939.

This new syntax looks to be a huge improvement over the previous :next-checker syntax. When I think about how I would add the proselint checker with this system, the first question I ask is: which 'kind' would it fall into? I guess 'style'? Not really sure.

I would ask for a (hopefully simple) modification to the proposed method, to be able to define arbitrary kinds, and then use convention to correctly place most checkers. In reading through this issue and #836, this seems to be the biggest issue with what is proposed: users not being sure which category to place their checker into. If we had a flycheck-define-kind function or similar, it seems like these issues would not exist. You can always create a new kind, but it is strongly advised that you try to use an existing kind. I believe that proselint would be one example that would be in it's own kind.

[swsnr]

@mkcode With arbitrary kinds how'd you order syntax checkers? What kind of "convention" do you have in mind?

And why should proselint have its own kind? Isn't it a style checker, obviously?

[mkcode]

What kind of "convention" do you have in mind?

The convention I had in mind are the kinds you are documenting in this PR. Eg: if there was a public flycheck-define-kind function it would be called by flycheck internally with syntax, type, lint, style, test as you have written in this PR. All new checkers would try their best to fit into one of these already defined kinds. If one does not happen to fit, then there is an escape option.

With arbitrary kinds how'd you order syntax checkers

Not sure. Perhaps have any user defined types run last.

And why should proselint have its own kind? Isn't it a style checker, obviously?

That isn't immediately obvious to me. That is what I would guess, but 'style' can mean different things in different contexts. The proselint checker isn't special, it's probably the least important of any! The purpose of it having it's own kind, would be so that it is always run - not dependent on any previous checkers. If you end up making kinds execute concurrently like it was mentioned, even better!

Looking at the code, I see no difference in how the different kinds are handled, so they are essentially different Queues - which the different checkers insert into if they want to wait for the results of a previous checker. Perhaps I am missing something and this is a bad idea? If so, Apologies for the PR pollution. Perhaps you would prefer to discuss in #939, and not here?

[purcell]

With arbitrary kinds how'd you order syntax checkers?

You'd have a list variable which defined the order, I guess. It wouldn't be bad IMO to expose that kind of thing.

[swsnr]

@mkcode I'm not sure whether I see the problem you're trying to solve. If you defined the proselint checker with :kind style and no maximum level it'd always run, wouldn't it?

@purcell I'm not sure, but I think I disagree. Making kinds configurable adds extra complexity, which I'd rather like to avoid unless I see a compelling reason. If there's a syntax checker that doesn't fit into any of said kinds I'd rather hard-code a new kind into Flycheck.

But proselint fits perfectly into style in my opinion. After all, that's what it does: It checks writing style, doesn't it?

[Simplify]

After reading what proselint does, I'll also put it in style. But some checks from spelling or security have nothing to do with style. I guess we can just ignore those, otherwise we'll have same problem with majority of JavaScript linters.
However, I still think that we need kind other. Take a look at the screenshot on flycheck-css-colorguard. I still have hard time to place this checker into any of declared kinds.

[swsnr]

@Simplify Ah, I thought that proselint would only check for writing style. But if it does more, why not put it into lint? That'd be a natural choice as well, given the name of the tool.

But ultimately it doesn't really matter: kind just tells Flycheck when to run the syntax checker, so choose it accordingly. You could also think of it as a priority from 1 to 5.

I'm against an other kind, though; I think that this name is way too generic and won't help to address any possible confusion about kinds. If you're unsure about whether it's style or lint, then other won't help, imho.

From what I see in the screenshot, colorguard would be a style checker, wouldn't it?

[Simplify]

The problem is that most developers, including me, think of style checkers as something that tell you where to use space, add or remove new line or something like that, in order to comply with coding standards from community or company. I can't see colorguard as style checker, for me it belongs more in lint. However, if I apply kind as priority level, as you suggested, colorguard is least important and belongs in style.

[JAremko]

Might be it will be better to use something like :suppresses and :suppressed-by instead of :conflicts-with to avoid ambiguity.

[swsnr]

@JAremko In how far do you think :conflicts-with is ambiguous? I'm fine with changing the terminology—I'm not convinced that the current names are good anyway—but I'd like to understand why the current names are difficult to understand.

---

Given the apparent obscurity of :kind, we should find a better name. Maybe we should abstract less and expose the technical meaning of the property in the name? As @mkcode said, this property conceptually denotes a running queue for syntax checkers[1], so maybe we should just call it :run-queue?

---

[1] Technically the chain is linearized currently, though, for simplicity. For this reason parallel execution of checkers of the same "kind" won't come any time soon. The current goal is to get a system that scales better in terms of maintenance overhead, and not to improve performance.

[JAremko]

In how far do you think :conflicts-with is ambiguous? I'm fine with changing the terminology—I'm not convinced that the current names are good anyway—but I'd like to understand why the current names are difficult to understand.

My input was ambiguous itself, sorry. 😄 I was talking about ambiguous behavior not wording. When two checkers conflict with each other, which one will run?

• If I created a linter. That is also a part of metal-inter pack (like gometalinter) I would use :suppressed-by gometalinter.
• If gometalinter devs want to add some old-linter that also has standalone flycheck version, but it's out of their reach - they can use :suppresses old-linter
• Same with checkers that might have a trade off between speed/scope/standard.

I think it's nice if end user could just use it without solving the conflicts.

[JAremko]

Another good example. Mediocre linter that works with major modes A B C and really good one that works only with B. So in B mode the second one(good) will use :suppresses to disable the other one(mediocre).

[swsnr]

@Simplify The current implementation resolves conflicts in order of flycheck-checkers. It takes the first existing checker that supports the current buffer (as by :modes and :predicate) and then one by one adds each checker which also supports the current buffer unless it conflicts—in either direction—with any checker already added.

Regarding your example, if you wrote a syntax checker for a "meta-linter" you'd add :conflicts-with to its declaration containing all checkers covered by the meta-linter, and then put it first in flycheck-checkers. Flycheck would pick the meta-linter and then subsequently evict all syntax checkers that the meta-linter conflicts with.

Likewise in your second example: Put the good linter first in flycheck-checkers and let it conflict with the mediocre linter. Since the good linter only supports major mode B, Flycheck will not even consider it for major modes A and C, but in mode B it'll prefer the good linter over the mediocre one.

I think it's nice if end user could just use it without solving the conflicts.

I'm not sure what you mean, but it's Flycheck's aim to work with minimal or no user interaction required and I think the new syntax checker chaining system proposed by this PR goes a long way to achieve this goal.

But at some point a user has to make a choice if there are equivalent linters. Flycheck can't know whether you prefer eslint or jshint for your Javascript, or GCC or Clang for your C. That's what flycheck-disabled-checkers is for, and I think we should explicitly encourage buffer-local bindings for flycheck-checkers as well.

That covers both ways of making a choice: Opting out from particular syntax checkers as well as explicitly choosing what you'd like to use.

[cpitclaudel]

This new system sounds very nice and well-designed. I think the potential for confusion between levels isn't too worrying. Some small questions before I make a pass on the code:

• Do we even need a :kind? I haven't used many of the languages that have multiple chained checkers, so I'm not sure how useful it is to order checkers. IOW, how much would we loose if we always ran all checkers? (And, relatedly, how much would we gain in terms of code simplicity?). How hard would it be to add a(n optionally buffer-local) setting to always run all checkers?
• Regarding the cons:
  • breaking change: could we just issue a warning when a checker declares a "next-checker", and in that case assign a default priority? In a future version we would change this to an error. Would that be a heavy price to pay? Otherwise I'm afraid we'll have to coordinate with checkers to make joint releases (they can't release the update until we push the new Flycheck out, and if they don't push it soon enough things will break).
  • select-checker: Extending the proposal from above, maybe we could have a buffer-local variable called something like "checker-selection-strategy", which could be 'all, 'kind, or '(single . CHECKER); first one would run all checkers in order, kind would order them by :kind as outlined in your proposal, and (single . CHECKER) would just run CHECKER.

In any case, this looks like a great proposal :)

[cpitclaudel]

Hmm. Why is this change needed? Without it, I could imagine e.g. "ispell" being a flycheck checker applicable in all buffers.

[cpitclaudel]

Some related thoughts while looking at the diff:

• Do we really want to force all checkers to have a :mode?
• Regarding conflicts: do we really want gcc and clang to be marked as conflicting? On small files they provide convenient, distinct warnings, and having them both on can be nice.
• When there are multiple checker in the same stage, with one reporting errors and the other one reporting warnings, do we continue to the next stage? Currently we don't but it could be convenient (I can imagine a checker returning an error like "cannot use this checker on this project"—though maybe this should be handled by a predicate?).

[cpitclaudel]

This comment looks incomplete :)

[cpitclaudel]

(also: congrats on updating the checker definitions 👍)

[purcell]

I haven't used many of the languages that have multiple chained checkers, so I'm not sure how useful it is to order checkers. IOW, how much would we loose if we always ran all checkers?

I can think of a specific example, e.g. ghc and hlint for Haskell -- there's no point running hlint if the source file has parsing errors, since they would be redundantly reported. So there needs to be some sort of checker order, and some sort of condition to determine whether to run later checkers based on the result of earlier ones. (It would be fine to run them in parallel, I guess, and combine/filter results later - not sure if flycheck does that.)

[swsnr]

@cpitclaudel We do need :kind (aka :stage, I'm renaming it currently). Many linters don't handle syntax errors well, and in the presence of bad syntax errors many linter warnings do not make sense and would but clutter the buffer but not provide any reasonable feedback. Generally linters degrade badly in face of syntax errors.

I'd rather not like to preserve backwards compatibility. We've got no formal versioning policy for deprecations and thus can't give a timeframe for the removal anyway so we'd essentially just delay the breakage in my opinion. On the other hand we can't map :next-checkers directly to a :stage so even if we try and preserve backwards compatibility this change will affect 3rd party checkers immediately as it changes their chaining behaviour. I'd rather like to force everyone to update immediately and drag updates along for a indefinite time. And personally I loathe to maintain kludges for backwards compatibility 😊

As for your suggestion about a selection strategy, I don't known, but it feels really complicated. I can't imagine writing a docstring for that, let alone an implementation. If you can come up with a good implementation for that I'd be happy to talk about a PR, but I'd rather not like to delay this pull request for that suggestions, and I'd not like to implement it. I'd rather get rid of local checker selection entirely and see if we can get along with only disabled checkers and local bindings for flycheck-checkers. If we can't, if we get requests for more sophisticated checker selection, we can still discuss a new implementation of this feature.

Generally, I'd like to keep this pull request as simple as possible, to get it done soon. It's been open far too long. In fact, I do intend to get rid of :maximum-level as well, and hard-code the error level as the boundary—i.e. if there's no error in the buffer, the chain continues, if there is, it aborts. If that doesn't do the trick we can still introduce a more sophisticated system, but I think checker selection became too complex and this PR is also an attempt to simplify it.

For the same reason—simplification—I'd like to force :mode upon all syntax checkers. Actually I'm about to submit a separate pull request for this change. If you don't mind please lets move the discussion about a mandatory :mode property to this to-be-opened PR.

Likewise I'd defer the discussion about clang and GCC. Currently it's not possible to run both at the same time either because they don't chain to each other. I added :conflicts-with to preserve the current behaviour and didn't think about whether it makes sense or not. Please lets discuss changes to the chains of particular languages after this PR was merged. In the new system it'll be much easier to make such changes anyway ☺️

[swsnr]

@cpitclaudel As for the diff it's not up to date. I've got a private branch on top of current master with :stage and some other modifications. I just didn't get to update the PR, and given the modifications the initial post isn't up to date either.

I think I may just create a new pull request and close this one, but I haven't decided yet. I've got time to spare next weekend so I hope to deliver something by that time.

[swsnr]

Superseded by #1072.

[cpitclaudel]

Thanks for the convincing example @purcell, and thanks for the thoughtful answer @lunaryorn :)