Replace JSON config with Hack configuration

[fredemmott]

Copying from #287 (comment)

--

There's a suggestion to replace the JSON configuration files with executable Hack configurations. This would:

• give more flexibility
• allow typechecker insight (e.g. to verify classnames)
• provide an obvious solution to various extensibility problems ("do what you do for Hack code, because it is Hack code")
• allow comments
• allow re-use of configurations between projects

--

For context: we use plain data configuration because originally the configuration was much simpler: you just provided a path.

We specifically use JSON because it's the best data format with builtin serialization/deserialization support.

[fredemmott]

@azjezz has the concrete suggestion of generating executables. I like this overall.

My only major concern is that I don't want hhast to depend on hack-codegen (circular dependencies are a pain, even if some of the links are dev-dependencies). Not a blocker, just means nasty hand-written codegen.

[Atry]

If the purpose of code generation is to create just some boilerplate code expected to be manually modified, we can let the IDE do such work, as simple as copying a template to the code base, instead of using hack-codegen.

[fredemmott]

as copying a template to the code base

👍

we can let the IDE do such work

I'm increasingly an IDE fan, but I believe in strong layering here: IDE should be a convenience. We should design flows to be as usable as possible without an IDE first, then figure out if IDEs even need special handling

[lexidor]

@fredemmott rereading the discussions here and the circular dependency thing / handwritten codegen dichotomy made me rethink the approach taken. Why does the codegen need to live in the same repository as the linters. If the codegen would live in facebookorhhast/hhast-linter-config this would break the cycle. Hhast would not depend on either hack-codegen or hhast-linter-config. Hack-codegen dev-depends on hhast. Hhast-linter-config would depend on hhast and hack-codegen. Circular dependencies averted and no handwritten codegen. Having cake and eating it too.

[lexidor]

allow typechecker insight (e.g. to verify classnames)

This is a double edged sword. Yes, it is great to have the typechecker check that you didn't typo a classname. But this causes a BC headache for libraries that have a large hhvm version support going. Enabling a linter that only exists on 4.145+ (f.e. HHClientLinter) requires mentioning it by name. This effectively forces the library to drop support for older hhvm versions. If the configuration was outside of the view of the typechecker, one would be able to mention a linter by name, without requiring that this linter be present on older builds.

[azjezz]

There's a way to actually avoid this, more of a "hack".

instead of referencing linters by class name, we refer to them by name, e.g 'comapny.camel_case_enum_members'.

We then add a static method to all linters named "getName(): string", which returns a unique string that refers to that linter.

Hhast would get all declared childrens of the Linter interface, and compare the name provided by the getName() to see which linters to use.

If a provided name doesn't exist, hhast would throw an exception by default, but we can add a configuration entry that tells hhast not to throw, and skip it instead, in which case, hhast will simply log the fact that it skipped a linter because it doesn't exist.

This way, you can refer to linters that don't exist yet, or that are known to exist in previous versions, without worrying about it.

This is not useful to many, but i can see how it can be useful for OSS that want to support multiple hhast versions.

Wdyt @fredemmott

[lexidor]

If names have to be unique anyway, why not accept the string representation of the fqn as well as the actual classname. The BC folks can spell out "Facebook\HHAST\HHClientLinter" and everyone else can use ::class instead. A mechanism of stating these classnames may not exist, they are not a typo would be great to have in general.

[azjezz]

Ah, that could work too, the setLinters() ( or whatever name it would go by ), would then accept a string instead of a classname, and hhast compares the classnames, meaning no need for "getName()" method.

[fredemmott]

I strongly feel that we should take classnames by default (or otherwise require fully typechecked code) - but, a separate, explicit useLinterIfAvailableAtRuntime(string) (or json setting) would make sense to me.

We should encourage fixed configurations that don't silently run less stuff - but we providing an escape hatch makes sense.

[lexidor]

If we end up going with the Hack code that instructs a codegen to build out a new EntryPoint way of configuring hhast, will the codegenned EntryPoint be mostly plain old data or will this EntryPoint evaluate a lot of predicates? I would be in favor of having the EntryPoint be the equivalent of a lock file with loads of plain old data.