Skip to content
This repository has been archived by the owner on Jan 12, 2021. It is now read-only.

Add InArrayUsage sniff #12

Merged
merged 1 commit into from
Oct 11, 2017
Merged

Add InArrayUsage sniff #12

merged 1 commit into from
Oct 11, 2017

Conversation

thiemowmde
Copy link
Collaborator

@thiemowmde thiemowmde commented Sep 11, 2017

Custom sniff that finds unnecessary slow in_array() that can be replaced with array_key_exists() or isset().

Pinging @legoktm as he might be interested.

@legoktm
Copy link

legoktm commented Sep 13, 2017

To clarify, this is about finding calls like in_array( array_keys( ... ) ) right?

@JeroenDeDauw
Copy link
Contributor

That is what I understood from looking at it


// Should use array_key_exists
in_array( $key, array_keys( $array ) );
in_array( $key, array_keys );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t this a false positive? array_keys is not a function call here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. The tokenizer does not know what a function call is. All I can look for is a T_STRING token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or a T_STRING token followed by a T_LPAREN or whatever the ( token is called? (Probably with whitespace in between, and comments…)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but why? What false-positive would that avoid? Bareword strings? ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, fair enough ;)

array_keys( $array );
array_key_exists( $key, $array );
in_array( $key ) && array_keys( $array );
in_array( array_keys( $key ), $array );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests for array_flip instead of array_keys?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I was a little sloppy. I will add one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@lucaswerkmeister
Copy link
Member

This doesn’t seem to work inside an if:

    public function queryConstraintsForProperty( PropertyId $propertyId ) {
        $id = $propertyId->getSerialization();
        if ( !in_array( $id, array_keys( $this->cache ) ) ) {
            $this->cache[$id] = $this->lookup->queryConstraintsForProperty( $propertyId );
        }
        return $this->cache[$id];
    }

No warning on this code. (If I copy one of the failing lines from the tests here into the file, it triggers an error, so I know the sniffer runs.)

@thiemowmde
Copy link
Collaborator Author

I found the issue and fixed the sniff. Awesome you found this!

@lucaswerkmeister
Copy link
Member

Cool, thanks! Let’s merge :)

@lucaswerkmeister lucaswerkmeister merged commit 21a8457 into master Oct 11, 2017
@lucaswerkmeister lucaswerkmeister deleted the inArrayUsage branch October 11, 2017 12:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants