Removing unintuitive coercions

[DavidSnider]

Definition of coercive operation for this context: an operation that changes a value to a different type/representation as part of an operation other than an explicit assignment. Note that this does not require writing back to the original reference.

General plan of action

Obviously don’t do this all at once. Start with when types don’t match (this is trivially true of “unary” operations that always coerce to a specific type). Once that is done, all non-container primitives are handled. On top of that, roll out the changes to each of these sections individually.

Once we get through all of this, we can figure out what exactly we want to do with containers and objects with the same type that get compared using ==, <, etc. We don’t need to front-load that solution, but we can discuss it contemporaneously. See the HIP for ==/===.

Note that for cases where it is reasonable to have a binary operator where one expression is an int and the other is a float (such as +), we choose not to trigger logs.

Note that we don’t mention cases where Class pointers and LazyClass pointers are coerced to strings if strings would not be further coerced.

String Concatenation (. , .=, interpolation)

Coercions occur when trying to concat/interpolate any of the following types:
      null, bool, int, double, object (though not in www), resource

When to log

Log if either the LHS or RHS are not strings. In the case of ConcatN, log if any aren’t strings.

Open question: Should we log if they’re ints, floats? None? Some?

The Fix

For ., wrap problematic expression in PHPism_FIXME::stringCastForConcatenation which explicitly casts the result to a string.

For .= if the RHS is the issue, do the same as for .. If the LHS is the issue insert a statement right before this that is a simple assignment to stringCastForConcatenation.

For interpolation, pull the expression out the interpolation into a temporary variable that is assigned to PHPism_FIXME::stringCastForInterpolation. Then interpolate that.

Bitwise operations (&, |, ^, ~, <<, >>) and assignment equivalents

Coercions occur when trying to operate any of the following types:
&, |, ^: null, double, string (unless both operands are strings), AnyArray, Object, resource, classptr, lazyclassptr
~: double
<<, >>: null, double, string, AnyArray, Object, resource, classptr, lazyclassptr

When to log

For &, |, ^, and assignment versions, log if either side are any of the coercive types and AND it is not the case that both sides are strings/lazyclassptr/classptr.

For ~, log if type is a double

For <<, >>, log if type is any of the coercive types

The Fix

For non-assignment versions, wrap problematic expression in PHPism_FIXME::intCastForBitwiseOp which explicitly casts the result to an int.

For assignment versions, if the RHS is the issue, do the same as for non-assignment versions. If the LHS is the issue add in an assignment in the previous statement that casts the LHS.

Pre/post increment/decrement

note: these are no longer usable within expressions. They should probably just be wholesale replaced with += and -=

This has different applications based on the situation

1. null/uninit - inc gives 1, dec gives null
2. int - stays int, changes value by +-1
3. float - stays float, changes value by +-1
4. classptr/lazyclass/string → classptr/lazyclass are first converted to strings
   1. “” goes to either -1 or “1”
   2. numeric string gets converted to int/float then does that case
   3. nonnumeric string does nothing on dec, and does some odd string changes on inc. Type stays string.
5. other - do nothing

When to log

Log under the above situations when types change, or

1. incrementing null
2. decrementing empty string
3. inc/dec numeric string (but not empty string)

The Fix

For the “pre” case, inject a statement immediately prior to this one that is an assignment to the result of PHPism_FIXME::coerceForIncOrDec. Alternatively, we could replace the pre-inc/dec with a call to something like PHPism_FIXME::coerceForAndIncDec which would accept either an int or a bool/enum to tell it whether to do the inc or dec case.

For the post case, remove the postincrement and inject a statement immediately following this one that is an assignment to the result of PHPism_FIXME::coerceForIncOrDec +/- 1.

coerceForIncOrDec works as follows, based on type of input:

1. null gets converted to 0
2. empty string gets converted to 0
3. numeric string gets int or float casted depending on how it looks

Mathematical operations (+, -, *, /, %, **) and assignment equivalents

+, -, *, /

Coercions occur when trying to operate any of the following types:
      null, bool, classptr, lazyclassptr, string, object, resource

When to log

log if either type is one of the coercive types

The Fix

Wrap the expression in PHPism_FIXME::coerceForMath. For the assignment version where we have logs for the LHS, just do this as a simple assignment prior to the use of the operator.

coerceForMath works as follows, based on type of input:

1. null converts to 0
2. bool converts to 0/1
3. classptr/lazyclassptr converts first to string and then that string gets converted as below
4. numeric string converts to an int or double based on how the string looks.
5. non-numeric string gets converted to 0
6. objects get casted to int
7. resources get casted to int

%

Coercions occur when trying to operate any of the following types:
      null, double, string (unless both operands are strings), AnyArray, Object, resource, Classptr, Lazyclassptr

When to log

Log if type is any of the coercive types

The Fix

Wrap the expression in PHPism_FIXME::coerceToIntForMod. For the assignment version where we have logs for the LHS, just do this as a simple assignment prior to the use of %.

coerceToIntForMod is just a universal int cast.

**

Coercions occur when trying to operate any of the following types:
      anyArray, null, bool, object, resource, string, classptr, lazyclassptr, funptr

When to log

Log if type is any of the coercive types

The Fix

Wrap the expression in PHPism_FIXME::coerceForPow. For the assignment version where we have logs for the LHS, just do this as a simple assignment prior to the use of **.

coerceForPow is implemented as follows, based on the type of input:

1. arrays get convert to 0.
2. null, bool, object, resource get coerced to int
3. string
   1. If numeric, convert to int or float accordingly
   2. it nonnumber, convert to 0
4. classptr, lazyclassptr, funcptr get coerced to string then do step 3

Comparison operations (< ,<=, > , >=, <=>)

#define str_to_num if string is numeric, convert to int or float. Else, convert to 0
#define class_meth_check !RuntimeOption::EvalHackArrDVArrs && RO::EvalIsCompatibleClsMethType

Note that for all of the classmeth and funptr stuff, we have to also handle the reified versions.

When coercions happen, based on types of LHS and RHS:

1. classmeth, classmeth → convert and test classnames and if same, convert and test function names
2. classmeth, bool → if class_meth_check convert LHS to true
3. X, bool where X is none of (any array, classmeth, funptr, refied versions) → convert LHS to bool
4. null, int/float → convert both sides to bool
5. bool, int/float → convert RHS to bool
6. string, int/float → str_to_num LHS.
7. non-Collection object, int/float → convert LHS to int/float
8. resource, int/float → convert LHS to int/float
9. classptr/lazyclassptr, int/float → convert LHS to string then str_to_num.
10. classmeth, int/float → if class_meth_check convert LHS to true and RHS to false
11. null, string → convert LHS to empty string
12. int, string → str_to_num RHS
13. bool, string → convert RHS to bool
14. double string → str_to_num RHS
15. non-Collection object, string → if Stringish, string cast LHS. Else, convert LHS to true and RHS to false
16. resource, string → convert both sides to floats
17. string, string → if LHS is numeric and RHS is numeric, convert them both to numbers and then compare. Yikes.
18. classmeth, string → if class_meth_check convert LHS to true and RHS to false
19. null, phparray → convert LHS to false
20. classmeth, phparray → if class_meth_check convert LHS to varray
21. null, object → convert LHS to false and RHS to true
22. int/float, non-Collection object → convert RHS to int/float
23. classptr/lazyclassptr, object → convert LHS to string and then...
24. string, non-Collection object → if Stringish, string cast RHS. Else, convert LHS to false and RHS to true
25. classmeth, non-Collection object → if class_meth_check convert LHS to false and RHS to true
26. null, resource → convert LHS to false and RHS to true
27. bool/int/float, resource → convert RHS to bool/int/float
28. classptr/lazyclassptr, resource → convert LHS to string and then...
29. string, resource → convert both sides to double
30. classmeth, resource → if class_meth_check convert LHS to true and RHS to false
31. resource, resource → convert both sides to ints
32. null, vec → convert LHS to false
33. classmeth, vec → if RuntimeOption::EvalHackArrDVArrs → convert LHS to vec
34. null, dict → convert LHS to false
35. null, keyset → convert LHS to false
36. null/int/float/string/classptr/lazyclassptr/resource, classmeth → if class_meth_check convert LHS to false and RHS to true
37. bool, classmeth → if class_meth_check convert LHS to true
38. varray/darray, classmeth → if class_meth_check convert JHS to varray
39. vec, classmeth → if RuntimeOption::EvalHackArrDVArrs && RO::EvalIsCompatibleClsMethType convert RHS to vec
40. non-Collection object, classmeth → convert LHS to true and RHS to false
41. null, (lazy)classptr → convert LHS to empty string and RHS to string
42. int/float, (lazy)classptr → convert RHS to string then str_to_num
43. bool, (lazy)classptr → convert RHS to true
44. non-Collection object, (lazy)classptr → if Stringish, string cast LHS. Else, convert LHS to true and RHS to false
45. resource, (lazy)classptr → convert LHS to double, convert RHS to string then double
46. clsmeth, lazyclassptr → if !RuntimeOption::EvalHackArrDVArrs convert LHS to true and RHS to false
47. clsmeth, classptr → if class_meth_check convert LHS to true and RHS to false
48. string, null → convert RHS to empty string
49. Object, null → convert LHS to true and RHS to false
50. not string or object, null → convert RHS to false

When to log

log whenever a coercion happens, based on the above list

The Fix

Inject explicit casts using PHPism_FIXME::TCastForComp for each necessary T.

Equality operations (==, !=)

When coercions happen, based on types of LHS and RHS (and is different than previous section):

1. bool, any array or vice versa → do a !Empty check on the AnyArray
2. reified classmeth/funptr, bool → convert the classmeth/funptr to true

When to log

log whenever a coercion happens, based on the above (and the above, above) list.

The Fix

Inject explicit casts using PHPism_FIXME::TCastForEQ for each necessary T.

Switch

The HHBC emitted can contain both the switch/sswitch bytecodes AND the jmp BCs / unoptimized switch info.

Switch opcodes implement == internally effectively, so we’ll probably need to add a new switcheq opcode that we can emit for switches and change in line with the internal implementation of the switch opcodes. Alternatively, we can do switch at the exact same time as we do ==.

Inside the opcodes, the following conversions happens, based on type of input:

switch

1. null → 0
2. false → 0
3. true → SPECIAL matches first non-zero int
4. float → if (int)$f == $f then convert to int
5. string → if intlike, convert to int. If floatlike, convert to float THEN if (int)$f == $f convert to int
6. Object → convert to int
7. resource → convert to int

sswitch

both interpreter and jit shell out directly to the implementation of == (i.e. tvEqual)

When to log

For the opcodes, log when a conversion happens
For the jmp and eq style, either

1. just let == logging handle it
2. emit a new switcheq opcode and have that log when a conversion happens. This would require teaching hhbbc etc to still be able to convert to the switch and sswitch opcodes.
3. log specifically when a case matches with eq but wouldn’t with same.

I think the first option is best, but that does require coupling the changes to switch with the changes to ==

There are two options for how to deal with the logs.

The Fix

OPTION 1:
For a given type of the case tested against T and an expression that will be coerced expr, appearing in the switch, wrap the expression with Phpism_FIXME::TCastIfEQ(expr, value_that_is_true_with_coercions). TCastIfEQ casts the first input to a T if expr === value_that_is_true_with_coercions.

OPTION 2:

switch($x) {
  case 1:
  break;
}

if we see a ‘1’ as the value for $x, we trigger a log. We then convert the switch to something like this:

switch($x) {
  case 1:
  case '1': // ADDED DUE TO TYPE COERCION IN SWITCH COMPARISON
  break;
}

Complications

OPTION 1:
If a switch tests against multiple types (more specifically when multiple cases are == each other), we could end up in an infinite loop of wrapping. We should simply fail to autofix when we don’t know that every case in a switch is the same type. Those should be relatively rare and be straightforwardly manually fixed.

If the switched expression gets too large, it could become unwieldy, but we can also test against that if it’s a serious concern.

OPTION 2:
With this change, we would still trigger a log when $x is ‘1’, since the first case will match. If we flip the cases, then we have the opposite problem.

The solution here is to teach HHVM that if it hits the path where it would have logged, it should check the immediately following cases (specifically where there is no code between them) and if another matches under ===, don’t trigger a log.

Doing this efficiently will be fun.

[ssandler]

I would argue that for purposes of string concatenation, coercing numeric types to strings is intuitive and is something many languages do. If we could consider keeping that behavior in place it would be a big help, we certainly rely on this heavily in our Hack codebase.

[fredemmott]

I generally agree; though what's the expectation for localization? None? (i.e. always . as a decimal, no thousands separator?)

[ssandler]

I don't have a strong opinion on that one, as anything that would potentially be shown to a user has to go through localization wrappers like the Intl extension for us. No thousands seperator and . as decimal seems reasonable to me.

[fredemmott]

Yeah, probably the only reasonable way, otherwise concatentation would have to be impure, depending on global state