Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UnusedPipeVariableLinter #459

Merged
merged 5 commits into from
May 16, 2022
Merged

Conversation

ryangreenberg
Copy link
Contributor

This PR introduces UnusedPipeVariableLinter to identify bugs where the pipe variable $$ is missing from a pipe operation.

I recently encountered a bug in a pipe operation where the right-hand side re-used the initial variable, discarding the intended input in $$:

$output = htmlspecialchars($html) |> format($html);

Note that the docs for "Expressions And Operators: Pipe" say:

The binary pipe operator, |>, evaluates the result of a left-hand expression and stores the result in $$, the pre-defined pipe variable. The right-hand expression must contain at least one occurrence of $$.

If it's a bug that the typechecker does not produce an error for a missing $$, we can close this PR and file an issue in https://github.com/facebook/hhvm.

@lexidor
Copy link
Contributor

lexidor commented May 3, 2022

If it's a bug that the typechecker does not produce an error for a missing $$, we can close this PR and file an issue in https://github.com/facebook/hhvm.

facebook/hhvm#8698

There is already an issue tracking this 😉.
Please don't let this convince you to withdraw this linter. Pipes without $$ are fishy it should be flagged. A lint rule could be deployed far quicker than a language change. I would be surprised if www would typecheck after 2.5 years since the unintentional (?) removal of the check. Strongly in favor of shipping this linter, even if it becomes a language level error later.

@ryangreenberg
Copy link
Contributor Author

Two of the test suites are failing because they could not run hhvm and two are failing with this error:

 Server launched with the following command:
  	'/usr/local/Cellar/hhvm-nightly/2022.05.03/bin/hh_server' '-d' '/Users/runner/work/hhast/hhast' '--waiting-client' '8'
  Running in daemon mode
  Server connection took over 2.0 seconds. Refreshing...
  Typing[4297] Was expecting an array or collection but type is unknown [1]
  -> It is unknown because the type of the lambda parameter could not be determined. Please add a type hint to the parameter [2]
  
  src/__Private/XHProf.hack:32:53
        30 |     invariant(self::$enabled === true, "Can't disable twice");
        31 |     self::$enabled = false;
  [1,2] 32 |     $raw = Dict\map(\xhprof_disable(), $v ==> (int) $v['wt']);
        33 | 
        34 |     $inclusive = dict[];
  
  1 error found.
  Error: Process completed with exit code 2.

This seems like an unrelated

@lexidor
Copy link
Contributor

lexidor commented May 4, 2022

Yes, I ran into that too. You can add a dict<arraykey, mixed> before $v and explicitly reformat. This makes the tests pass again without conflict with my branches.

6c00e36 is the diff you can apply.

@Atry Atry closed this May 5, 2022
@Atry Atry reopened this May 5, 2022
@Atry
Copy link
Contributor

Atry commented May 5, 2022

Retrigger the CI

@ryangreenberg
Copy link
Contributor Author

Retriggered CI again to get the build to pass on macOS. I think this PR is now ready for review/discussion.

Copy link
Contributor

@Atry Atry left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Atry Atry merged commit 202b982 into hhvm:main May 16, 2022
@ryangreenberg ryangreenberg deleted the pipe_var_use_linter branch May 18, 2022 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants