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

Add a test to detect duplicated lint rules #427

Merged
merged 5 commits into from
Dec 10, 2021
Merged

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Dec 9, 2021

HHClientDuplicatedLintErrorTest ensures that no lint error is found by HHClientLinter in examples for SingleRuleLinters, closing #409.

@Atry Atry marked this pull request as draft December 9, 2021 23:48
@Atry Atry marked this pull request as ready for review December 9, 2021 23:55
@Atry Atry requested a review from fredemmott December 9, 2021 23:55
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.

It's fine to have two errors for the same node, just as long as they're not talking about the same problem - e.g. await-in-a-loop with an always-false condition is a valid combination.

I think you're already handling this with the HHAST_IGNORE_ALL in the tests, but just wanted to make sure?

If so, it would be good to add an explanation comment to the test case itself about this, and saying use HHAST_IGNORE_ALL in tests to suppress false positives

@@ -6,6 +6,7 @@
* LICENSE file in the root directory of this source tree.
*
*/
// HHAST_IGNORE_ALL[0] because the error code is not the interested in this example
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we raising 0, or does this have a magic meaning?

Copy link
Contributor Author

@Atry Atry Dec 10, 2021

Choose a reason for hiding this comment

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

It means this file does not type check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it have this meaning? hh_client doesn't pay attention to it, and it's not really intuitive.

Does this have special meaning to HHClientLinter, affecting non-lint errors? If so:

  • this behavior should probably be removed
  • HHClientLinter should probably always ignore any errors out of the 5xxx range:
    • all linters are in the 5xxx range
    • 1xxx-4xxx should be handled via /* HH_FIXME */ + hh_client - they shouldn't affect hhast at all (beyond whether or not hhast can parse the file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means a bug in the OCaml linter. facebook/hhvm#8950

Copy link
Contributor

@fredemmott fredemmott Dec 10, 2021

Choose a reason for hiding this comment

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

IMO we should not report this as a lint error, and we should not require suppression - similarly to how we skip files that hh_parse can't deal with - unless the specific file is requested.

@Atry
Copy link
Contributor Author

Atry commented Dec 10, 2021

It's fine to have two errors for the same node, just as long as they're not talking about the same problem - e.g. await-in-a-loop with an always-false condition is a valid combination.

I think you're already handling this with the HHAST_IGNORE_ALL in the tests, but just wanted to make sure?

If so, it would be good to add an explanation comment to the test case itself about this, and saying use HHAST_IGNORE_ALL in tests to suppress false positives

Comments added in b94f654

@Atry Atry merged commit bf471e2 into hhvm:main Dec 10, 2021
@Atry Atry deleted the duplicate-lint-rules branch December 10, 2021 21:22
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