Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Add HHClientLinter #368

Closed
wants to merge 0 commits into from
Closed

Add HHClientLinter #368

wants to merge 0 commits into from

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Nov 4, 2021

This is the initial implementation of HHClientLinter, which is a proxy calling hh_client --lint.

Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • test failure: either use a lint for the test that exists in all supported versions, or increase the minimum version (grep for the old one, probably composer.json and .github/workflows/*)
  • needs adding to LintRunConfig::NON_DEFAULT_LINTERS so it's include in all runs.
    • this will make it so that it's include in HHAST's own runs; probably best to exclude this linter explicitly in hhast-lint.json for now, then fix them up separately

tests/HHClientLinterTest.hack Outdated Show resolved Hide resolved
src/Linters/HHClientLintError.hack Outdated Show resolved Hide resolved

final class HHClientLinter extends BaseLinter {

const type TConfig = shape();
Copy link
Contributor

@fredemmott fredemmott Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The active lints need to be configurable; perhaps:

shape(
  ?'ignore_except' => keyset<int>,
  ?'ignore' => keyset<int>,
 )

Both would default to empty; if something is present in both, that should be a runtime error.

Additionally, there should probably be a hardcoded list of lints to always ignore, e.g. 5583 - await in a loop - is known buggy, and has a pure Hack implementation in HHAST and Facebook www already. The current list is HackLintRule::HACK_LINT_IGNORED_CODES in FB WWW, mostly being known-bad linters that should be fixed at some point, but for 5583/AWAIT_IN_LOOP, this is permanently replaced by the linters written in Hack so blocking it here (and in FB) is a band aid - the right fix is probably to delete that one from the ocaml code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will implement the config in another PR as this PR is already too large

@Atry Atry force-pushed the hh-client-linter branch 2 times, most recently from 341cb0d to d9de021 Compare November 4, 2021 17:06
src/Linters/HHClientLintError.hack Outdated Show resolved Hide resolved
@Atry Atry mentioned this pull request Nov 4, 2021
@Atry Atry force-pushed the hh-client-linter branch 7 times, most recently from ba00fa8 to ff1784d Compare November 5, 2021 01:39
@Atry Atry marked this pull request as draft November 5, 2021 03:21
@Atry Atry self-assigned this Nov 5, 2021
@Atry Atry marked this pull request as ready for review November 10, 2021 01:24
@Atry Atry requested a review from fredemmott November 10, 2021 01:27
@Atry Atry force-pushed the hh-client-linter branch 2 times, most recently from af9e9b7 to 1267b3f Compare November 10, 2021 01:55
@Atry
Copy link
Contributor Author

Atry commented Nov 10, 2021

  • test failure: either use a lint for the test that exists in all supported versions, or increase the minimum version (grep for the old one, probably composer.json and .github/workflows/*)

Fixed

  • needs adding to LintRunConfig::NON_DEFAULT_LINTERS so it's include in all runs.

    • this will make it so that it's include in HHAST's own runs; probably best to exclude this linter explicitly in hhast-lint.json for now, then fix them up separately

Will be addressed in another PR

Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note followup PRs (to add to non-default linters, and the configurability) are required before this can actually be used outside of unit tests.

return $this->getBlameCode();
}

public function getLintRule(): LintRule {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be memoized or passed in to the constructor given that it's never going to change for a particular instance

Copy link
Contributor Author

@Atry Atry Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, Hotspot JVM is good at optimizing allocation for short-lived objects like this (when it is not memoized). Is HHVM able to eliminate the allocation like JVM does?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not familiar enough with either VM:)

src/Linters/HHClientLintError.hack Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants