hhvm / hhast Goto Github PK

We recommend concatenation (or sprintf) instead; this could be an autofixing linter.

Should check:

• "No Errors" only appears when there were no errors - having had autofixes doesn't get that
• exit code is 0 if there were no errors, or all errors were autofixed
• exit code is not 0 if there were unfixed errors

These currently execute hh_client for every file - we only need to do it once per codebase, and we don't need to traverse the file tree at all.

Some linters have unsafe autofixes, for example, the 'basic assignment' linter also detects accidental invariant($foo = $bar, ...)

Solve issues such as: facebookarchive/fbctf#576

It should take a fully qualified function/method name (e.g. foo(), Foo\bar, Foo::bar(), Foo\Bar::baz()) as input. It should then:

• make the function async
• make the return type awaitable
• replace any calls to \HH\Asio\join() with await in simple cases. Replace with easily-greppable wrapper (maybe add \HHAST\PartiallyMigrated_FIXME\Asio\join() ?
• do the same to the same method in superclasses/interfaces/subclasses/other implementations

If windows newlines are present, the linter should suggest that they're replaced with unix newlines.

There's two ways to do this:

• as an AST linter, looking at each node's first token's leading whitespace, and last token's trailing whitespace
• making a new 'line based linter' which doesn't use the AST at all, but does use the existing linter base classes. This could also be useful for #3

Either way, please make sure the CLI output is clear when running the linter, both in your terminal, and non-interactively (e.g. when redirecting to a file with > /tmp/somefile)

e.g.

use Foo\Bar;

function foo(): Bar {};

... should be autofixed to use type

Should not be on by default, but should be on when linters = all

Prefer single quoted strings. Exceptions:

• if the string contains quotation marks
• if the string contains interpolation
• if the string contains escape sequences (e.g. "\n")

3.24's schema contains backwards-incompatible changes. We need to either:

• drop support for 3.23 and below
• build a way to hide the differences

We probably also should build support for verifying that hh_parse --schema returns a recognized schema version string.

There may still be changes - current diff from 3.23 is:

https://gist.github.com/fredemmott/85632d66bc09413e00ebf8710e5fa0af

Some highlights:

-        { "field_name" : "visibility" },
+        { "field_name" : "modifiers" },

-    { "kind_name" : "XHPAttribute",
...
+    { "kind_name" : "XHPSimpleAttribute",
...
+    { "kind_name" : "XHPSpreadAttribute",

-        { "field_name" : "parameter_types" },
+        { "field_name" : "parameter_list" },

If the path is passed, it will validate everything in that path, ignoring the contents of hhast-lint.json

For example, this will lint vendor/

• all methods and properties (including static methods) should be lowerCamelCase
• all function and local variables should be snake_case

Promoted constructor arguments should be lowerCamelCase

This was accidentally permitted in 3.26 with the parser cutover, and we expect to drop support in 3.27

This should be (new Foo())->bar()

It would be best to:

• add an autofixing linter for this
• add a migration for this (possibly adding a new LinterPoweredMigration base class or similar)

This should ban:

• $x = [];
• $y = array();

etc

There are exceptions, but this is a good general rule.

The NullLiteralToken always uses null for the text, even if the source was using a different case. This means that these all get changed when running a migration, even though it's not related to the migration.

This does not happen with true/false

$ cat test.php
<?hh

NULL;
TRUE;
FALSE;
$ bin/hhast-migrate --optional-shape-fields test.php
$ cat test.php
<?hh

null;
TRUE;
FALSE;

Autofixable: fixer could use hh_client --search --json to find out if use type is appropriate; if not, fall back to use namespace. Not 100% accurate, but probably close enough.

e.g:

$a = await foo();
$b = await bar();

should suggest autofix:

list($a, $b) = await \HH\Asio\va(foo(), bar());

... but this is fine:

$a = await foo();
$b = await bar($a->getFoo());

This option will be supported in HHVM 3.23+; it requires that callsites specify '&' when passing an argument by ref; for example:

$foo = vec[1,3,2];
sort(&$foo);

.hhconfig:

enable_experimental_tc_features=safe_pass_by_ref

With the option off, in 3.23+, the '&' is ignored - but it is an error in master. This can be worked on now if you build HHVM master, or brew tap hhvm/hhvm; brew install hhvm-preview if working on a mac.

This requires building support for migrations that query and fix typechecker errors, rather than purely operating on the AST

It's somewhat more convenient than actually having to construct a replacement AST for simple fixes/migrations. This should use a manually created EditableNode subclass with no children, and user-specifiable text.

The main downside is that it doesn't stack correctly - if a later migration step needs to modify the same code, it will fail. This should be covered in the documentation.

e.g.:

use type Herp\Derp;
use type Foo\Bar;
use type Foo\Baz;

should become:

use type Herp\Derp;
use type Foo\{
  Bar,
  Baz,
};

cc @PranayAgarwal

This would allow updating HHAST independently of HHVM versions

e.g.

"overrides": [
  {
    "patterns": [ "tests/data/*" ],
    "disableAllLinters": true
  }
]

For example, if vendor/hh_autoload.php is changed, it shouldn't be linted

return new self($a != $b);

This raises a NoPHPEquality lint error - but I can't find any place to insert a HHAST_IGNORE_ERROR that suppresses it.

If there is whitespace at the end of a line, it should be removed.

There's two ways to do this:

• as an AST linter, looking at each node's first token's leading whitespace, and last token's trailing whitespace
• making a new 'line based linter' which doesn't use the AST at all, but does use the existing linter base classes. This could also be useful for #2

Line-based is 'good enough' here, though if you use the AST, you could improve on this by excluding HEREDOCs. Please don't handle HEREDOCs or other edge cases if you do a line-based linter.

Either way, please make sure the CLI output is clear when running the linter, both in your terminal, and non-interactively (e.g. when redirecting to a file with > /tmp/somefile)

For example, the __Override attribute operates on MethodishDeclarations, but should really raise the error against the the decl header, not the entire method.

This matters more now that the LSP/JSON interfaces report ranges.

This is a more opinionated one: it hurts greppability and readability.

Tested on HipHop VM 3.22.0 (rel)

composer require facebook/definition-finder
bin/hhast-migrate --hhvm-3.22-to-3.23 vendor/facebook/definition-finder

Error

Fatal error: Uncaught exception 'Exception' with message 'unexpected JSON kind: extra_token_error' in hhast/codegen/editable_node_from_json.php:371
Stack trace:
#0 hhast/src/EditableNode.php(129): Facebook\HHAST\__Private\editable_node_from_json()
#1 hhast/src/EditableToken.php(145): Facebook\HHAST\EditableNode::fromJSON()
#2 hhast/src/__Private/fold_map.php(24): Closure$Facebook\HHAST\EditableToken::fromJSON()
#3 hhast/src/EditableToken.php(148): Facebook\HHAST\__Private\fold_map()
#4 hhast/codegen/editable_node_from_json.php(21): Facebook\HHAST\EditableToken::fromJSON()
#5 hhast/src/EditableNode.php(129): Facebook\HHAST\__Private\editable_node_from_json()
#6 hhast/codegen/syntax/FunctionCallExpression.php(58): Facebook\HHAST\EditableNode::fromJSON()
#7 hhast/codegen/editable_node_from_json.php(241): Facebook\HHAST\FunctionCallExpression::fromJSON()
#8 hhast/src/EditableNode.php(129): Facebook\HHAST\__Private\editable_node_from_json()
#9 hhast/codegen/syntax/ExpressionStatement.php(34): Facebook\HHAST\EditableNode::fromJSON()
#10 hhast/codegen/editable_node_from_json.php(132): Facebook\HHAST\ExpressionStatement::fromJSON()
#11 hhast/src/EditableNode.php(129): Facebook\HHAST\__Private\editable_node_from_json()
#12 hhast/src/EditableList.php(92): Facebook\HHAST\EditableNode::fromJSON()
#13 hhast/codegen/editable_node_from_json.php(23): Facebook\HHAST\EditableList::fromJSON()
#14 hhast/src/EditableNode.php(129): Facebook\HHAST\__Private\editable_node_from_json()
#15 hhast/codegen/syntax/Script.php(29): Facebook\HHAST\EditableNode::fromJSON()
#16 hhast/codegen/editable_node_from_json.php(47): Facebook\HHAST\Script::fromJSON()
#17 hhast/src/EditableNode.php(129): Facebook\HHAST\__Private\editable_node_from_json()
#18 hhast/src/entrypoints.php(20): Facebook\HHAST\EditableNode::fromJSON()
#19 hhast/src/entrypoints.php(36): Facebook\HHAST\from_json()
#20 hhast/src/__Private/MigrationCLI.php(57): Facebook\HHAST\from_file()
#21 hhast/src/__Private/MigrationCLI.php(74): Facebook\HHAST\__Private\MigrationCLI->migrateFile()
#22 hhast/src/__Private/MigrationCLI.php(98): Facebook\HHAST\__Private\MigrationCLI->migrateDirectory()
#23 hhast/bin/hhast-migrate(18): Facebook\HHAST\__Private\MigrationCLI->mainAsync()
#24 {main}

By default, hhast-migrate --hhvm-3.22-to-3.23 migrates the vendor directory as well.
facebook/definition-finder is used by hhvm/hhvm-autoload so a many projects might be affected.

Probably want to publish CodeActions, that send a workplace/applyEdit

Report the file causing them.

For example, right now, running AsyncFunctionAndMethodLinter against hhvm/definition-finder throws an exception, but it doesn't say what file causes it

This would be a completely new feature; it would make it much easier to debug and work on HHAST-powered tools - replacing workflows such as hh_parse --full-fidelity-json foo.php | jq

This could possibly tie into hhvm/fbmarkdown@2

Probably worth doing given hackfmt --diff in 3.23 removes them :(

EditableList is always marked as returning EditableNodes. The inference process could refine this.

This is probably not a good first issue as the relationship inference process is extremely slow

for example, if you have HHAST's tree open in your IDE, and run the tests, the IDE starts showing lint errors for the .out files

Replace $this->assertEquals() and similar with expect($a)->tobePHPEqual($b)

In addition to being a pre-requisite to replacing PHPUnit, this solves real issues in tests introduced by Hack array migration: assertEquals() is always true if both sides are vecs for example, even if the vecs have completely different elements.

This should be a multi-step migration (see https://github.com/hhvm/hhast/blob/master/src/Migrations/ImplicitShapeSubtypesMigration.php for an example). It should:

1. add use function Facebook\FBExpect\expect if it isn't already present. This should be (in preference order, depending on the file)
   • next to existing use function if present
   • next to other use declarations if present
   • immediately after a namespace declaration (or just inside the block) if there is one
   • at the top of the file, after any leading comments that aren't docblock comments (copyright notices are a common example)
2. change understood assertions (i.e. only change assertFoo if we know that expect has a replacement for Foo, not just everything that matches that pattern)

This shouldn't change the base class from PHPUnit.

I've been developing with 3.23.2 locally but we've finally rolled out 3.21.3 on the boxes where our linter is run. I've been trying to install both hhast and our repo that includes hhast as a dep, but get the following when trying to install:

Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for facebook/definition-finder v1.6.4 -> satisfiable by facebook/definition-finder[v1.6.4].
    - facebook/definition-finder v1.6.4 requires hhvm ~3.23 -> your HHVM version does not satisfy that requirement.
  Problem 2
    - Installation request for facebook/hack-codegen v3.0.3 -> satisfiable by facebook/hack-codegen[v3.0.3].
    - facebook/hack-codegen v3.0.3 requires hhvm >=3.23.0 -> your HHVM version does not satisfy that requirement.
  Problem 3
    - Installation request for hhvm/hhvm-autoload v1.5.4 -> satisfiable by hhvm/hhvm-autoload[v1.5.4].
    - hhvm/hhvm-autoload v1.5.4 requires hhvm ^3.23 -> your HHVM version does not satisfy that requirement.
  Problem 4
    - Installation request for hhvm/type-assert v3.2.0 -> satisfiable by hhvm/type-assert[v3.2.0].
    - hhvm/type-assert v3.2.0 requires hhvm ^3.23.0 -> your HHVM version does not satisfy that requirement.
  Problem 5
    - Installation request for 91carriage/phpunit-hhi 5.7.2 -> satisfiable by 91carriage/phpunit-hhi[5.7.2].
    - 91carriage/phpunit-hhi 5.7.2 requires hhvm >=3.23.0 -> your HHVM version does not satisfy that requirement.
  Problem 6
    - hhvm/hhvm-autoload v1.5.4 requires hhvm ^3.23 -> your HHVM version does not satisfy that requirement.
    - hhvm/hsl v1.2.0 requires hhvm/hhvm-autoload ^1.4 -> satisfiable by hhvm/hhvm-autoload[v1.5.4].
    - Installation request for hhvm/hsl v1.2.0 -> satisfiable by hhvm/hsl[v1.2.0].

I know 3.21.3 probably isn't fun to support, so I've tried to fork and figure out a set of deps that does work up until v0.5 and got this far:

{
    "name": "hhvm/hhast",
    "bin": [ "bin/hhast-lint" ],
    "require-dev": {
        "facebook/hack-codegen": "=3.0.1",
        "phpunit/phpunit": "^5.7",
        "91carriage/phpunit-hhi": "=5.7.1",
        "facebook/fbexpect": "^0.3.0",
        "hhvm/hhvm-autoload": "=1.5.3",
        "facebook/definition-finder": "=1.6.3"
    },
    "require": {
        "hhvm": "^3.21.0",
        "hhvm/hsl": "=1.1.0",
        "hhvm/type-assert": "=3.1.0"
    }
}

but when I try to get hhast to lint itself:

bradley@slack:~/hhast$ hhvm bin/hhast-lint .

Warning: Destructor threw an object exception: exception 'HH\InvariantException' with message 'counter Facebook\HHAST\__Private\LinterCLI destroyed without calling ::end()' in /home/bradley/hhast/src/__Private/PerfCounter.php:51
Stack trace:
#0 /home/bradley/hhast/src/__Private/PerfCounter.php(51): HH\invariant_violation()
#1 /home/bradley/hhast/src/__Private/LinterCLI.php(155): Facebook\HHAST\__Private\PerfCounter->__destruct()
#2 /home/bradley/hhast/bin/hhast-lint(38): Facebook\HHAST\__Private\LinterCLI::mainAsync()
#3 {main} in /home/bradley/hhast/src/__Private/LinterCLI.php on line 155

Fatal error: Uncaught exception 'Exception' with message 'Invalid configuration file: /home/bradley/hhast/hhast-lint.json
  Expected type 'vec<T>', got type 'array'
Type trace:
#0 shape[roots]
' in /home/bradley/hhast/src/__Private/LinterCLIConfig.php:243
Stack trace:
#0 /home/bradley/hhast/src/__Private/LinterCLIConfig.php(97): Facebook\HHAST\__Private\LinterCLIConfig::getConfigFromFile()
#1 /home/bradley/hhast/src/__Private/LinterCLIConfig.php(98): Facebook\HHAST\__Private\LinterCLIConfig::getFromConfigFile$memoize_impl()
#2 /home/bradley/hhast/src/__Private/LinterCLIConfig.php(121): Facebook\HHAST\__Private\LinterCLIConfig::getFromConfigFile()
#3 /home/bradley/hhast/src/__Private/LinterCLIConfig.php(127): Facebook\HHAST\__Private\LinterCLIConfig::getForPathImpl$memoize_impl()
#4 /home/bradley/hhast/src/__Private/LinterCLIConfig.php(109): Facebook\HHAST\__Private\LinterCLIConfig::getForPathImpl()
#5 /home/bradley/hhast/src/__Private/LinterCLI.php(138): Facebook\HHAST\__Private\LinterCLIConfig::getForPath()
#6 /home/bradley/hhast/bin/hhast-lint(38): Facebook\HHAST\__Private\LinterCLI->mainAsync()
#7 {main}

at this point I'm not sure what else to try. Any suggestions?

hhast-lint currently produces user readable output for lint errors. It would be helpful to support a --json option (similar to hh_client) to make this parsable by IDEs and other automated tooling.

• These should all be separate linters, except for UNSAFE and UNSAFE_BLOCK
• UNSAFE_EXPR is better than UNSAFE and UNSAFE_EXPR but not as good as FIXME|IGNORE_ERROR
• using HH_IGNORE_ERROR[1002] or HH_FIXME[1002] suppresses parse errors, so gives you undefined behavior for the rest of the file. The usual reason for ignoring 1002 is pseudomain code in a strict file; the linter should suggest moving most of the code to a separate file, and making the file partial instead

With the HSL, we're establishing a new convention for async functions (there's currently none):

• async functions should end with either _async or _asyncx
• async methods (both instance methods and static methods) should end with Async or Asyncx
• should ignore functions that start with 'test'

This should be an AST-based linter, but should not support autofixing (as all the callsites would need updating too). It should use https://github.com/hhvm/hhast/blob/master/src/Linters/FunctionNamingLinterTrait.php -
https://github.com/hhvm/hhast/blob/master/src/Linters/CamelCasedMethodsUnderscoredFunctionsLinter.php may be a useful reference for this.

https://github.com/hhvm/hhast/blob/master/src/__Private/CLIBase.php

It's normal to be able to combine multiple short options into a single one - for example, -a -b should be equivalent to -ab.

Complications:

• this shouldn't be true for short options which require an argument - for example, if b needs an option, -a -b foo is required - not -abc foo or -ab foo
• this will require banning multi-character short options - you can do this in the CLIOption base class with an invariant
• that means that -vv will need to be removed from bin/hhast-lint; this can be done by changing -v to increment $verbosity, instead of setting it to 1

Large repositories with lots of custom rules aren't going to finish linting in a reasonable amount of time, so LSP mode will need an option to not start a lint run on the entire repository at launch, but rather:

• Lint individual files on open & save
• Remove diagnostics for a file when it is closed

This is fairly common...

foo($bar = '123', $baz = '456');

... and has two main problems:

• it's often done with the assumption that it's a real named parameter
• it is often unexpected that it sets a local in the containing scope

This could be an autofixing-linter, suggesting this instead:

foo(/* bar = */ '123', /* baz = */ '456');

It would be good for users to opt-in to a full suite of linters from a third-party, rather than having to whitelist every specific linter - for example, if a framework decides to ship linters for its' users

This is less of an "issue" and more of a question:

Is there a good way to stub or mock the shouldLintFile of linters in tests?

The canonical linters are all marked final, and even if they weren't PHPUnit's createMock() appears to not handle Hack which is understandable. Any advice would be greatly appreciated, as we have linters that only run on files whose names match regexs but would like to be able to have test coverage. Thanks!

This should:

• take a git or hg diff on STDIN (if you don't use hg, a PR adding just git support is very welcome)
• run the linters on every file that the diff touched
• not error out if the diff deleted a file

https://github.com/hhvm/hhast/blob/master/src/__Private/LinterCLI.php is where you'll need to hook in - but please don't add the diff parsing code there directly. My instinct would be to add a get_filenames_from_diff(string) function, and use that in LinterCLI, but I'm open to other approaches.