Warning for negative string offset (< PHP 7.1) #1791

maksimovic · 2018-06-15T21:02:34Z

This is my first attempt to do something related to Phan's true job - it took a while to figure things out enough for a small task like fixing #1778

Now, there are tree things that could/should be improved:

I'm not sure about the severity of the issue; probably it's not really critial.
the new if statement is rather big, but that's the pattern I found to be reliable. Since it's all new to me, I didn't want to go to far with shortening it. However, maybe I went too far in the opposite direction.
Couldn't find PHP 7.0-related tests except at the place where I added this one, but I can't run the suite from that directory - looks like things are outdated there.

@todo

@todo test

TysonAndre · 2018-06-16T01:32:30Z

src/Phan/Parse/ParseVisitor.php

@@ -1014,6 +1014,21 @@ public function visitDim(Node $node) : Context
            return $this->context;


Config::get_backward_compatibility_checks() is usually false. The new code should be added before this check.

TysonAndre · 2018-06-16T01:33:26Z

src/Phan/Parse/ParseVisitor.php

@@ -1014,6 +1014,21 @@ public function visitDim(Node $node) : Context
            return $this->context;
        }

+        // check for $str{-3} for PHP < 7.1
+        if (Config::get_closest_target_php_version_id() < 70100
+            && $node->children['expr'] instanceof Node


The code that performs the check can go into its own helper method

TysonAndre · 2018-06-16T01:36:32Z

src/Phan/Parse/ParseVisitor.php

+        if (Config::get_closest_target_php_version_id() < 70100
+            && $node->children['expr'] instanceof Node
+            && $node->children['expr']->kind == \ast\AST_VAR
+            && ($node->children['dim'] ?? null) instanceof Node


$union_type = UnionTypeVisitor::unionTypeFromNode($code_base, $context, $node->children['expr']) is recommended for checking if something is a string (e.g. $union_type->isNonNullStringType() is similar to what you want as a check).

The code you have here would also warn about accessing negative indices of arrays, which is a false positive.

TysonAndre · 2018-06-16T01:37:02Z

tests/php70_test/src/008_negative_string_offset.php

+$str = "abcdef";
+
+$char = $str{-1};
+


Add a test that this doesn't warn about negative offsets of arrays.

TysonAndre · 2018-06-16T01:39:28Z

Couldn't find PHP 7.0-related tests except at the place where I added this one, but I can't run the suite from that directory - looks like things are outdated there.

It's run by invoking ./test.sh, which requires bash (unix/linux)

At the time it was originally written, it seemed impossible to change certain config settings and plugins from the config that was originally loaded.

Now, it seems like it is possible (some tests do exactly that), and a regular PHPUnit test would be more appropriate for those tests. I just haven't gotten around to trying that.

- separated the check into its own method - using type check for strings, so we don't catch arrays - renamed the issue - it's not specific to PHP 7.0 - updated .gitignore to exclude .idea/ - updated config finder so it doesn't break when realpath() returns false

maksimovic · 2018-06-16T18:09:30Z

Thanks for the feedback.

I did come back to rearrange and improve the code. Also managed to run the tests for PHP 7.0 after fixing the test.sh file a bit (can't try to remove the file like this on osx):

- rm $ACTUAL_PATH -f || exit 1

+ if [ -f $ACTUAL_PATH ]; then
+    rm -f $ACTUAL_PATH || exit 1
+ fi

However, I'm having issues with determining the node type with UnionTypeVisitor::unionTypeFromNode() when the variable is in the global scope (like $str in my first test file), or when trying to get the type of a node that is a class property.

$str = "string";
$char = $str{-1}; // PhanUndeclaredVariable $str is undeclared

or:

class a
{

    public $string = "string";

    public function something()
    {
        return $this->string{-1}; // PhanUndeclaredVariable Variable $this is undeclared
    }
}

It just emits the PhanUndeclaredVariable warning and returns EmptyUnionType for these nodes.

I suspect that something could be wrong with the "context" for the $str variable, but I'm not sure how to get this properly. Class variable is a total mistery to me. I doubt that Phan is wrong, it's probably me missing something. If I sound confusing, then maybe it's best to push the example I've came up with and then you can run Phan against it to check.

Edit: note that static class properties or class constants aren't triggering the PhanUndeclaredVariable, they're doing just fine.

also, expected values aren't real (yet)

TysonAndre · 2018-06-16T18:53:12Z

src/Phan/Parse/ParseVisitor.php

+                )
+                // $str{"-1"}|["-1"]
+                || (
+                    \is_string($node->children['dim'])


Remove this, using a non-integer as a string literal as a string offset is already a separate issue type and doesn't need to be checked for.

src/008_negative_string_offset.php:17 PhanTypeMismatchDimFetch When fetching an array index from a value of type string, found an array index of type '-1', but expected the index to be of type int

I've noticed that, and I was in doubt whether to dismiss this case or to take it into account (so both warnings get emitted by default).

However, one could suppress/exclude PhanTypeMismatchDimFetch so this would go unnoticed, but... you are probably aware of that. My gut feeling is that we shouldn't do this, too. Will remove it.

Pretty much.

This code can be moved to the same place as PhanTypeMismatchDimFetch, since Phan is already checking the fetched offsets there

TysonAndre · 2018-06-16T19:07:21Z

src/Phan/Parse/ParseVisitor.php

@@ -1010,6 +1011,10 @@ public function visitAssign(Node $node) : Context

    public function visitDim(Node $node) : Context


I missed this the first time I looked at this code because I didn't expect it:

All of your changes to ParseVisitor need to be moved to visitDim in PostOrderAnalysisVisitor.

That's why you're seeing the strange undefined variables and undefined $this

UnionTypeVisitor::visitDim might be more appropriate, actually.
That already has checks if code is reading to or writing to that offset.

Eh, sorry. Thought I've made myself sound like a total noob when opening the PR :)

I knew that it must be something like that, but I couldn't figure it out myself. It's a bit overwhelming experience to dive into the internals of Phan, still struggling.

https://github.com/phan/phan/wiki/Developer%27s-Guide-To-Phan might be of help if you haven't seen it yet.

e.g. https://github.com/phan/phan/wiki/Developer%27s-Guide-To-Phan#ast-node-visitors

# Conflicts: # src/Phan/Issue.php

- moved the check to the place where we're pretty sure it could be a string - test file simplified - cleaned up the check from ParseVisitor

maksimovic · 2018-06-16T21:12:37Z

Build finally passing 😌 Added the check to UnionTypeVisitor::visitDim(), at the place where we're pretty confident that the type is string-ish.

I've also simplified the test file. All those cases from the previous version aren't really required now that the check is where it really should be.

Note that I did some minor changes along the way:

test.sh checking if $ACTUAL_PATH exists before trying to delete it
conditions in some of the if statements swapped, so the ones whose execution costs less are evaluated first
merged two separate if statements into one
.idea/ added to .gitignore (PhpStorm...)
check for $config_file_name being false, because realpath() can return it and then file_exists() blows up
added several blank lines to make the code less "compressed" - sometimes it's hard to distinguish where one thing ends and another one starts, especially when dealing with unfamiliar code.

I'm not emotionally attached to these small touches listed above, but I'd appreciate if they're not dismissed 🍺

maksimovic added 3 commits June 15, 2018 17:37

Merge branch 'upstream/master'

37e28bf

catch negative string offset for PHP < 7.1

fe990f3

@todo test

added 7.0 tests but not sure if it's useless or not

ce7e89d

TysonAndre requested changes Jun 16, 2018

View reviewed changes

maksimovic changed the title ~~Warning for negative string offset (PHP 7.0)~~ [WIP] Warning for negative string offset (PHP 7.0) Jun 16, 2018

work in progress - the test will fail

7b4555d

also, expected values aren't real (yet)

TysonAndre requested changes Jun 16, 2018

View reviewed changes

maksimovic added 3 commits June 16, 2018 21:24

Merge branch 'upstream/master' into negative-string-offset-warning

721cc4e

# Conflicts: # src/Phan/Issue.php

renamed test files due to the collision with another PR

7460ea6

check for negative string offset at the correct place

854c0e4

maksimovic changed the title ~~[WIP] Warning for negative string offset (PHP 7.0)~~ Warning for negative string offset (< PHP 7.1) Jun 16, 2018

finishing up negative string offset

4d241ef

- moved the check to the place where we're pretty sure it could be a string - test file simplified - cleaned up the check from ParseVisitor

maksimovic force-pushed the negative-string-offset-warning branch from 55e4d6e to 4d241ef Compare June 16, 2018 20:51

TysonAndre merged commit a8f816a into phan:master Jun 16, 2018

maksimovic deleted the negative-string-offset-warning branch June 16, 2018 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warning for negative string offset (< PHP 7.1) #1791

Warning for negative string offset (< PHP 7.1) #1791

maksimovic commented Jun 15, 2018

TysonAndre Jun 16, 2018

TysonAndre Jun 16, 2018

TysonAndre Jun 16, 2018

TysonAndre Jun 16, 2018

TysonAndre commented Jun 16, 2018

maksimovic commented Jun 16, 2018 •

edited

TysonAndre Jun 16, 2018

maksimovic Jun 16, 2018

TysonAndre Jun 16, 2018

TysonAndre Jun 16, 2018

TysonAndre Jun 16, 2018

maksimovic Jun 16, 2018

TysonAndre Jun 16, 2018

maksimovic commented Jun 16, 2018

		@@ -1014,6 +1014,21 @@ public function visitDim(Node $node) : Context
		return $this->context;

		@@ -1010,6 +1011,10 @@ public function visitAssign(Node $node) : Context

		public function visitDim(Node $node) : Context

Warning for negative string offset (< PHP 7.1) #1791

Warning for negative string offset (< PHP 7.1) #1791

Conversation

maksimovic commented Jun 15, 2018

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

TysonAndre commented Jun 16, 2018

maksimovic commented Jun 16, 2018 • edited

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

maksimovic commented Jun 16, 2018

maksimovic commented Jun 16, 2018 •

edited