Add support for references and improve UnusedVariable checks (fixes #7254). #7339

AndrolGenhald · 2022-01-07T22:21:32Z

Adds support for references by setting Context::$vars_in_scope['$reference'] as a reference to Context::$vars_in_scope['$target'].
Adding the reference to $vars_in_scope basically fixed everything by itself, except for the Unused* checks, which required quite a lot of additional work.

AndrolGenhald · 2022-01-07T22:23:08Z

src/Psalm/Codebase.php

-        $expr_type->parent_nodes = [
-            $source->id => $source,
-        ];
+        $expr_type->parent_nodes[$source->id] = $source;


This isn't really that closely related to everything else in this PR, but I'm pretty sure it's a correct change. I'm not familiar enough with the taint feature to know how to test it, but I worry that without this change some of the taint stuff might mess up the parent_nodes for a reference.

Let's fix that when we have a reported bug then. I see we have multiple instances of both ways of declaring parent_nodes and I'm not sure when to choose which

I was leaning that way originally, but it looks like this only exists for plugins to use, and based on the name it's supposed to add a source, not remove all other sources as well.
I think the only time parent_nodes should be set to an array rather than setting an offset is when the previous nodes are known to be wrong, such as when assigning a variable.

$foo = 'a'; $foo = 'b'; // Should no longer have a parent node for `$foo = 'a'`

I'm unclear on what can cause a variable to have multiple parents though

I'm a bit unsure about that myself. I utilized multiple parents for references, but outside of that I don't know what use it has, other than it probably has something to do with taints.

AndrolGenhald · 2022-01-07T22:26:04Z

src/Psalm/Context.php

+     * Maps references to referenced variables for the current scope.
+     * With `$b = &$a`, this will contain `['$b' => '$a']`.
+     *
+     * All keys and values in this array are guaranteed to be set in $vars_in_scope.


I'm fairly confident I caught all the places a var_in_scope might be unset, but if this isn't true for some reason I consider that a bug to be fixed.

AndrolGenhald · 2022-01-07T22:32:07Z

src/Psalm/Context.php

@@ -667,7 +753,7 @@ public function removeDescendents(

        foreach ($this->vars_in_scope as $var_id => $type) {
            if (preg_match('/' . preg_quote($remove_var_id, '/') . '[\]\[\-]/', $var_id)) {
-                unset($this->vars_in_scope[$var_id]);
+                $this->remove($var_id, false);


Not sure why this wasn't unsetting $this->vars_possibly_in_scope before, but it works fine this way. I'm guessing it was just overlooked? Or maybe $this->vars_possibly_in_scope is guaranteed to not contain descendents? (side note, that should actually be descendant, descendent is an adjective with a subtly different meaning).

well, vars_possibly_in_scope is just an array of bools. It's nothing without vars_in_scope so yeah, probably overlooked and never caused issue

AndrolGenhald · 2022-01-07T22:42:05Z

src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php

+                        // `$a = &$b` this variable is $b), we need to analyze it as an assignment to null.
+                        AssignmentAnalyzer::analyze($statements_analyzer, $stmt, null, $stmt_type, $context, null);
+
+                        // Stop here, we don't want it to be considered possibly undefined like the array case.


Per #7328 I don't think the array should be PossiblyUndefined either, but I'll think about it and maybe do a separate pull request for that.

src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php

src/Psalm/Context.php

orklah · 2022-01-07T23:03:34Z

tests/FileManipulation/UnusedVariableManipulationTest.php

@@ -348,14 +348,27 @@ function foo() : void {
                '<?php
                    function foo() : void {
                        $a = 5;
-                        $b = &$a;


weird, this test was basically testing nothing...

Yep, it was confirming incorrect behavior...
Unfortunately the actual correct behavior doesn't work yet either, but I figure I'll leave it for a future improvement.

I hate that kind of tests when they're not flagged properly. When you break them, you just have no idea if the test is legit or not

Agreed, but at least this one was obvious.

tests/ReferenceTest.php

orklah · 2022-01-10T20:46:37Z

Just thinking about it, you removed every unset from $context->vars_in_scope, but we could had some. Would that be a problem?

Should we target Psalm 5 after telling plugin developpers to use your new function?

AndrolGenhald · 2022-01-10T21:37:59Z

Just thinking about it, you removed every unset from $context->vars_in_scope, but we could had some. Would that be a problem?

Should we target Psalm 5 after telling plugin developpers to use your new function?

I already targeted Psalm 5 because I removed Context::$vars_from_global, but that's another thing to think about.

I'm going to convert this to a draft for now, I'm trying to get assertions working on references, and I discovered that PHP's reference implementation is a broken mess.

AndrolGenhald · 2022-01-10T23:57:17Z

src/Psalm/Type/Reconciler.php

@@ -96,6 +101,32 @@ public static function reconcileKeyedTypes(
            return $existing_types;
        }

+        $reference_map = [];
+        if (!empty($existing_references)) {
+            // PHP behaves oddly when passing an array containing references: https://bugs.php.net/bug.php?id=20993


So this is fine and makes perfect sense. But then you do this, which is the exact same thing but with an array item, and everything falls apart. So what you end up with is this black magic to work around the problem.

I've come to the conclusion that references in PHP are a half-supported buggy mess, and if you use references in arrays or objects you're asking for trouble.

AndrolGenhald · 2022-01-11T00:01:00Z

I think there's still quite a bit of improvement that could be done, but the basic support is there now and for references that aren't array items or object properties things more or less work as expected. I think I'll leave it here unless someone reports a bug with it, it seems to me that relying on references in PHP for anything more than basic local variable modification is a bad idea, and there are probably much more impactful things that could be worked on.

AndrolGenhald · 2022-01-11T00:11:26Z

UPGRADING.md

+ - To remove a variable from the context, Context::remove(). Calling
+   `unset($context->vars_in_scope[$var_id])` can cause problems when using references.


Kind of makes me want @psalm-readonly-external or @psalm-readonly-allow-internal-mutation that would be like @psalm-readonly-allow-private-mutation but instead of restricting modifications to the class itself, it would restrict it to a namespace.

orklah · 2022-01-20T19:57:24Z

Can you describe on https://github.com/vimeo/psalm/blob/master/UPGRADING.md that unset on vars_in_scope is now forbidden? If you see any other BC break, please add them also

AndrolGenhald · 2022-01-20T20:49:09Z

@orklah That's already in there.

orklah · 2022-01-20T20:52:18Z

Sorry :( I did not look back at the changes.

I'd like to merge that soon, can you do a rebase?

orklah · 2022-01-25T19:55:40Z

Looks like merging the constant one created a lot more conflict. Please ping me after rebase and I'll merge this

…imeo#7254).

phpcs want `as` to have a single space before it, but it also wants the line indented. Worked around by assigning to another variable.

AndrolGenhald · 2022-01-26T19:01:59Z

@orklah done. That's the most painful rebase I've had to do in a while, there have been a good number of large changes recently like the 'code' key in tests.

orklah · 2022-01-26T19:08:57Z

Yep, sorry for that, but it's good news, the project is alive :)

orklah · 2022-01-26T19:09:07Z

Thanks!

AndrolGenhald commented Jan 7, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php Outdated Show resolved Hide resolved

AndrolGenhald commented Jan 7, 2022

View reviewed changes

src/Psalm/Context.php Outdated Show resolved Hide resolved

orklah reviewed Jan 7, 2022

View reviewed changes

tests/ReferenceTest.php Outdated Show resolved Hide resolved

orklah reviewed Jan 7, 2022

View reviewed changes

tests/ReferenceTest.php Show resolved Hide resolved

AndrolGenhald marked this pull request as draft January 10, 2022 21:38

AndrolGenhald marked this pull request as ready for review January 10, 2022 23:53

AndrolGenhald commented Jan 10, 2022

View reviewed changes

AndrolGenhald commented Jan 11, 2022

View reviewed changes

AndrolGenhald force-pushed the feature/references branch from 0dc5f41 to 6883665 Compare January 11, 2022 17:45

orklah added release:feature PR: Need review labels Jan 20, 2022

AndrolGenhald added 7 commits January 26, 2022 12:55

Add support for references and improve UnusedVariable checks (fixes v…

f945065

…imeo#7254).

Minor CS fix, phrasing improvement, fix test.

3c7d176

Remove impossible TODO.

bf60620

Add more reference tests.

089700d

Work around phpcs impossible syntax issue.

7694eb8

phpcs want `as` to have a single space before it, but it also wants the line indented. Worked around by assigning to another variable.

Improve reference support for assertions, array offsets, and properties.

57b99be

Add reference changes to UPGRADING documentation.

139c3af

AndrolGenhald added 2 commits January 26, 2022 12:56

Fix crash when a reference is reassigned in a loop.

740a101

Minor fixes after rebasing.

e268a05

AndrolGenhald force-pushed the feature/references branch from 6883665 to e268a05 Compare January 26, 2022 19:00

orklah merged commit f3f3fde into vimeo:master Jan 26, 2022

AndrolGenhald mentioned this pull request Feb 16, 2022

Alter removes a reference variable, breaking the code after #6114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for references and improve UnusedVariable checks (fixes #7254). #7339

Add support for references and improve UnusedVariable checks (fixes #7254). #7339

AndrolGenhald commented Jan 7, 2022

AndrolGenhald Jan 7, 2022

orklah Jan 7, 2022

AndrolGenhald Jan 7, 2022

orklah Jan 7, 2022

AndrolGenhald Jan 7, 2022

AndrolGenhald Jan 7, 2022

AndrolGenhald Jan 7, 2022

orklah Jan 7, 2022

AndrolGenhald Jan 7, 2022

orklah Jan 7, 2022

AndrolGenhald Jan 7, 2022

orklah Jan 7, 2022

AndrolGenhald Jan 7, 2022

orklah commented Jan 10, 2022

AndrolGenhald commented Jan 10, 2022

AndrolGenhald Jan 10, 2022

AndrolGenhald commented Jan 11, 2022

AndrolGenhald Jan 11, 2022 •

edited

Loading

orklah commented Jan 20, 2022 •

edited

Loading

AndrolGenhald commented Jan 20, 2022

orklah commented Jan 20, 2022

orklah commented Jan 25, 2022

AndrolGenhald commented Jan 26, 2022

orklah commented Jan 26, 2022

orklah commented Jan 26, 2022

		- To remove a variable from the context, Context::remove(). Calling
		`unset($context->vars_in_scope[$var_id])` can cause problems when using references.

Add support for references and improve UnusedVariable checks (fixes #7254). #7339

Add support for references and improve UnusedVariable checks (fixes #7254). #7339

Conversation

AndrolGenhald commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orklah commented Jan 10, 2022

AndrolGenhald commented Jan 10, 2022

Choose a reason for hiding this comment

AndrolGenhald commented Jan 11, 2022

AndrolGenhald Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

orklah commented Jan 20, 2022 • edited Loading

AndrolGenhald commented Jan 20, 2022

orklah commented Jan 20, 2022

orklah commented Jan 25, 2022

AndrolGenhald commented Jan 26, 2022

orklah commented Jan 26, 2022

orklah commented Jan 26, 2022

AndrolGenhald Jan 11, 2022 •

edited

Loading

orklah commented Jan 20, 2022 •

edited

Loading