KEMBAR78
Fix some shortcomings of UnusedParameterLinter by lexidor · Pull Request #506 · hhvm/hhast · GitHub
Skip to content
This repository was archived by the owner on Dec 1, 2024. It is now read-only.

Conversation

lexidor
Copy link
Contributor

@lexidor lexidor commented Aug 11, 2022

  • Variadic parameters were not checked
    function _(int ... $x): void {} now emits an error for $x.
  • Lambda parameters are now linted in unparenthesized params.
    $x ==> 0 now emits an error for $x.
    Same applies for (int ... $x) ==> 0.
  • Added explicit tests for old style variadics.
    The linter handled them fine, but there was no test for it.

Checking for unparenthesized lambda parameters required a new linter.
The UnusedParameterLinter checked against ParameterDeclaration.
$x ==> 0; does not introduce one, so a new TNode was required.

This PR may cause old HHAST_FIXME comments to break.
You will have to upgrade:
/*HHAST_FIXME[UnusedParameter]*/ ($x) ==> 0; to
/*HHAST_FIXME[UnusedLambdaParameter]*/ ($x) ==> 0;
I suspect this linter is rarely suppressed, since the autofix is easier.

 - Variadic parameters were not checked
   function _(int ... $x): void {} now emits an error for $x.
- Lambda parameters are now linted in unparenthesized params.
   $x ==> 0 now emits an error for $x.
   Same applies for (int ... $x) ==> 0.
 - Added explicit tests for old style variadics.
   The linter handled them fine, but there was no test for it.
Checking for unparenthesized lambda parameters required a new linter.
The UnusedParameterLinter checked against ParameterDeclaration.
$x ==> 0; does not introduce one, so a new TNode was required.
This PR may cause old HHAST_FIXME comments to break.
You will have to upgrade:
/*HHAST_FIXME[UnusedParameter]*/ ($x) ==> 0; to
/*HHAST_FIXME[UnusedLambdaParameter]*/ ($x) ==> 0;
I suspect this linter is rarely suppressed, since the autofix is easier.
@Atry
Copy link
Contributor

Atry commented Aug 11, 2022

Looks good to me.

@Atry Atry merged commit 0b8dee8 into hhvm:main Aug 11, 2022
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