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

PhanTypeInvalidDimOffset for local variable in closure #2504

Closed
Daimona opened this issue Feb 28, 2019 · 3 comments
Closed

PhanTypeInvalidDimOffset for local variable in closure #2504

Daimona opened this issue Feb 28, 2019 · 3 comments
Labels
enhancement This improves the quality of Phan's analysis of a codebase

Comments

@Daimona
Copy link
Member

Daimona commented Feb 28, 2019

While switching MediaWiki to Phan 1.2.4, I noticed what I think is a false positive. For this code, Phan emits the following:
includes/AbuseFilterHooks.php:460 PhanTypeInvalidDimOffset Invalid offset "af_enabled" of array type array{afa_consequence:'tag',af_deleted:false}
I tried to find a simplified version of this code reporting the same issue, but I couldn't find one. All I can say is that moving those 4 lines before the function call at line 450 makes the error disappear. So I guessed that this is somehow related to the code being inside a closure.
I also tried different variable and key naming, just in case, but that doesn't make any difference.

This is our phan config file, and I must also say that I only tried it on 1.2.4 (so not on master) due to troubles with my PC. And I also couldn't find any similar bug report.

@TysonAndre
Copy link
Member

I can reproduce this with this simpler example as well:

<?php

class AbuseFilterHooks {

    /**
     * @param array $tags
     * @param bool $enabled
     */
    private static function fetchAllTags( array &$tags, $enabled ) {
        // Function that derives the new key value
        return function () {
            // This is a pretty awful hack.

            $where = [ 'afa_consequence' => 'tag', 'af_deleted' => false ];
            $where['af_enabled'] = true;
        };
    }

    /**
     * @param string[] &$tags
     */
    public static function onListDefinedTags( array &$tags ) {
        self::fetchAllTags( $tags, false );
    }

}

@TysonAndre
Copy link
Member

Probably need to update PhanAnnotationAdder to recurse into closures as well - It adds FLAG_IGNORE_NULLABLE_AND_UNDEF to the left hand side of assignments

// in PostOrderAnalysisVisitor
        if ($node->flags & PhanAnnotationAdder::FLAG_IGNORE_NULLABLE_AND_UNDEF) {
            return $context;
        }
        // Check the array type to trigger TypeArraySuspicious
        try {
            /* $array_type = */
            UnionTypeVisitor::unionTypeFromNode(
                $code_base,
                $context,
                $node,
                false
            );
            // TODO: check if array_type has array but not ArrayAccess.
            // If that is true, then assert that $dim_type can cast to `int|string`
        } catch (IssueException $_) {
            // Detect this elsewhere, e.g. want to detect PhanUndeclaredVariableDim but not PhanUndeclaredVariable
        }
        return $context;
    }

@TysonAndre TysonAndre added the enhancement This improves the quality of Phan's analysis of a codebase label Feb 28, 2019
TysonAndre added a commit to TysonAndre/phan that referenced this issue Feb 28, 2019
@TysonAndre
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This improves the quality of Phan's analysis of a codebase
Projects
None yet
Development

No branches or pull requests

2 participants