[ RFC ] What changes would you like to make to the hhast-lint.json format?

[lexidor]

I'd like to rethink the hhast-lint.json config settings, since there are some short comings.
Here is just a short list of properties that I'd like to have, in no particular order.
This is not an issue that is meant to be implemented; it is meant to start a discussion.

Updating HHAST should not cause more linters to be activated (causing failing travis builds).

When upgrading hhast versions, linters that have been written for that version are automatically turned on. This makes upgrading hhast (which you are sometimes required to do, because of hhvm AST changes) a more painful process than it should be.

There should be more granularity in the built-in linters than 'none' | 'default' | 'all'

Currently all, doesn't mean all linters.
The only way to know that you are missing these linters is by digging into hhast and finding little gems like NoEmptyStatementsLinter or FinalOrAbstractClassLinter.
They might have been left out of the all option (NON_DEFAULT_LINTERS in the source) by mistake.

The default option currently excludes 8 linters, but it is not clear what makes a linter non-default.

The none option is made for people who require a stable output from their linters and who do not want and upgrade to introduce more linters that their code will fail under.
These people will list out every single linter they care about, one by one, and they will not be made aware of new and exiting linters.

A hhast-lint.json file that is valid in version B should be valid for version A, even if linters that are declared in the hhast-lint.json file are not present.

Currently, when you name a linter that does not exist, you get this error:
Expected type 'Facebook\HHAST\BaseLinter', got type 'string'
This stops HHAST dead in its tracks, since it is an uncaught exception.
I'd like for HHAST to report that linter X could not be found and continue linting with the rest of the linters.

I would be nice to be able to refer to groups of linters, instead of individual linters.

I'd like to be able to define Lexidor's pedantic linter pack: 1.4, Fully autofixable linters: *, Code style linters: 0.8 and have that be recognized by hhast.

I would like to introduce a new hhast-lint.lock file.
This would allow us to:

• Add new linters that would be enabled under the hhast-lint.json config, without having them pop-up upon upgrading hhast.
• Break BC in the hhast-lint.json format, without bricking every travis job, since hhast-lint.lock would still work under the newer version of hhast.
• Introduce a hhast-lint-update script, which will tell you which changes it is about to make.

[jjergus]

Updating HHAST should not cause more linters to be activated (causing failing travis builds).

I like this suggestion. Perhaps we could tag each new linter with the HHAST version in which it was added (or really any incremental number, since the HHAST version might not be known at the time a linter is added), then people could specify max linter version in their config. That way people could update HHAST itself without causing new lint errors, then later bump the linter version number if/when they're ready.

That said, it's probably not too hard to update HHAST + immediately add any new linters to disabledLinters until they're ready to deal with the errors.

There should be more granularity in the built-in linters than 'none' | 'default' | 'all'
I would be nice to be able to refer to groups of linters, instead of individual linters.

I'm not opposed to this but I can't imagine how we'd decide which linter goes in which group (like you said, right now it's unclear what should be default, if we have more groups it will become even more confusing).

I wonder if we should just have "all". I expect most people would have 1--2 linters they don't like but they are unlikely to be the same ones for everyone, so everyone'd just use disableLinters? (+ maybe the version number)

[fredemmott]

This discussion should be tied to (not merged with) #279 :)

[fredemmott]

Updating HHAST should not cause more linters to be activated (causing failing travis builds).

I strongly disagree with this for all; default, I need to think about some more - my initial stance was that we match HHVM versions, and this would hold us to a higher standard than HHVM and Hack, which doesn't feel appropriate.

One of the proposed alternatives to 'all' and 'default' could be better

it is not clear what makes a linter non-default.

Definitely; initially, it was "is something like this needed for FB projects", but it's drifted and pretty arbitrary now.

Currently all, doesn't mean all linters.

I believe this is currently always a bug; the exception I expect at some point is completely contradictory linters - e.g. "always use \ , even in XHP" and "Use : for XHP wherever possible"

There should be more granularity in the built-in linters than 'none' | 'default' | 'all'

Perhaps get rid of default, add all-x.yy ?

The none option is made for people who require a stable output from their linters

This is a valid use case for it, but FWIW, it's made for people who want to gradually start linting their codebase, moving to all or default over time.

I would be nice to be able to refer to groups of linters, instead of individual linters.

#13 :)

That said, now the AST's moving again, I'm not sure how viable third-party repositories of linters are, and this impacts the 'default' vs 'all' discussion:

if we strongly disagree with the intent of a linter, but know some people really want it, it's well-written, should we accept or reject the pull request? Should it be included in whatever our "recommended" set is, whether that's "default" or "all-x.yy" ? Practicality of third-party repositories does change this, but the need for new codegen can be a pain.

I'm going to add another one of these:

Should HHAST be split up in to hhast-lib,hhast-tools, and hhast-linters projects?

• keep hhast-lib versioning matching HHVM versions
• hhast-tools and hhast-linters could use SEMVER
• hhast-tools contains the executables and linting framework, but no linters
• changes that only affect the codegen - but not the majority of linters - would only affect hhast-lib
• change the way config works so that hhast-linters don't have any special-casing - third-party linters are on truly equal footing
• projects could depend on 'any hhast-lib that is compatible with my hhvm', hast-tools a.b, and hhast-linteres c.d

There's a risk of that being substantially more maintenance work, but I think that's my gut reaction - thinking through the last few months, I don't think that would actually be true in practice - @jjergus ?

We could continue publishing an 'hhast' package, that would just depend on all 3, but contain no actual code

[fredemmott]

More specifically:

• hhvm/hhast-lib
• hhvm/hhast-tools
• facebook/hhast-linters

[fredemmott]

... and none of the version numbers - even majors - need to match.

[fredemmott]

In that world:

• schema changes
  • new hhvm/hhast-lib release
• we change what is codegened from the schema
  • new hhvm/hhast-lib release
  • probably not a new hhvm/hhast-tools release
  • maybe new releases of any linter packages, depending on what nodes/features they use.
• we add a new default/all linter
  • new major release of facebook/hhast-linters
  • nothing else
• we change what is acceptable in hhast-lint.json
  • new major release of hhvm/hhast-tools
  • nothing else

[fredemmott]

@xixixao also likely has some input here, and #279 more generally (and has several other issues that are related to #279)

[fredemmott]

So, regardless, we're going to end up with some concept of "we recommend this set/configuration for new projects if you don't want to whitelist specific ones"; it's basically the same problem whether that means:

• 'all' vs 'default'
• in facebook/hhast-linters vs "put it in a non-FB repo"

I don't think there's a good answer here: linters are not for soundness, they are for enforcing stylistic choices, which are fundamentally subjective. Even the ones that exist pretty much to highlight common mistakes have a counterpoint of "(I know what I'm doing|we idiomatically use them in a safe way), and we like writing code that way".

The FB internal approach is "anyone can turn it on, and if there's too much push-back it gets disabled/limited", which won't work here.

If we split out the linters to a separate package under facebook/ , perhaps "this matches FB coding standards and would be an appropriate linter at FB" is the correct separator; problems are:

• that doesn't feel appropriate while this is a monorepo
• what about the other ones that are already here? One option (that I don't really like, but can't think of anythign better) would be:
  • no new ones that don't enforce part of FB coding styles - they need to go to third-party repos
  • existing ones are 'grandfathered in', but not enabled by default - need an explicit opt-in to get these enabled
  • if someone stops being in FB coding styles, it joins 'off-by-default' like the current ones

would be an appropriate linter at FB

This has non-style factors, such as how much 'good' code it complains about, and whether it's autofixable

[jjergus]

There's a risk of that being substantially more maintenance work, but I think that's my gut reaction - thinking through the last few months, I don't think that would actually be true in practice - @jjergus ?

There's definitely some extra maintenance work for each project right now (e.g. an AST change in HHVM requires new HHAST release + potentially a new release of anything else that depends on HHAST like definition-finder, which would now include hhast-tools and hhast-linter).

If we proceed with the plan to merge some projects into a "monorepo" that we've been discussing, that would make it easier (assuming we have good automation to publish a release of each project from the monorepo).

[fredemmott]

Fair - I"m hoping that "just update hhast-lib without enabling more linters" in downstream projects would offset that

[lexidor]

Splitting hhast up into linters, tooling, and core would be the right thing to do for projects that want to depend on hhast.
Most of those libraries that rely on hhast want access to the AST, but not to the linters.

I am torn on the maintainability question.
If we decide in favor of this, it becomes painful to add methods onto Node classes that you want to use in a new linter.
ExampleQualifiedName::isFullyQualified() would require a release of hhast-lib, and hhast-linters would require that fresh hhast-lib version.

This discourages adding things like this into hhast-lib and may lead to people placing functions like this as free standing functions into hhast-linters instead to reduce dependency hell.

[azjezz]

One thing I would like to note, since HHAST liners are a "tool" ( not a library to use or build upon ), its okay to have internal BC-breaks, such as changing class names or architecture structure. however, this shouldn't affect projects that disable linters.

therefore, I suggest that each linter now implements a new method getName() and hhast recommend using that in the configuration files when referring to a linter instead of FQN.

[fredemmott]

Some form of name spacing is desired so that third party linters can be created independently of each other without risk of conflict with other linters, bundles or not.

Having this be distinct to the FQN feels like solving the same problem twice

For conciseness, it does support a configuration equivalent to “use namespace”

[azjezz]

Some form of namespacing is desired so that third party linters can be created independently of each other without the risk of conflict with other linters, bundles or not.

the linter name itself can be prefixed, e.g: facebook_async_gen_prefix, nuxed_camel_case_enum_members.

with official linters ( e.i ones shipped with hhast ), not using prefix, e.g: await_in_a_loop