[CodingStandard] add ArrayPropertyDefaultValueFixer #165

TomasVotruba · 2017-05-05T22:50:15Z

This fixer checks for undefined default values for arrray properties.

class SomeClass
{
    /**
     * @var int[]
     */
    public $property;
}

That would fail on:

foreach ($this->property as ...) {

}

@todo

wait for Symfony and CodeSniffer 3.0.1 and Slevomat releases: [SymbioticController] 2.0 Release #172 (comment) to prevent build fails

keradus · 2017-05-06T18:54:45Z

composer.json

@@ -67,7 +67,8 @@
        "phpstan/phpstan-nette": "^0.6",
        "phpstan/phpstan-doctrine": "^0.6",
        "object-calisthenics/phpcs-calisthenics-rules": "v3.0.0-RC3",
-        "roave/security-advisories": "dev-master"
+        "roave/security-advisories": "dev-master",
+        "gecko-packages/gecko-php-unit": "^2.0"


wow, you are using it directly ? :D

TBH I've to update that package soon for new PHPUnit ;)

It is required by test case and not included in original package :( should be, to prevent such additions.

it's there for 6 months:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/blame/2.2/composer.json#L34

Wrong place. It should be in require section, as AbstractFixerTestCase in /src uses it.

Now I realize I didn't understand your surprised comment initially :). No, I'm not using it directly. It's missed dependency, that is required by PHP-CS-Fixer, but forgotten there.

oh, nice catch ;)
then it would be better to fix it in original repo instead of making workarounds :D let me fix it for you

PHP-CS-Fixer/PHP-CS-Fixer#2757

@keradus Thanks! I can finally remove it

keradus · 2017-05-06T18:55:24Z

packages/CodingStandard/src/Fixer/Property/ArrayPropertyDefaultValueFixer.php

+use PhpCsFixer\Tokenizer\Tokens;
+use SplFileInfo;
+
+final class ArrayPropertyDefaultValueFixer extends AbstractFixer


don't ever rely on internal class of external library

Suggestion?

I would recommend implementing FixerInterface and not extend the internal abstract. The interface is not that complex and I'm happy to help out out it if needed.
Also; we keep a BC promise on the interface, which we do not for internal classes. So its the safer bet.

Will try, thanks.

@SpacePossum Works fine 👍

keradus · 2017-05-06T19:00:09Z

packages/CodingStandard/src/Fixer/Property/ArrayPropertyDefaultValueFixer.php

+
+    private function addDefaultValueForArrayProperty(Token $semicolonToken): void
+    {
+        $semicolonToken->setContent(' = [];');


This is violation and root cause of your problem reported by Travis.

Basically, token_get_all return an array collection of tokens.
All of them are either array (eg [T_CLASS, 'class', _line_number_]) or primitive string (eg ;).
At PHP CS Fixer, we wrap that array collection in Tokens class and single token (which we call token prototype) in Token (to normalize access across array/string token).

Token must be a valid representation of single token in language parsing terminology.
Here, you try to put 6 tokens in one instance (space, =, space, [, ], ;). That's abuse of token representation and that's why it cause you an issue.
Instead of overriding token with a lot of code, you need to inject new tokens into collection.

And solution?

ref PHP-CS-Fixer/PHP-CS-Fixer#2756 (comment)

ref PHP-CS-Fixer/PHP-CS-Fixer#2756 (comment)

That's exactly what I've tried before, but doesn't work: 73cffaf

what was the issue ?
oh, that's new commit, Travis will uncover the issue

You can do a few things here;

remove the token and insert new ones

override the token and after that insert new ones (or in other order)

override the token and insert new ones (in one go)

It doesn't matter which one you pick (not much at least), the important part is that each token is added to the Tokens collection and the one you no longer want is removed.

In this case I would do something like;

private function addDefaultValueForArrayProperty(Tokens $tokens, int $semicolonTokenIndex): void { $tokens[$semicolonTokenIndex]->clear(); $tokens->insertAt($semicolonTokenIndex, new Token([T_WHITESPACE, ' '])); $tokens->insertAt($semicolonTokenIndex, new Token('=')); $tokens->insertAt($semicolonTokenIndex, new Token([T_WHITESPACE, ' '])); $tokens->insertAt($semicolonTokenIndex, new Token([CT::T_ARRAY_SQUARE_BRACE_OPEN, '['])); $tokens->insertAt($semicolonTokenIndex, new Token([CT::T_ARRAY_SQUARE_BRACE_CLOSE, ']'])); }

note: CT:: is in namespace namespace PhpCsFixer\Tokenizer
note 2: didn't test the code ;)

note 3: I missed the last token for ;, but its more of the same ;)

one small improvement.
adding Token instances has the biggest complexity in Tokens collection.

$tokens->insertAt( $semicolonTokenIndex, [ array of `Token` instances ] );

would be 5 times faster

note 3: I missed the last token for ;, but its more of the same ;)

then... no need to remove ; first ;) it's the token you cleared on start.

Will try this, thank you!

keradus · 2017-05-06T21:14:30Z

packages/CodingStandard/src/Fixer/Property/ArrayPropertyDefaultValueFixer.php

-        $semicolonToken->setContent(' = [];');
+        $tokens->insertAt($semicolonPosition, new Token(']'));
+        $tokens->insertAt($semicolonPosition, new Token('['));
+        $tokens->insertAt($semicolonPosition, new Token(' '));


so... this is not a valid lexem.
new Token([T_WHITESPACE, ' ']) as shown in PHPUnit failure log

I don't understand the error and would have no idea that should be solved like this.

Also token accepts string|array, confusing.

Thanks, will try.

Still something wrong: a32fba2#diff-77b0aab940d6b4e15bf451af42434261R97

I feels like journey to Mount Doom to add just few characters. Could we do something about that? :)

I'm getting lost more and more and don't understand any error PHPUnit or Travis shows. Yes, I know about them, still don't how-to-fix them..

Could you please write exact code I should put there to make it work? I would really aprecciate it!

sadly, that's how internal PHP's parser works

feel free to propose how to handle it in nicer way

waiting for travis for new issue it will reveal

@TomasVotruba , you cannot replace one token with content of multiple tokens...

Next, you cannot simply inject string, as string is pointless without context. you need to provide context-specified values, so you provide tokens.
if not, what is class content? what it represents? info that one is just about creating new class definition, or he is getting class name via Foo::class syntax ?

possible workaround:

$new = Tokens::fromCode("<?php $mySuperContent=[];"); $new->clearRange(...); // drop initial code, like php tag opening $tokens->insertAt($index, $new);

But... parsing the code is slow. and you are not inserting random content - you know what you want to insert.

@TomasVotruba , you cannot replace one token with content of multiple tokens...

I know, that's what the method would do.

get a code, in this case = []

parse it to tokens

add those tokens before $position

Something you wrote, just not in 3 lines, but under one clear method.

Now there are many WTFs and hidden knowledge (you know as author, I don't user) like:

why backwards?

where to find tokens?

why some raw, some in CT and some in string?

still, it would be a method at Tokens, not at Token.
originally we had solution that works similar to what you propose internally (it wasn't nice for dev, but it done what you are saying). by removing it we gained ~50% performance boost.

about knowledge - I agree that sth may not be clear, if you are able to improve docs - go ahead !

backwards - general programming knowledge, not related to PHP CS Fixer at all, neither to PHP only.

where to find tokens - links PHP list of tokens and our registerec CT tokens

array vs string - internal php shit, ask them; CT - reasoning is hidden indeed, if interested, go for https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v2.3.0/src/Tokenizer/TransformerInterface.php#L20-L21

I know I enter this one a bit late, but I would like to point out one of the "difficulties" we face with tokens. I get it would be nice to override some tokens with new code in string form.
In your example you mention something like [ ], lets say I would like to replace a token with some like [$a].
Within PHP this can mean a lot of things, like;
$a[$a]; -> array index
[$a] = b(); -> array destruction on PHP 7.1 (CT tokens as PHP does not tokenize different)
b = [$a]; -> new array with element one element; $a

In order to figure out how this code-string inserted should be tokenized to new tokens the context is needed or a lot back tracking has to be done. Therefor we did the trade off for performance over complex, slow and hard to maintain code.

So yes, dealing with Tokens does require some background knowledge about how PHP does it and how we worked around some of it within the fixer. This might not be perfect or under documented, so feel free to contribute or talk to us, as documenting it all is just to much we can handle atm.

It's never too late to improve the code 👍
I don't want to replace anything in this case, just add some code.

I realize the fixer is complicated, so just this should clear the intent

keradus · 2017-05-06T21:39:08Z

packages/CodingStandard/src/Fixer/Property/ArrayPropertyDefaultValueFixer.php

+
+    public function fix(SplFileInfo $file, Tokens $tokens): void
+    {
+        foreach ($tokens as $index => $token) {


adding element to collection you are traversing forward is not the best idea, as you may end up with not visiting all of the elements:
https://3v4l.org/YBaFj
iterate backward instead

I'm glad you point out what si wrong. Could you also add solution right to your comment? would bring me much more value.

So, how?

iterate backward instead

replace

foreach ($tokens as $index => $token) {

with something like

for ($index = count($tokens) - 1; $index > 1; --$index) { $token = $tokens[$index]; }

as long as you insert tokens after the used $index you'll no longer miss candidates

How to iterate backward?

@SpacePossum Thanks!

Why is that not used in other fixers then?
Is that really relevant now? All works fine.

it is used in many fixers
it's not used when it's not needed (eg when fixer is not adding new content)

it is. your scenario works as you have it very small and few-elements offset is not big issue, as it means you not trying to fix end of script, which is usually class closing.

Imagine you would have 100 elements to fix in big crappy file. then fixer would fix only first ~80 properties

Some fixers do this, typically fixers that add tokens at some point.
It tends to become an issue when adding tokens while there are more candidates behind it.
For utest we add multiple fix candidates for this after each other.
In a abstract way, lets say we replace A with BC in;
XA => XBC; all good
XAYA => XBCYA ; because we stopped iterating at Y (after first fix), expected XBCYBC final result is.
Doing the reverse will not cause this issue;
XAYA => XAYBC (after first fix), XBCYBC (after second)

Thank you guys, I will try the reverse loop

keradus · 2017-05-06T21:42:43Z

packages/CodingStandard/src/Fixer/Property/ArrayPropertyDefaultValueFixer.php

+                continue;
+            }
+
+            $semicolonSignOrArrayOpenerPosition = $tokens->getNextMeaningfulToken($index + 4);


this is one issue. jumping by arbitrary +4 is causing the requested code to be added twice.
i have no clue how you ended up with this magic value.

I need to found out, if there is = [] or ; defined with the property.
How would you solve it?

don't use magic offsets

use getNextTokenOfKind to search for T_VARIABLE

but that's the solution for your assumption that docblock is put in correct place

This seems fine.

keradus · 2017-05-06T21:45:50Z

packages/CodingStandard/src/Fixer/Property/ArrayPropertyDefaultValueFixer.php

+
+            $semicolonSignOrArrayOpenerPosition = $tokens->getNextMeaningfulToken($index + 4);
+            $semicolonSignOrArrayOpenerToken = $tokens[$semicolonSignOrArrayOpenerPosition];
+            if ($semicolonSignOrArrayOpenerToken->equals('[')) {


next bug.

$foo = []; // case 1 $foo = array(); // case 2 $foo = null; // case 3 $foo; // case 4

so what actually you wanna detect?
case 4.

what you are detecting? nothing. this condition is always false. as [ is array index opening token.
as you saw previously, short array opening token is CT::T_ARRAY_SQUARE_BRACE_OPEN

instead of trying to guess that there is no need for fixing, check the opposite - is there a need for fix:

if (! $tokenAfterVariable->equals(';')) { $this->insert... }

Awesome answer. That fixed the issue, you're the man!

Thanks a lot.

keradus · 2017-05-06T21:46:36Z

packages/CodingStandard/tests/Fixer/Property/ArrayPropertyDefaultValueFixerTest.php

+                    /**
+                     * @var int[]
+                     */
+                    public $property;


add utest with mix of visibility/final/static properties

also, be aware that one could omit visibility attribute

Will do, just wanted to run at least the simplest use case first.

sure, i guess I just to much used to TDD

PHP-CS-Fixer and TDD doesn't combine to well. There are missing many explicit exceptions.

then... collaborate ;)

keradus · 2017-05-06T21:47:52Z

packages/CodingStandard/tests/Fixer/Property/ArrayPropertyDefaultValueFixerTest.php

+                    /**
+                     * @var int[]
+                     */
+                    public $property = [];


be aware of crappy code that one could make:

/** * @var int[] */ function count_my_array(array $array) { ... }

your code will crash on this

I think there is a another fixer for that.

But...

How to include it in the Fixer? Not sure how to detect, if is property on next line or not.

Case I presented is different - your fixer shall not touch that code, as one put documentation for properties ('@var' instead of '@param') but he tried to document a function.
You must not assume the documentation is valid, put in correct place and has meaning.

with current pattern, when you have docblock, see what is the next token - is it function related code or var related code, but that's tricky as it could be final public static... and then function or $var....

I would rather change your main loop to not search for docblock token, but for T_VARIABLE, and then you are sure that code you gonna modify would be a property, not function.
Then, having properties, you could search for ; or docblock

I would rather change your main loop to not search for docblock token, but for T_VARIABLE, and then you are sure that code you gonna modify would be a property, not function.
Then, having properties, you could search for ; or docblock

That was my first initial trial, but I was not able to find the docblock for property. findGivenKind() was throwing some errors I didn't understand.

I will look on that now.

test the meaningful tokens after the PHPDoc;
possible modifiers for properties are:

T_VAR

visibility token (T_PRIVATE, T_PUBLIC, T_PROTECTED)

T_PROTECTED

followed by

T_VARIABLE

if this does not apply than your are looking at either a method or class constant.

or grab variable and test the meaningful tokens before it (yes, you could search backward). all the same except variable would be replaced with docblock.

I would strongly advice to investigate T_VARIABLE's neighborhood as the fixer is trying to fix it. not make everything dependent on docblock, which may even not be for variable.

keradus · 2017-05-06T22:15:18Z

ok, I think that we make pretty deep analysis with @SpacePossum ;P
going sleep ;)

TomasVotruba · 2017-05-06T22:19:49Z

Thanks you guys ❤️ , me too.

I have some ideas how to improve this process thanks to you 👍

GN

This PR was merged into the 2.2 branch. Discussion ---------- GeckoPHPUnit is not dev dependency deprecated-packages/symplify#165 (comment) Commits ------- 9ef65af GeckoPHPUnit is not dev dependency

TomasVotruba · 2017-06-29T17:06:55Z

Thanks for feedback, buys.

I've added few changes and made it work for normal and static properties.

I merged this, because it drags too long and I couldn't remember my work weeks ago :(

Saying that, any additional feedback is appreciated.

SpacePossum · 2017-06-29T17:31:43Z

Nice to see this merged, I would say your fixer is good for what you want to do, so it nice work done : D
Let this be a two way street, if you want me to have a second look on something for refinement or whatever, just ping me 👍

TomasVotruba · 2017-06-29T22:41:43Z

Thanks for your affirmation, it means a lot to me from you.

Thanks for your feedback offer as well. I will definitely use it. I'd rather wait for next fixer, so it motivates me to do it sooner :)

TomasVotruba force-pushed the coding-standard-fixers branch from b3af954 to 8326d0b Compare May 5, 2017 23:03

keradus reviewed May 6, 2017

View reviewed changes

keradus mentioned this pull request May 6, 2017

GeckoPHPUnit is not dev dependency PHP-CS-Fixer/PHP-CS-Fixer#2757

Merged

TomasVotruba force-pushed the master branch from 5939bc4 to e3cde35 Compare May 30, 2017 10:29

TomasVotruba force-pushed the coding-standard-fixers branch from 13eefcd to 93d4622 Compare June 15, 2017 20:26

TomasVotruba added 10 commits June 29, 2017 18:13

[CodingStandard] complete few sniff to readme

0404bf3

[CodingStandard] complete few sniff to readme

f792de5

[CodingStandard] init ArrayPropertyDefaultValueFixer

6e080a3

[CodingStandard] first fixer fix?

b8457e0

[CodingStandard] first fixer fix?

d8e698d

[CodingStandard] first fixer fix? still broken

910fe0a

[CodingStandard] first fixer fix? still broken

0cfe509

[CodingStandard] first fixer fix

998f0d9

[CodingStandard] fixer temp

30f8fac

[CodingStandard] apply recommendations to the php-cs-fixer fixer

818510a

TomasVotruba force-pushed the coding-standard-fixers branch from 58d3d42 to 818510a Compare June 29, 2017 16:19

TomasVotruba added 2 commits June 29, 2017 18:23

conflicts cleanup

4790dfc

[ecs] return removed checkers, use PHPUnit 6.0 compatible package

86d9395

TomasVotruba changed the title ~~[CodingStandard] complete README and few checkers~~ [CodingStandard] add ArrayPropertyDefaultValueFixer Jun 29, 2017

TomasVotruba added 2 commits June 29, 2017 18:58

[CodingStandard] fix ArrayPropertyDefaultValue fixer

2141c29

fix cs and phpstan

3f34155

TomasVotruba merged commit 9390e1a into master Jun 29, 2017

TomasVotruba deleted the coding-standard-fixers branch June 29, 2017 17:07

TomasVotruba mentioned this pull request Jul 8, 2017

[CodingStandard] sniff ideas #59

Closed

13 tasks

This was referenced May 27, 2018

[ChangelogLinker] inspiration #841

Closed

[ChangelogLinker] generate from merged PRs #822

Closed

deprecated-packages locked as resolved and limited conversation to collaborators Oct 27, 2020

[CodingStandard] add ArrayPropertyDefaultValueFixer #165

[CodingStandard] add ArrayPropertyDefaultValueFixer #165

Conversation

TomasVotruba commented May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba May 6, 2017 • edited Loading

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

keradus May 6, 2017 • edited Loading

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

TomasVotruba May 6, 2017 • edited Loading

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

TomasVotruba May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba May 6, 2017 • edited Loading

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

SpacePossum May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba May 6, 2017 • edited Loading

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

keradus May 6, 2017 • edited Loading

Choose a reason for hiding this comment

TomasVotruba May 6, 2017 • edited Loading

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

keradus May 6, 2017 • edited Loading

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

SpacePossum May 6, 2017 • edited Loading

Choose a reason for hiding this comment

keradus May 6, 2017 • edited Loading

Choose a reason for hiding this comment

keradus commented May 6, 2017

TomasVotruba commented May 6, 2017 • edited Loading

TomasVotruba commented Jun 29, 2017

SpacePossum commented Jun 29, 2017

TomasVotruba commented May 5, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

keradus May 6, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

TomasVotruba May 11, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

SpacePossum May 6, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

keradus May 6, 2017 •

edited

Loading

TomasVotruba May 6, 2017 •

edited

Loading

keradus May 6, 2017 •

edited

Loading

SpacePossum May 6, 2017 •

edited

Loading

keradus May 6, 2017 •

edited

Loading

TomasVotruba commented May 6, 2017 •

edited

Loading