Add configuration for running etsy/phan against core
authorErik Bernhardson <ebernhardson@wikimedia.org>
Fri, 1 Jul 2016 00:11:52 +0000 (17:11 -0700)
committerErik Bernhardson <ebernhardson@wikimedia.org>
Thu, 8 Dec 2016 04:04:01 +0000 (20:04 -0800)
Adds the necessary configuration and stubs for phan static analysis to
run against mediawiki core. A variety of fixes have been applied to core
recently such with this current configuration we are completely passing,
except for one issue with a bug in Phan (https://github.com/etsy/phan/issues/286)

In it's current configuration Phan will detect fatal errors trying to
access non existant classes, undefined method calls, and other errors.
The analysis can be expanded as we cleanup more of the codebase.  This
is in preparation for working on getting the CI to run phan as part of
the review process. I have found phan to be usefull in CirrusSearch
(although with stricter config) for catching bugs and I think it could
help in core as well.

We arn't too far from being able to disable `null_casts_as_any_type`,
there are only a few classes that need adjusting. Lots more work needs
to be done to reduce `minimum_severity` from 10 to 5 (normal) or
0 (low).

Change-Id: Iafd55b1380f37d7b0d195eed081f8042166690e8

.gitignore
tests/phan/bin/phan [new file with mode: 0755]
tests/phan/config.php [new file with mode: 0644]
tests/phan/issues/.gitkeep [new file with mode: 0644]
tests/phan/stubs/README [new file with mode: 0644]
tests/phan/stubs/hhvm.php [new file with mode: 0644]
tests/phan/stubs/mail.php [new file with mode: 0644]
tests/phan/stubs/wikidiff.php [new file with mode: 0644]

index 01a11bf..b2c4d45 100644 (file)
@@ -71,3 +71,4 @@ Thumbs.db
 /tags
 /.htaccess
 /.htpasswd
+/tests/phan/issues
diff --git a/tests/phan/bin/phan b/tests/phan/bin/phan
new file mode 100755 (executable)
index 0000000..07caee6
--- /dev/null
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+# Note that this isn't loaded in via composer because then composer can
+# only be run with php7.0
+if [ ! -f "$PHAN" ]; then
+       echo "The environment variable PHAN must point to the 'phan' file"
+       echo "in a checkout of https://github.com/etsy/phan.git"
+       exit 1
+fi
+
+cd "$(dirname "$0")"
+
+# Root directory of project
+export ROOT="$(git rev-parse --show-toplevel)"
+
+# Phan's issues directory
+export ISSUES="${ROOT}/tests/phan/issues"
+
+# Go to the root of this git repo
+cd "$ROOT"
+
+# Get the current hash of HEAD
+export REV="$(git rev-parse HEAD)"
+
+# Destination for issues found
+export RUN="${ISSUES}/issues-${REV}"
+
+# Run the analysis, emitting output to the
+# issues file.
+php7.0 $PHAN \
+       --project-root-directory "$ROOT" \
+       --config-file "$ROOT/tests/phan/config.php" \
+       --output "$ROOT/tests/phan/issues/issues-${REV}" \
+       -j 4
+
+# Re-link the latest directory
+rm -f "${ISSUES}/latest"
+ln -s "${RUN}" "${ISSUES}/latest"
+
+# Output any issues that were found
+cat "${RUN}"
+
+if [ $(wc -l < ${RUN}) -ne 0 ]; then
+       exit 1
+fi
diff --git a/tests/phan/config.php b/tests/phan/config.php
new file mode 100644 (file)
index 0000000..7d4cfb9
--- /dev/null
@@ -0,0 +1,416 @@
+<?php
+
+use \Phan\Config;
+
+// If xdebug is enabled, we need to increase the nesting level for phan
+ini_set( 'xdebug.max_nesting_level', 1000 );
+
+/**
+ * This configuration will be read and overlayed on top of the
+ * default configuration. Command line arguments will be applied
+ * after this file is read.
+ *
+ * @see src/Phan/Config.php
+ * See Config for all configurable options.
+ *
+ * A Note About Paths
+ * ==================
+ *
+ * Files referenced from this file should be defined as
+ *
+ * ```
+ *   Config::projectPath('relative_path/to/file')
+ * ```
+ *
+ * where the relative path is relative to the root of the
+ * project which is defined as either the working directory
+ * of the phan executable or a path passed in via the CLI
+ * '-d' flag.
+ */
+return [
+       /**
+        * A list of individual files to include in analysis
+        * with a path relative to the root directory of the
+        * project. directory_list won't find .inc files so
+        * we augment it here.
+        */
+       'file_list' => [
+               'maintenance/7zip.inc',
+               'maintenance/backupPrefetch.inc',
+               'maintenance/commandLine.inc',
+               'maintenance/sqlite.inc',
+               'maintenance/userOptions.inc',
+               'maintenance/backup.inc',
+               'maintenance/cleanupTable.inc',
+               'maintenance/importImages.inc',
+               'maintenance/userDupes.inc',
+               'maintenance/language/checkLanguage.inc',
+               'maintenance/language/languages.inc',
+       ],
+
+       /**
+        * A list of directories that should be parsed for class and
+        * method information. After excluding the directories
+        * defined in exclude_analysis_directory_list, the remaining
+        * files will be statically analyzed for errors.
+        *
+        * Thus, both first-party and third-party code being used by
+        * your application should be included in this list.
+        */
+       'directory_list' => [
+               'includes/',
+               'languages/',
+               'maintenance/',
+               'mw-config/',
+               'resources/',
+               'skins/',
+               'vendor/',
+               'tests/phan/stubs/',
+       ],
+
+       /**
+        * A file list that defines files that will be excluded
+        * from parsing and analysis and will not be read at all.
+        *
+        * This is useful for excluding hopelessly unanalyzable
+        * files that can't be removed for whatever reason.
+        */
+       'exclude_file_list' => function_exists( 'xcache_get' ) ? [] : [
+               // References xcache which probably isn't installed
+               'includes/libs/objectcache/XCacheBagOStuff.php'
+       ],
+
+       /**
+        * A list of directories holding code that we want
+        * to parse, but not analyze. Also works for individual
+        * files.
+        */
+       "exclude_analysis_directory_list" => [
+               'vendor/',
+               'tests/phan/stubs/',
+               // The referenced classes are not available in vendor, only when
+               // included from composer.
+               'includes/composer/',
+               // Directly references classes that only exist in Translate extension
+               'maintenance/language/',
+               // External class
+               'includes/libs/jsminplus.php',
+       ],
+
+       /**
+        * Backwards Compatibility Checking. This is slow
+        * and expensive, but you should consider running
+        * it before upgrading your version of PHP to a
+        * new version that has backward compatibility
+        * breaks.
+        */
+       'backward_compatibility_checks' => false,
+
+       /**
+        * A set of fully qualified class-names for which
+        * a call to parent::__construct() is required
+        */
+       'parent_constructor_required' => [
+       ],
+
+       /**
+        * Run a quick version of checks that takes less
+        * time at the cost of not running as thorough
+        * an analysis. You should consider setting this
+        * to true only when you wish you had more issues
+        * to fix in your code base.
+        *
+        * In quick-mode the scanner doesn't rescan a function
+        * or a method's code block every time a call is seen.
+        * This means that the problem here won't be detected:
+        *
+        * ```php
+        * <?php
+        * function test($arg):int {
+        *      return $arg;
+        * }
+        * test("abc");
+        * ```
+        *
+        * This would normally generate:
+        *
+        * ```sh
+        * test.php:3 TypeError return string but `test()` is declared to return int
+        * ```
+        *
+        * The initial scan of the function's code block has no
+        * type information for `$arg`. It isn't until we see
+        * the call and rescan test()'s code block that we can
+        * detect that it is actually returning the passed in
+        * `string` instead of an `int` as declared.
+        */
+       'quick_mode' => false,
+
+       /**
+        * By default, Phan will not analyze all node types
+        * in order to save time. If this config is set to true,
+        * Phan will dig deeper into the AST tree and do an
+        * analysis on all nodes, possibly finding more issues.
+        *
+        * See \Phan\Analysis::shouldVisit for the set of skipped
+        * nodes.
+        */
+       'should_visit_all_nodes' => true,
+
+       /**
+        * If enabled, check all methods that override a
+        * parent method to make sure its signature is
+        * compatible with the parent's. This check
+        * can add quite a bit of time to the analysis.
+        */
+       'analyze_signature_compatibility' => true,
+
+       // Only emit critical issues
+       "minimum_severity" => 10,
+
+       /**
+        * If true, missing properties will be created when
+        * they are first seen. If false, we'll report an
+        * error message if there is an attempt to write
+        * to a class property that wasn't explicitly
+        * defined.
+        */
+       'allow_missing_properties' => false,
+
+       /**
+        * Allow null to be cast as any type and for any
+        * type to be cast to null. Setting this to false
+        * will cut down on false positives.
+        */
+       'null_casts_as_any_type' => true,
+
+       /**
+        * If enabled, scalars (int, float, bool, string, null)
+        * are treated as if they can cast to each other.
+        *
+        * MediaWiki is pretty lax and uses many scalar
+        * types interchangably.
+        */
+       'scalar_implicit_cast' => true,
+
+       /**
+        * If true, seemingly undeclared variables in the global
+        * scope will be ignored. This is useful for projects
+        * with complicated cross-file globals that you have no
+        * hope of fixing.
+        */
+       'ignore_undeclared_variables_in_global_scope' => false,
+
+       /**
+        * Set to true in order to attempt to detect dead
+        * (unreferenced) code. Keep in mind that the
+        * results will only be a guess given that classes,
+        * properties, constants and methods can be referenced
+        * as variables (like `$class->$property` or
+        * `$class->$method()`) in ways that we're unable
+        * to make sense of.
+        */
+       'dead_code_detection' => false,
+
+       /**
+        * If true, the dead code detection rig will
+        * prefer false negatives (not report dead code) to
+        * false positives (report dead code that is not
+        * actually dead) which is to say that the graph of
+        * references will create too many edges rather than
+        * too few edges when guesses have to be made about
+        * what references what.
+        */
+       'dead_code_detection_prefer_false_negative' => true,
+
+       /**
+        * If disabled, Phan will not read docblock type
+        * annotation comments (such as for @return, @param,
+        * @var, @suppress, @deprecated) and only rely on
+        * types expressed in code.
+        */
+       'read_type_annotations' => true,
+
+       /**
+        * If a file path is given, the code base will be
+        * read from and written to the given location in
+        * order to attempt to save some work from being
+        * done. Only changed files will get analyzed if
+        * the file is read
+        */
+       'stored_state_file_path' => null,
+
+       /**
+        * Set to true in order to ignore issue suppression.
+        * This is useful for testing the state of your code, but
+        * unlikely to be useful outside of that.
+        */
+       'disable_suppression' => false,
+
+       /**
+        * If set to true, we'll dump the AST instead of
+        * analyzing files
+        */
+       'dump_ast' => false,
+
+       /**
+        * If set to a string, we'll dump the fully qualified lowercase
+        * function and method signatures instead of analyzing files.
+        */
+       'dump_signatures_file' => null,
+
+       /**
+        * If true (and if stored_state_file_path is set) we'll
+        * look at the list of files passed in and expand the list
+        * to include files that depend on the given files
+        */
+       'expand_file_list' => false,
+
+       // Include a progress bar in the output
+       'progress_bar' => false,
+
+       /**
+        * The probability of actually emitting any progress
+        * bar update. Setting this to something very low
+        * is good for reducing network IO and filling up
+        * your terminal's buffer when running phan on a
+        * remote host.
+        */
+       'progress_bar_sample_rate' => 0.005,
+
+       /**
+        * The number of processes to fork off during the analysis
+        * phase.
+        */
+       'processes' => 1,
+
+       /**
+        * Add any issue types (such as 'PhanUndeclaredMethod')
+        * to this black-list to inhibit them from being reported.
+        */
+       'suppress_issue_types' => [
+               // MediaWiki has so much deprecated class usage it's a bit hopeless to
+               // fix this immediately
+               'PhanDeprecatedClass',
+               'PhanDeprecatedFunction',
+               'PhanDeprecatedProperty',
+               // There are arround 1400 usages of undeclared properties.
+               // php is ok with that but it's a code smell.
+               'PhanUndeclaredProperty',
+       ],
+
+       /**
+        * If empty, no filter against issues types will be applied.
+        * If this white-list is non-empty, only issues within the list
+        * will be emitted by Phan.
+        */
+       'whitelist_issue_types' => [
+               // 'PhanAccessMethodPrivate',
+               // 'PhanAccessMethodProtected',
+               // 'PhanAccessNonStaticToStatic',
+               // 'PhanAccessPropertyPrivate',
+               // 'PhanAccessPropertyProtected',
+               // 'PhanAccessSignatureMismatch',
+               // 'PhanAccessSignatureMismatchInternal',
+               // 'PhanAccessStaticToNonStatic',
+               // 'PhanCompatibleExpressionPHP7',
+               // 'PhanCompatiblePHP7',
+               // 'PhanContextNotObject',
+               // 'PhanDeprecatedClass',
+               // 'PhanDeprecatedFunction',
+               // 'PhanDeprecatedProperty',
+               // 'PhanEmptyFile',
+               // 'PhanNonClassMethodCall',
+               // 'PhanNoopArray',
+               // 'PhanNoopClosure',
+               // 'PhanNoopConstant',
+               // 'PhanNoopProperty',
+               // 'PhanNoopVariable',
+               // 'PhanParamRedefined',
+               // 'PhanParamReqAfterOpt',
+               // 'PhanParamSignatureMismatch',
+               // 'PhanParamSignatureMismatchInternal',
+               // 'PhanParamSpecial1',
+               // 'PhanParamSpecial2',
+               // 'PhanParamSpecial3',
+               // 'PhanParamSpecial4',
+               // 'PhanParamTooFew',
+               // 'PhanParamTooFewInternal',
+               // 'PhanParamTooMany',
+               // 'PhanParamTooManyInternal',
+               // 'PhanParamTypeMismatch',
+               // 'PhanParentlessClass',
+               // 'PhanRedefineClass',
+               // 'PhanRedefineClassInternal',
+               // 'PhanRedefineFunction',
+               // 'PhanRedefineFunctionInternal',
+               // 'PhanStaticCallToNonStatic',
+               // 'PhanSyntaxError',
+               // 'PhanTraitParentReference',
+               // 'PhanTypeArrayOperator',
+               // 'PhanTypeArraySuspicious',
+               // 'PhanTypeComparisonFromArray',
+               // 'PhanTypeComparisonToArray',
+               // 'PhanTypeConversionFromArray',
+               // 'PhanTypeInstantiateAbstract',
+               // 'PhanTypeInstantiateInterface',
+               // 'PhanTypeInvalidLeftOperand',
+               // 'PhanTypeInvalidRightOperand',
+               // 'PhanTypeMismatchArgument',
+               // 'PhanTypeMismatchArgumentInternal',
+               // 'PhanTypeMismatchDefault',
+               // 'PhanTypeMismatchForeach',
+               // 'PhanTypeMismatchProperty',
+               // 'PhanTypeMismatchReturn',
+               // 'PhanTypeMissingReturn',
+               // 'PhanTypeNonVarPassByRef',
+               // 'PhanTypeParentConstructorCalled',
+               // 'PhanTypeVoidAssignment',
+               // 'PhanUnanalyzable',
+               // 'PhanUndeclaredClass',
+               // 'PhanUndeclaredClassCatch',
+               // 'PhanUndeclaredClassConstant',
+               // 'PhanUndeclaredClassInstanceof',
+               // 'PhanUndeclaredClassMethod',
+               // 'PhanUndeclaredClassReference',
+               // 'PhanUndeclaredConstant',
+               // 'PhanUndeclaredExtendedClass',
+               // 'PhanUndeclaredFunction',
+               // 'PhanUndeclaredInterface',
+               // 'PhanUndeclaredMethod',
+               // 'PhanUndeclaredProperty',
+               // 'PhanUndeclaredStaticMethod',
+               // 'PhanUndeclaredStaticProperty',
+               // 'PhanUndeclaredTrait',
+               // 'PhanUndeclaredTypeParameter',
+               // 'PhanUndeclaredTypeProperty',
+               // 'PhanUndeclaredVariable',
+               // 'PhanUnreferencedClass',
+               // 'PhanUnreferencedConstant',
+               // 'PhanUnreferencedMethod',
+               // 'PhanUnreferencedProperty',
+               // 'PhanVariableUseClause',
+       ],
+
+       /**
+        * Override to hardcode existence and types of (non-builtin) globals in the global scope.
+        * Class names must be prefixed with '\\'.
+        * (E.g. ['_FOO' => '\\FooClass', 'page' => '\\PageClass', 'userId' => 'int'])
+        */
+       'globals_type_map' => [
+               'IP' => 'string',
+       ],
+
+       // Emit issue messages with markdown formatting
+       'markdown_issue_messages' => false,
+
+       /**
+        * Enable or disable support for generic templated
+        * class types.
+        */
+       'generic_types_enabled' => true,
+
+       // A list of plugin files to execute
+       'plugins' => [
+       ],
+];
diff --git a/tests/phan/issues/.gitkeep b/tests/phan/issues/.gitkeep
new file mode 100644 (file)
index 0000000..e69de29
diff --git a/tests/phan/stubs/README b/tests/phan/stubs/README
new file mode 100644 (file)
index 0000000..c458ab5
--- /dev/null
@@ -0,0 +1,3 @@
+These stubs describe how code that is not available at analysis time should be
+used. No implementations are necessary, just define the classes and their
+methods and use phpdoc to describe what arguments are allowed.
diff --git a/tests/phan/stubs/hhvm.php b/tests/phan/stubs/hhvm.php
new file mode 100644 (file)
index 0000000..8ffcdfb
--- /dev/null
@@ -0,0 +1,27 @@
+<?php
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+// @codingStandardsIgnoreFile
+
+/**
+ * @param callable $callback
+ * @param mixed ...$parameters
+ */
+function register_postsend_function( $callback ) {
+}
+
diff --git a/tests/phan/stubs/mail.php b/tests/phan/stubs/mail.php
new file mode 100644 (file)
index 0000000..e906cdb
--- /dev/null
@@ -0,0 +1,89 @@
+<?php
+
+// @codingStandardsIgnoreFile
+
+/**
+ * Minimal set of classes necessary for UserMailer to be happy. Types
+ * taken from documentation at pear.php.net.
+ */
+class PEAR {
+       /**
+        * @param mixed $data
+        * @return bool
+        */
+       public static function isError( $data ) {
+       }
+}
+
+class PEAR_Error {
+       /**
+        * @return string
+        */
+       public function getMessage() {
+       }
+}
+
+class Mail {
+       /**
+        * @param string $driver
+        * @param array $params
+        * @return self
+        */
+       static public function factory( $driver, array $params = [] ) {
+       }
+
+       /**
+        * @param mixed $recipients
+        * @param array $headers
+        * @param string $body
+        * @return bool|PEAR_Error
+        */
+       public function send( $recipients, array $headers, $body ) {
+       }
+}
+
+class Mail_smtp extends Mail {
+}
+
+class Mail_mime {
+       /**
+        * @param mixed $params
+        */
+       public function __construct( $params = [] ) {
+       }
+
+       /**
+        * @param string $data
+        * @param bool $isfile
+        * @param bool $append
+        * @return bool|PEAR_Error
+        */
+       public function setTXTBody( $data, $isfile = false, $append = false ) {
+       }
+
+       /**
+        * @param string $data
+        * @param bool $isfile
+        * @return bool|PEAR_Error
+        */
+       public function setHTMLBody( $data, $isfile = false ) {
+       }
+
+       /**
+        * @param array|null $parms
+        * @param mixed $filename
+        * @param bool $skip_head
+        * @return string|bool|PEAR_Error
+        */
+       public function get( $params = null, $filename = null, $skip_head = false ) {
+       }
+
+       /**
+        * @param array|null $xtra_headers
+        * @param bool $overwrite
+        * @param bool $skip_content
+        * @return array
+        */
+       public function headers( array $xtra_headers = null, $overwrite = false, $skip_content = false ) {
+       }
+}
diff --git a/tests/phan/stubs/wikidiff.php b/tests/phan/stubs/wikidiff.php
new file mode 100644 (file)
index 0000000..9bd5d8d
--- /dev/null
@@ -0,0 +1,28 @@
+<?php
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+// @codingStandardsIgnoreFile
+
+/**
+ * @param string $text1
+ * @param string $text2
+ * @param int $numContextLines
+ * @return string
+ */
+function wikidiff2_do_diff( $text1, $text2, $numContextLines ) {
+}