From f5b4bd19b2719e039a8b6a9ec35f88c1385abca0 Mon Sep 17 00:00:00 2001 From: Erik Bernhardson Date: Thu, 30 Jun 2016 17:11:52 -0700 Subject: [PATCH] Add configuration for running etsy/phan against core 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 | 1 + tests/phan/bin/phan | 45 ++++ tests/phan/config.php | 416 ++++++++++++++++++++++++++++++++++ tests/phan/issues/.gitkeep | 0 tests/phan/stubs/README | 3 + tests/phan/stubs/hhvm.php | 27 +++ tests/phan/stubs/mail.php | 89 ++++++++ tests/phan/stubs/wikidiff.php | 28 +++ 8 files changed, 609 insertions(+) create mode 100755 tests/phan/bin/phan create mode 100644 tests/phan/config.php create mode 100644 tests/phan/issues/.gitkeep create mode 100644 tests/phan/stubs/README create mode 100644 tests/phan/stubs/hhvm.php create mode 100644 tests/phan/stubs/mail.php create mode 100644 tests/phan/stubs/wikidiff.php diff --git a/.gitignore b/.gitignore index 01a11bf411..b2c4d45cb1 100644 --- a/.gitignore +++ b/.gitignore @@ -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 index 0000000000..07caee6a59 --- /dev/null +++ b/tests/phan/bin/phan @@ -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 index 0000000000..7d4cfb95e0 --- /dev/null +++ b/tests/phan/config.php @@ -0,0 +1,416 @@ + [ + '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 + * 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 index 0000000000..e69de29bb2 diff --git a/tests/phan/stubs/README b/tests/phan/stubs/README new file mode 100644 index 0000000000..c458ab58fe --- /dev/null +++ b/tests/phan/stubs/README @@ -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 index 0000000000..8ffcdfb53b --- /dev/null +++ b/tests/phan/stubs/hhvm.php @@ -0,0 +1,27 @@ +