Skip to content

Commit

Permalink
New phan config setting: override_return_types (#4874)
Browse files Browse the repository at this point in the history
* traits should modify undefined return types with inferred return values too

* fix test output

* override return types if config is enabled

* fix syntax err

* add new config setting to config

* update config documentation

* alphabetize documentation config flags

* add tets

* add override return types tests to the test runner

* docs

* more docs

* Update src/Phan/Language/Element/FunctionTrait.php

Co-authored-by: Justin <justin.donato@gmail.com>

---------

Co-authored-by: Justin <justin.donato@gmail.com>
  • Loading branch information
dasl- and miniatureape authored Aug 13, 2024
1 parent 26b92a7 commit b52a882
Show file tree
Hide file tree
Showing 19 changed files with 429 additions and 70 deletions.
11 changes: 10 additions & 1 deletion .phan/config.php
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,21 @@
'guess_unknown_parameter_type_using_default' => false,

// Allow adding types to vague return types such as @return object, @return ?mixed in function/method/closure union types.
// Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature..
// Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature.
// This setting can be disabled on individual methods by adding `@phan-hardcode-return-type` to the doc comment.
//
// Disabled by default. This is more useful with `--analyze-twice`.
'allow_overriding_vague_return_types' => true,

// Add types to all return types. Normally, Phan only adds inferred returned types when there is no `@return` type
// or real return type signature. This setting can be disabled on individual methods by adding
// `@phan-hardcode-return-type` to the doc comment.
//
// Disabled by default. This is more useful with `--analyze-twice` and in conjunction with `PhoundPlugin` to
// detect more callsite possibilities. See the [PR description](https://github.com/phan/phan/pull/4874) where
// this setting was added for more details.
'override_return_types' => false,

// When enabled, infer that the types of the properties of `$this` are equal to their default values at the start of `__construct()`.
// This will have some false positives due to Phan not checking for setters and initializing helpers.
// This does not affect inherited properties.
Expand Down
14 changes: 13 additions & 1 deletion internal/Phan-Config-Settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ defined.
## allow_overriding_vague_return_types

Allow adding types to vague return types such as @return object, @return ?mixed in function/method/closure union types.
Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature..
Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature.
This setting can be disabled on individual methods by adding `@phan-hardcode-return-type` to the doc comment.

Disabled by default. This is more useful with `--analyze-twice`.
Expand Down Expand Up @@ -483,6 +483,18 @@ by changing this setting.

(Default: `2`)

## override_return_types

Add types to all return types. Normally, Phan only adds inferred returned types when there is no `@return` type
or real return type signature. This setting can be disabled on individual methods by adding
`@phan-hardcode-return-type` to the doc comment.

Disabled by default. This is more useful with `--analyze-twice` and in conjunction with `PhoundPlugin` to
detect more callsite possibilities. See the [PR description](https://github.com/phan/phan/pull/4874) where
this setting was added for more details.

(Default: `false`)

## parent_constructor_required

A set of fully qualified class-names for which
Expand Down
19 changes: 11 additions & 8 deletions src/Phan/Analysis/PostOrderAnalysisVisitor.php
Original file line number Diff line number Diff line change
Expand Up @@ -1635,13 +1635,11 @@ public function visitReturn(Node $node): Context
if ($method_return_type->isEmpty()
|| $method->isReturnTypeUndefined()
) {
if (!$is_trait) {
$method->setIsReturnTypeUndefined(true);
$method->setIsReturnTypeUndefined(true);

// Set the inferred type of the method based
// on what we're returning
$method->setUnionType($method->getUnionType()->withUnionType($expression_type));
}
// Set the inferred type of the method based
// on what we're returning
$method->setUnionType($method->getUnionType()->withUnionType($expression_type));

// No point in comparing this type to the
// type we just set
Expand All @@ -1661,10 +1659,15 @@ public function visitReturn(Node $node): Context
$is_mismatch = self::analyzeReturnStrict($code_base, $method, $resolved_expression_type, $method_return_type, $lineno, $inner_node);
}
}

// For functions that aren't syntactically Generators,
// update the set/existence of return values.

if ($method->isReturnTypeModifiable() && !$is_mismatch) {
//
// If `override_return_types` is enabled, update the return value even if it doesn't
// match the method's declared return value. One reason for this approach is because
// phpdoc return values may be incorrect or out of date, and phan errors about the
// incorrect phpdoc return values may be suppressed.
if ($method->isReturnTypeModifiable() && (!$is_mismatch || Config::getValue('override_return_types'))) {
// Add the new type to the set of values returned by the
// method
$method->setUnionType($method->getUnionType()->withUnionType($expression_type));
Expand Down
11 changes: 10 additions & 1 deletion src/Phan/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -339,12 +339,21 @@ class Config
'guess_unknown_parameter_type_using_default' => false,

// Allow adding types to vague return types such as @return object, @return ?mixed in function/method/closure union types.
// Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature..
// Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature.
// This setting can be disabled on individual methods by adding `@phan-hardcode-return-type` to the doc comment.
//
// Disabled by default. This is more useful with `--analyze-twice`.
'allow_overriding_vague_return_types' => false,

// Add types to all return types. Normally, Phan only adds inferred returned types when there is no `@return` type
// or real return type signature. This setting can be disabled on individual methods by adding
// `@phan-hardcode-return-type` to the doc comment.
//
// Disabled by default. This is more useful with `--analyze-twice` and in conjunction with `PhoundPlugin` to
// detect more callsite possibilities. See the [PR description](https://github.com/phan/phan/pull/4874) where
// this setting was added for more details.
'override_return_types' => false,

// When enabled, infer that the types of the properties of `$this` are equal to their default values at the start of `__construct()`.
// This will have some false positives due to Phan not checking for setters and initializing helpers.
// This does not affect inherited properties.
Expand Down
25 changes: 21 additions & 4 deletions src/Phan/Language/Element/FunctionTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -346,23 +346,40 @@ public function isReturnTypeUndefined(): bool

/**
* @return bool
* True if this method had no return type defined when it was defined,
* or if the method had a vague enough return type that Phan would add types to it
* (return type is inferred from the method signature itself and the docblock).
* True if any of the following are true:
* 1) This method had no return type defined when it was defined
* 2) The setting `allow_overriding_vague_return_types` is enabled and the method had a vague enough return
* type that Phan would add types to it (return type is inferred from the method signature
* itself and the docblock).
* 3) The setting `override_return_types` is enabled and the method has no hardcoded or dependent return type.
*/
public function isReturnTypeModifiable(): bool
{
if ($this->isReturnTypeUndefined()) {
return true;
}

$has_hardcoded_return_type = (bool) ($this->getPhanFlags() & Flags::HARDCODED_RETURN_TYPE);

if (Config::getValue('override_return_types')) {
if ($has_hardcoded_return_type || $this->hasDependentReturnType()) {
// If the return type is hardcoded or we have a plugin that's inferring the return
// type based on method params, assume that those will do a better job of
// determining what the return type should actually be.
return false;
} else {
return true;
}
}

if (!Config::getValue('allow_overriding_vague_return_types')) {
return false;
}
// Don't allow overriding method types if they have known overrides
if ($this instanceof Method && $this->isOverriddenByAnother()) {
return false;
}
if ($this->getPhanFlags() & Flags::HARDCODED_RETURN_TYPE) {
if ($has_hardcoded_return_type) {
return false;
}
$return_type = $this->getUnionType();
Expand Down
1 change: 1 addition & 0 deletions tests/Phan/Internal/ConfigEntry.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class ConfigEntry
'guess_unknown_parameter_type_using_default' => self::CATEGORY_ANALYSIS,
'use_tentative_return_type' => self::CATEGORY_ANALYSIS,
'allow_overriding_vague_return_types' => self::CATEGORY_ANALYSIS,
'override_return_types' => self::CATEGORY_ANALYSIS,
'infer_default_properties_in_construct' => self::CATEGORY_ANALYSIS,
'inherit_phpdoc_types' => self::CATEGORY_ANALYSIS,
'minimum_severity' => self::CATEGORY_ISSUE_FILTERING,
Expand Down
146 changes: 146 additions & 0 deletions tests/override_return_types_test/.phan/config.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
<?php

use \Phan\Issue;

/**
* This configuration will be read and overlaid 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.
*
* This is a config file which tests Phan's ability to infer and report missing types.
*/
return [
'target_php_version' => '7.3',

// If enabled, Phan will act as though it's certain of real return types of a subset of internal functions,
// even if those return types aren't available in reflection (real types were taken from php 8.0-dev).
//
// Note that in php 7 and earlier, php would return null or false if the argument types or counts were incorrect.
// As a result, enabling this setting may result in false positives for `--redundant-condition-detection`.
'assume_real_types_for_internal_functions' => true,

// Set to true in order to attempt to detect redundant and impossible conditions.
//
// This has some false positives involving loops,
// variables set in branches of loops, and global variables.
'redundant_condition_detection' => true,

// If true, missing properties will be created when
// they are first seen. If false, we'll report an
// error message.
'allow_missing_properties' => true,

// Allow null to be cast as any type and for any
// type to be cast to null.
'null_casts_as_any_type' => false,

// If enabled, scalars (int, float, bool, string, null)
// are treated as if they can cast to each other.
'scalar_implicit_cast' => false,

// If enabled, Phan will warn if **any** type in the method's object
// is definitely not an object.
// Setting this to true will introduce numerous false positives
// (and reveal some bugs).
'strict_method_checking' => true,

// If enabled, Phan will warn if **any** type in the argument's type
// cannot be cast to a type in the parameter's expected type.
// Setting this to true will introduce a large number of false positives (and some bugs).
// (For self-analysis, Phan has a large number of suppressions and file-level suppressions, due to \ast\Node being difficult to type check)
'strict_param_checking' => true,

// If enabled, Phan will warn if **any** type in a property assignment's type
// cannot be cast to a type in the property's expected type.
// Setting this to true will introduce a large number of false positives (and some bugs).
// (For self-analysis, Phan has a large number of suppressions and file-level suppressions, due to \ast\Node being difficult to type check)
'strict_property_checking' => true,

// If enabled, Phan will warn if **any** type in the return statement's type
// cannot be cast to a type in the method's declared return type.
// Setting this to true will introduce a large number of false positives (and some bugs).
// (For self-analysis, Phan has a large number of suppressions and file-level suppressions, due to \ast\Node being difficult to type check)
'strict_return_checking' => true,

// Test dead code detection
'dead_code_detection' => true,

'unused_variable_detection' => true,

'redundant_condition_detection' => true,

'guess_unknown_parameter_type_using_default' => true,

// If enabled, warn about throw statement where the exception types
// are not documented in the PHPDoc of functions, methods, and closures.
'warn_about_undocumented_throw_statements' => true,

// If enabled (and warn_about_undocumented_throw_statements is enabled),
// warn about function/closure/method calls that have (at)throws
// without the invoking method documenting that exception.
'warn_about_undocumented_exceptions_thrown_by_invoked_functions' => true,

'minimum_severity' => Issue::SEVERITY_LOW,

'directory_list' => ['src'],

'analyzed_file_extensions' => ['php'],

// Set this to true to enable the plugins that Phan uses to infer more accurate literal return types of `implode`, `implode`, and many other functions.
//
// Phan is slightly faster when these are disabled.
'enable_extended_internal_return_type_plugins' => true,

// This is a unit test of Phan itself, so don't cache it because the polyfill implementation may change before the next release.
'cache_polyfill_asts' => false,

// A list of plugin files to execute
// (Execute all of them.)
// FooName is shorthand for /path/to/phan/.phan/plugins/FooName.php.
'plugins' => [
'AlwaysReturnPlugin',
'DollarDollarPlugin',
'DuplicateArrayKeyPlugin',
'InvokePHPNativeSyntaxCheckPlugin',
'NumericalComparisonPlugin',
'PregRegexCheckerPlugin',
'PrintfCheckerPlugin',
'UnreachableCodePlugin',
'UnusedSuppressionPlugin',
'UseReturnValuePlugin',
'DuplicateExpressionPlugin',
'WhitespacePlugin',
'UnknownElementTypePlugin',
'AvoidableGetterPlugin',
'UnknownClassElementAccessPlugin',
],

// Set this to false to emit `PhanUndeclaredFunction` issues for internal functions that Phan has signatures for,
// but aren't available in the codebase, or from Reflection.
// (may lead to false positives if an extension isn't loaded)
//
// If this is true(default), then Phan will not warn.
//
// Even when this is false, Phan will still infer return values and check parameters of internal functions
// if Phan has the signatures.
'ignore_undeclared_functions_with_known_signatures' => false,

// Allow adding types to vague return types such as @return object, @return ?mixed in function/method/closure union types.
// Normally, Phan only adds inferred returned types when there is no `@return` type or real return type signature.
// This setting can be disabled on individual methods by adding `@phan-hardcode-return-type` to the doc comment.
//
// Disabled by default. This is more useful with `--analyze-twice`.
'allow_overriding_vague_return_types' => false,

// Add types to all return types. Normally, Phan only adds inferred returned types when there is no `@return` type
// or real return type signature. This setting can be disabled on individual methods by adding
// `@phan-hardcode-return-type` to the doc comment.
//
// Disabled by default. This is more useful with `--analyze-twice` and in conjunction with `PhoundPlugin` to
// detect more callsite possibilities. See the [PR description](https://github.com/phan/phan/pull/4874) where
// this setting was added for more details.
'override_return_types' => true,
];
1 change: 1 addition & 0 deletions tests/override_return_types_test/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This tests when the config setting `override_return_types` is enabled.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
src/001_override_return_types.php:24 PhanTypeMismatchReturnProbablyReal Returning new Cat() of type \Cat but getAnimal2() is declared to return \Vegetable (no real type) (the inferred real return type has nothing in common with the declared phpdoc return type)
src/001_override_return_types.php:29 PhanTypeMismatchArgumentReal Argument 1 ($v) is $this->getAnimal() of type \Animal|\Cat|\Dog but \OverrideReturnTypesTest::plant() takes \Vegetable defined at src/001_override_return_types.php:33
src/001_override_return_types.php:30 PhanPartialTypeMismatchArgument Argument 1 ($v) is $this->getAnimal2() of type \Cat|\Vegetable but \OverrideReturnTypesTest::plant() takes \Vegetable (\Animal|\Cat is incompatible) defined at src/001_override_return_types.php:33
37 changes: 37 additions & 0 deletions tests/override_return_types_test/src/001_override_return_types.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

class Animal {}

class Cat extends Animal {}

class Dog extends Animal {}

/* @phan-suppress-next-line PhanUnreferencedClass */
class Vegetable {}

/* @phan-suppress-next-line PhanUnreferencedClass */
class OverrideReturnTypesTest {

public function getAnimal(): Animal {
return random_int(0, 10) > 5 ? new Cat : new Dog;
}

/**
* This method has an incorrect phpdoc @return annotation.
* @return Vegetable
*/
public function getAnimal2() {
return new Cat;
}

/* @phan-suppress-next-line PhanUnreferencedPublicMethod */
public function run(): void {
$this->plant($this->getAnimal());
$this->plant($this->getAnimal2());
}

public function plant(Vegetable $v): void {
var_dump($v);
}

}
38 changes: 38 additions & 0 deletions tests/override_return_types_test/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/usr/bin/env bash
EXPECTED_PATH=expected/all_output.expected
ACTUAL_PATH=all_output.actual
if [ ! -d expected ]; then
echo "Error: must run this script from tests/override_return_types_test folder" 1>&2
exit 1
fi
echo "Generating test cases"
for path in $(echo expected/*.php.expected | LC_ALL=C sort); do cat "$path"; done > $EXPECTED_PATH
EXIT_CODE=$?
if [[ $EXIT_CODE != 0 ]]; then
echo "Failed to concatenate test cases" 1>&2
exit 1
fi
echo "Running phan in '$PWD' ..."
rm -f $ACTUAL_PATH || exit 1

# We use the polyfill parser because it behaves consistently in all php versions.
../../phan --force-polyfill-parser --memory-limit 1G --analyze-twice | tee $ACTUAL_PATH

# diff returns a non-zero exit code if files differ or are missing
# This outputs the difference between actual and expected output.
echo
echo "Comparing the output:"

if type colordiff >/dev/null; then
DIFF="colordiff"
else
DIFF="diff"
fi

$DIFF $EXPECTED_PATH $ACTUAL_PATH
EXIT_CODE=$?
if [ "$EXIT_CODE" == 0 ]; then
echo "Files $EXPECTED_PATH and output $ACTUAL_PATH are identical"
rm $ACTUAL_PATH
fi
exit $EXIT_CODE
Loading

0 comments on commit b52a882

Please sign in to comment.