[ TypeChecker | Regression? ] It is nolonger an error to forget to use the $$ variable on the RHS of a |>

[lexidor]

Describe the bug

In hhvm 4.27, forgetting to use the $$ variable on the RHS of a |> operator, would be a typechecker error.

Naming[2069] This expression does not contain a usage of the special pipe variable. Did you forget to use the ($$) variable?

Hhvm 4.28 and above do not report an error for this code.

Standalone code, or other way to reproduce the problem

function main(): void {
  $x = 0 |> \intval(0);
  // or
  $x = 0 |> 0;
}

Steps to reproduce the behavior:

1. Create an empty hack project
2. Copy the example shown above to a src file.
3. Run hh_client under hhvm 4.27, observe two typechecker errors.
4. Run hh_client under hhvm 4.28, observe zero typechecker errors.

Expected behavior

The typechecker behavior of 4.27 is preferable.

Terminal output

$ docker run --tty --interactive -v /path/to/code:/mnt/code hhvm/hhvm:4.27.0 /bin/bash -l
root@bce64dc9fdce:/# cd /mnt/code/
root@bce64dc9fdce:/mnt/code# hh_client 
For more detailed logs, try `tail -f $(hh_client --monitor-logname) $(hh_client --logname)`
Server launched with the following command:
	'/opt/hhvm/4.27.0/bin/hh_server' '-d' '/mnt/code' '--waiting-client' '6'
Spawned typechecker (child pid=20)
Logs will go to /tmp/hh_server/monitor_logs/zSmntzScode-2020-05-13-21-05-39.monitor_log
Naming[2069] This expression does not contain a usage of the special pipe variable. Did you forget to use the ($$) variable?
   --> src/main.hack
  2 |   $x = 0 |> \intval(0);
    |             ^^^^^^^^^^

Naming[2069] This expression does not contain a usage of the special pipe variable. Did you forget to use the ($$) variable?
   --> src/main.hack
  4 |   $x = 0 |> 0;
    |             ^

2 errors found.
root@bce64dc9fdce:/mnt/code# exit
logout

$ docker run --tty --interactive -v /path/to/code:/mnt/code hhvm/hhvm:4.28.0 /bin/bash -l
root@d532c47e7f03:/# cd /mnt/code/
root@d532c47e7f03:/mnt/code# hh_client 
For more detailed logs, try `tail -f $(hh_client --monitor-logname) $(hh_client --logname)`
Server launched with the following command:
	'/opt/hhvm/4.28.0/bin/hh_server' '-d' '/mnt/code' '--waiting-client' '6'
Spawned typechecker (child pid=22)
Logs will go to /tmp/hh_server/monitor_logs/zSmntzScode-2020-05-13-21-05-59.monitor_log
No errors!
root@d532c47e7f03:/mnt/code# exit
logout

Desktop (please complete the following information):

• OS: Ubuntu 18.04 with docker
• HHVM Version: 4.27.0 |> 4.28.0+

hackc-f89bc3b7246753b2cdea6205005e1d49038d68f4-4.27.0
HipHop VM 4.27.0 (rel)
Compiler: 1571072038_021874391
Repo schema: 8bb24baf4d626dbff09412d248269b46ebf11e93

Additional context Add any other context about the problem here.

Between hhvm 4.27 and 4.28, there is one commit that mentions the pipe variable, which I believe may be the cause of this change:

commit f1e5724fd9062aaa22305af06fe825fe69238c1a Author: Andrew Kennedy [email protected] Date: Wed Oct 16 05:02:42 2019 -0700

Analyse scope of pipe variable in typing, not naming

Summary:
The naming phase now does very little scope analysis, using the integer first component of a `Local_id.t` only for (a) the experimental let-bound variables, and (b) pipe variables `$$`.

But these latter variables also don't need the integer: it's easy to scope them properly during typing, with the expression `e1 |> e2` typed by adding `$$` of type `t` (where `t` is the type of `e1`) to the environment when typing `e2`, and then restoring `$$` (or unsetting it) to its original state.

We also need to remove the variable under a lambda, as this is not supported by HackC.

Reviewed By: CatherineGasnier

Differential Revision: D17880302

fbshipit-source-id: 2e28d3909e339e52dbc5a094c16d185489857138

[lexidor]

https://github.com/facebook/hhvm/commit/f1e5724fd9062aaa22305af06fe825fe69238c1a#diff-c50b7cac126b2aee3ca59cdbe06d56de

[lexidor]

@Wilfred you recently commented the following:

https://github.com/facebook/hhvm/commit/7c6cc5f59d3a66731c9d49c90d76400fb72de28d#diff-99b49fa637b3602c3b1c6f7c88154a4de724ca51a8a86ff018c8b53f5f7b0c59R509

$$ is not required on the RHS of pipe expressions, but it's pretty pointless to use pipes without $$.

This used to be an error. The error code was never removed either. https://github.com/facebook/hhvm/blob/b84a47b6ae9cc8350079fe97422c5208495f70e5/hphp/hack/src/oxidized/gen/error_codes.rs#L119

If this is a remnant and the current behavior is the intended behavior, I can close this issue.

[Wilfred]

@lexidor oh, nice catch. I still think this an issue. It should probably be a parsing error.

My comment explains the runtime today -- we had some tests verifying the existing behaviour and the previous doc comment I wrote was wrong (foo() |> bar() is not equivalent to bar(foo()) as I previously thought.

I suggest we leave this open.

[lexidor]

https://docs.hhvm.com/hack/expressions-and-operators/pipe

Note for future. If it is later decided to keep allowing 0 |> 0 the docs need to be updated.

[lexidor]

I have one to use the |> operator in the same way some people use the , operator in JavaScript.

// Very confusing, looks like f(a(), b()), but behaves like
// a()
// f(b())
doingTwoThingsAtExpressionScope(
  (doThisButIgnoreTheReturnValue(), thenDoThisAndUseTheReturnValue())
);

atExpressionScope(doThis() |> thenThat());

I'd be affected if the hhvm 4.27 behavior was restored. Closing, but keeping the hhvm-docs issue open.