Ban overwriting a parameter in Hack

[fredemmott]

function foo($bar) {
  ...
  $bar = '123';
  ...
}

This is generally confusing and makes stuff harder to debug; it also makes func_get_args()/debug_backtrace() lie in PHP7 and HHVM.

[Ocramius]

:+1: on this.

Also relevant: http://3v4l.org/dAKRa - http://3v4l.org/eBMIR

[jwatzman]

I'm less convinced that this is a good idea to ban. The fact that it breaks func_get_args and debug_backtrace seem like a limitation of the runtime, not of the language. Doing this is idiomatic in other languages, as well.

I wonder how much it actually comes up in real code, or FB www at least. It might be worth adding the check as an experiment to see how wide the breakage is.

So I'm not necessarily opposed, but I'm so far unconvinced either :) @elgenie, what's your instinct on this?

[fredemmott]

Yeah, I'm not sure this is a good idea - I just filed this to get a discussion :)

[fredemmott]

That said, when func_get_args() was misbehaving, it took 4 of us > 1h to notice that the arg was being reassigned. Even if it's rare, the debugging/readability cost can be very high.

[jwatzman]

How hard would it be to change the behavior of func_get_args/debug_backtrace in hh mode? (Do you even agree that the current behavior is undesirable?) Do you think the behavior is relied upon? Would it be a good idea to make it both a static and runtime error in hh mode -- though I think I'd rather fix it outright, that might be a good "warn" transitional.

[fredemmott]

The current behavior doesn't seem desirable, but is an intentional performance tradeoff - fixing it requires copying every parameter on any function that potentially calls debug_backtrace() (even directly), or adding a COW check on every assignment.

[fredemmott]

Also, even if they worked fine, except if the assignment is at the top of the function, it's still bad for readability.

[Ocramius]

In general, overwriting a parameter also means changing its semantic meaning. Forcing a new variable declaration in those cases can do no harm

[fredemmott]

Basically, I feel that if I see:

function foo(T $bar) {
...
do_something_that_expects_a_T($bar);
...
}

It should be safe (and the same value) in any sane code; I shouldn't have to read however many lines above to be sure.

[steelbrain]

bump. and until this is fixed in the runtime, wouldn't it be cool to have hh_client report it?

[jwatzman]

Yep, but I am as yet not totally convinced that the readability gain would be worth the cost of fixing up code that already does this, since it's perfectly meaningful, if occasionally confusing. Not a high priority in any event.

[dlreeves]

If we would do something like this I'll rather we introduce a keyword that marks that a local variable should not be reassigned. In Scala they use val to be mean a variable that cannot be reassigned, and var for a variable that can be reassigned. I believe Java uses final to indicate the same thing. Using final we could express the desired behavior doing

function foo(final $bar) {
  ...
  $bar = 123; // Error because $bar was marked final
  ...
  final $x = 'Don't Override';
  ...
  $x = 'New Value'; // Error for the same reason
}

Not sure how the syntax would work or how hard it would be to implement in the runtime but I think it's a sensible feature.

[lexidor]

Meta, then Facebook, open sourced the https://github.com/hhvm/hhast library. This sounds like a great idea for a linter.

[lexidor]

Tracked as hhvm/hhast#545.