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

[Downgrade PHP 7.3] List reference assignment #4371

Merged
merged 28 commits into from Oct 8, 2020

Conversation

leoloso
Copy link
Contributor

@leoloso leoloso commented Oct 8, 2020

Fixes #4315

@leoloso
Copy link
Contributor Author

leoloso commented Oct 8, 2020

@TomasVotruba I need your input here.

I'm checking if this use case is downgradable:

$array = [[1, 2], [3, 4]];
foreach ($array as list(&$a, $b)) {
    $a = 7;
}
var_dump($array);
/*
array(2) {
  [0]=>
  array(2) {
    [0]=>
    int(7)
    [1]=>
    int(2)
  }
  [1]=>
  array(2) {
    [0]=>
    &int(7)
    [1]=>
    int(4)
  }
}
*/

According to the RFC this is not downgradable:

The predominant advantage of adding support for this is that it allows you to use reference assignment for multiple variables at once, which is not currently possible. [...] the advantage here is that you can reference assign some, but not all of the variables in list().

After running this PHP code, the reference exists on $array[1][0] since $a is still in scope after the foreach(). This part cannot be reproduced in PHP 7.2

If we didn't care about the references, but only about the final values, this is doable:

$array = [[1, 2], [3, 4]];
foreach ($array as &$item) {
    $item[0] = 7;
}
var_dump($array);
/*
array(2) {
  [0]=>
  array(2) {
    [0]=>
    int(7)
    [1]=>
    int(2)
  }
  [1]=>
  &array(2) {
    [0]=>
    int(7)
    [1]=>
    int(4)
  }
}
*/

Notice how the reference at the end points to the 2nd item in the array, and not to the 1st element of that item.

Hence, the final references cannot be recreated, but the values can.

So, what to do? I see 2 options:

Or maybe a bit more complex:

  • Downgrade it, unless variable $a is still referenced after running the code, if so throw DowngradeNotPossibleException

What do you think?

@leoloso
Copy link
Contributor Author

leoloso commented Oct 8, 2020

I thought a better alternative:

  • Allow to downgrade the code, if enabled by a configuration entry DANGEROUSLY_DOWNGRADE_LIST_REFERENCE_ASSIGNMENT_IN_FOREACH
  • By default, it is disabled
  • If disabled, and that code is found, throw a DangerousDowngradePossibleByConfigurationOnlyException, and there it shows the error message explaining that values are recreated, but references are not

@leoloso
Copy link
Contributor Author

leoloso commented Oct 8, 2020

Or maybe we could create a new set DANGEROUS_DOWNGRADE_PHP73, and extract this as a separate rule DangerousDowngradeListReferenceAssignmentInsideForeachRector

@leoloso
Copy link
Contributor Author

leoloso commented Oct 8, 2020

Otherwise this rule is ready. So I'll remove it from draft, and you can decide if to already ship it

@leoloso leoloso marked this pull request as ready for review October 8, 2020 07:59
@TomasVotruba
Copy link
Member

I think we can go for low hanging fruit now and test it in the wild. Just not to waste time on problem that might have 1 person from million.

If it will cause troubles, we can improve it :)

@TomasVotruba
Copy link
Member

Thank you 👍

@TomasVotruba TomasVotruba merged commit effa4d3 into master Oct 8, 2020
@TomasVotruba TomasVotruba deleted the downgrade-list-reference-assignment branch October 8, 2020 14:04
@leoloso
Copy link
Contributor Author

leoloso commented Oct 8, 2020

Ok, but we need to discuss this as a concept. This is just one example of a functionality that cannot be downgraded without side-effects. I actually think that having a set DANGEROUS_DOWNGRADE_PHP73 is a pretty good idea. We'll also have DANGEROUS_DOWNGRADE_PHP74 and DANGEROUS_DOWNGRADE_PHP80 too, for instance for WeakMap and others.

We can talk about it on our next meeting.

Btw, if you can generate a tag that would be cool, so I can already publicize this new set

@TomasVotruba
Copy link
Member

Set names makes sense 👍

I'll release it today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Downgrade PHP 7.3] list() and array destructuring reference assignment
2 participants