Skip to content

Commit

Permalink
Fix #15 - check for uncaught throws if config flag is set
Browse files Browse the repository at this point in the history
  • Loading branch information
muglug committed Jun 22, 2018
1 parent dcc2c76 commit e3ae1bf
Show file tree
Hide file tree
Showing 24 changed files with 480 additions and 3 deletions.
9 changes: 9 additions & 0 deletions config.xsd
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<xs:element name="stubs" type="StubsType" minOccurs="0" maxOccurs="1" />
<xs:element name="plugins" type="PluginsType" minOccurs="0" maxOccurs="1" />
<xs:element name="issueHandlers" type="IssueHandlersType" minOccurs="0" maxOccurs="1" />
<xs:element name="ignoreExceptions" type="ExceptionsType" minOccurs="0" maxOccurs="1" />
</xs:choice>

<xs:attribute name="name" type="xs:string" />
Expand All @@ -39,6 +40,7 @@
<xs:attribute name="memoizeMethodCallResults" type="xs:string" />
<xs:attribute name="hoistConstants" type="xs:string" />
<xs:attribute name="addParamDefaultToDocblockType" type="xs:string" />
<xs:attribute name="checkForThrowsDocblock" type="xs:string" />
</xs:complexType>

<xs:complexType name="ProjectFilesType">
Expand Down Expand Up @@ -78,6 +80,12 @@
</xs:sequence>
</xs:complexType>

<xs:complexType name="ExceptionsType">
<xs:sequence>
<xs:element name="class" maxOccurs="unbounded" type="NameAttributeType" />
</xs:sequence>
</xs:complexType>

<xs:complexType name="StubsType">
<xs:sequence>
<xs:element name="file" maxOccurs="unbounded" type="NameAttributeType" />
Expand Down Expand Up @@ -167,6 +175,7 @@
<xs:element name="MissingPropertyType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="MissingReturnType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="MissingDocblockType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="MissingThrowsDocblock" type="IssueHandlerType" minOccurs="0" />
<xs:element name="MixedArgument" type="IssueHandlerType" minOccurs="0" />
<xs:element name="MixedArrayAccess" type="IssueHandlerType" minOccurs="0" />
<xs:element name="MixedArrayAssignment" type="IssueHandlerType" minOccurs="0" />
Expand Down
2 changes: 2 additions & 0 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ Psalm uses an XML config file. A barebones example looks like this:
When `true`, constants defined in a function in a file are assumed to be available when requiring that file, and not just when calling that function. Defaults to `false` (i.e. constants defined in functions will *only* be available for use when that function is called)
- `addParamDefaultToDocblockType=[bool]`<br />
Occasionally a param default will not match up with the docblock type. By default, Psalm emits an issue. Setting this flag to `true` causes it to expand the param type to include the param default. Defaults to `false`.
- `checkThrowsDocblock=[bool]`<br />
When `true`, Psalm will check that the developer has supplied `@throws` docblocks for every exception thrown in a given function or method. Defaults to `false`.

### Running Psalm

Expand Down
14 changes: 14 additions & 0 deletions docs/issues.md
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,20 @@ function foo() {
}
```

### MissingThrowsDocblock

Emitted when a function doesn't have a return type defined

```php
function foo(int $x, int $y) : int {
if ($y === 0) {
throw new \InvalidArgumentException('Cannot divide by zero');
}

return intdiv($x, $y);
}
```

### MixedArgument

Emitted when Psalm cannot determine the type of an argument
Expand Down
5 changes: 4 additions & 1 deletion src/Psalm/Checker/ClassChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -852,8 +852,11 @@ private function analyzeClassMethod(
}
}

$method_context = clone $class_context;
$method_context->collect_exceptions = $config->check_for_throws_docblock;

$method_checker->analyze(
clone $class_context,
$method_context,
$global_context ? clone $global_context : null
);

Expand Down
6 changes: 6 additions & 0 deletions src/Psalm/Checker/CommentChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,12 @@ public static function extractFunctionDocblockInfo($comment, $line_number)
}
}

if (isset($comments['specials']['throws'])) {
foreach ($comments['specials']['throws'] as $throws_entry) {
$info->throws[] = preg_split('/[\s]+/', $throws_entry)[0];
}
}

if (isset($comments['specials']['template'])) {
foreach ($comments['specials']['template'] as $template_line) {
$template_type = preg_split('/[\s]+/', $template_line);
Expand Down
24 changes: 24 additions & 0 deletions src/Psalm/Checker/FunctionLikeChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Psalm\Issue\MismatchingDocblockParamType;
use Psalm\Issue\MissingClosureParamType;
use Psalm\Issue\MissingParamType;
use Psalm\Issue\MissingThrowsDocblock;
use Psalm\Issue\ReservedWord;
use Psalm\Issue\UnusedParam;
use Psalm\IssueBuffer;
Expand Down Expand Up @@ -619,6 +620,29 @@ public function analyze(Context $context, Context $global_context = null, $add_m
}
}

if ($context->collect_exceptions) {
if ($context->possibly_thrown_exceptions) {
$undocumented_throws = array_diff_key($context->possibly_thrown_exceptions, $storage->throws);

foreach ($undocumented_throws as $possibly_thrown_exception => $_) {
if (IssueBuffer::accepts(
new MissingThrowsDocblock(
$possibly_thrown_exception . ' is thrown but not caught - please either catch'
. ' or add a @throws annotation',
new CodeLocation(
$this,
$this->function,
null,
true
)
)
)) {
// fall through
}
}
}
}

if ($add_mutations) {
if (isset($this->return_vars_in_scope[''])) {
$context->vars_in_scope = TypeChecker::combineKeyedTypes(
Expand Down
4 changes: 4 additions & 0 deletions src/Psalm/Checker/Statements/Block/DoChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ function (Clause $c) use ($mixed_var_ids) {
$context->unreferenced_vars = $do_context->unreferenced_vars;
}

if ($context->collect_exceptions) {
$context->possibly_thrown_exceptions += $do_context->possibly_thrown_exceptions;
}

ExpressionChecker::analyze($statements_checker, $stmt->cond, $inner_loop_context);
}
}
11 changes: 11 additions & 0 deletions src/Psalm/Checker/Statements/Block/ForChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,17 @@ public static function analyze(
);
}

if ($context->collect_references) {
$context->unreferenced_vars = array_intersect_key(
$for_context->unreferenced_vars,
$context->unreferenced_vars
);
}

if ($context->collect_exceptions) {
$context->possibly_thrown_exceptions += $for_context->possibly_thrown_exceptions;
}

return null;
}
}
4 changes: 4 additions & 0 deletions src/Psalm/Checker/Statements/Block/ForeachChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,10 @@ public static function analyze(
$context->referenced_var_ids
);

if ($context->collect_exceptions) {
$context->possibly_thrown_exceptions += $foreach_context->possibly_thrown_exceptions;
}

if ($context->collect_references) {
foreach ($foreach_context->unreferenced_vars as $var_id => $locations) {
if (isset($context->unreferenced_vars[$var_id])) {
Expand Down
12 changes: 12 additions & 0 deletions src/Psalm/Checker/Statements/Block/IfChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,10 @@ protected static function analyzeIfBlock(
}
}
}

if ($outer_context->collect_exceptions) {
$outer_context->possibly_thrown_exceptions += $if_context->possibly_thrown_exceptions;
}
}

/**
Expand Down Expand Up @@ -1273,6 +1277,10 @@ function (Clause $c) use ($conditional_assigned_var_ids) {
);
}

if ($outer_context->collect_exceptions) {
$outer_context->possibly_thrown_exceptions += $elseif_context->possibly_thrown_exceptions;
}

$if_scope->negated_clauses = array_merge(
$if_scope->negated_clauses,
Algebra::negateFormula($elseif_clauses)
Expand Down Expand Up @@ -1555,6 +1563,10 @@ protected static function analyzeElseBlock(
}
}

if ($outer_context->collect_exceptions) {
$outer_context->possibly_thrown_exceptions += $else_context->possibly_thrown_exceptions;
}

if ($project_checker->infer_types_from_usage) {
$else_possible_param_types = $else_context->possible_param_types;

Expand Down
4 changes: 4 additions & 0 deletions src/Psalm/Checker/Statements/Block/SwitchChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,10 @@ public static function analyze(
}
}

if ($context->collect_exceptions) {
$context->possibly_thrown_exceptions += $case_context->possibly_thrown_exceptions;
}

if ($context->collect_references) {
$new_possibly_assigned_var_ids =
$new_possibly_assigned_var_ids + $new_case_possibly_assigned_var_ids;
Expand Down
53 changes: 53 additions & 0 deletions src/Psalm/Checker/Statements/Block/TryChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ public static function analyze(
$project_checker = $statements_checker->getFileChecker()->project_checker;
$codebase = $project_checker->codebase;

$existing_thrown_exceptions = $context->possibly_thrown_exceptions;

/**
* @var array<string, bool>
*/
$context->possibly_thrown_exceptions = [];

if ($all_catches_leave) {
$try_context = $context;
} else {
Expand Down Expand Up @@ -179,6 +186,43 @@ public static function analyze(
$fq_catch_classes[] = $fq_catch_class;
}

$potentially_caught_classes = array_flip($fq_catch_classes);

if ($catch_context->collect_exceptions) {
foreach ($fq_catch_classes as $fq_catch_class) {
$fq_catch_class_lower = strtolower($fq_catch_class);

foreach ($context->possibly_thrown_exceptions as $exception_fqcln => $_) {
$exception_fqcln_lower = strtolower($exception_fqcln);

if ($exception_fqcln_lower === $fq_catch_class_lower) {
unset($context->possibly_thrown_exceptions[$exception_fqcln]);
continue;
}

if ($codebase->classExists($exception_fqcln)
&& $codebase->classExtendsOrImplements(
$exception_fqcln,
$fq_catch_class
)
) {
unset($context->possibly_thrown_exceptions[$exception_fqcln]);
continue;
}

if ($codebase->interfaceExists($exception_fqcln)
&& $codebase->interfaceExtends(
$exception_fqcln,
$fq_catch_class
)
) {
unset($context->possibly_thrown_exceptions[$exception_fqcln]);
continue;
}
}
}
}

$catch_var_id = '$' . $catch_var_name;

$catch_context->vars_in_scope[$catch_var_id] = new Union(
Expand Down Expand Up @@ -272,6 +316,13 @@ function ($fq_catch_class) use ($codebase) {
}
}

if ($context->collect_exceptions) {
$potentially_caught_classes = array_diff_key(
$potentially_caught_classes,
$context->possibly_thrown_exceptions
);
}

if ($catch_actions[$i] !== [ScopeChecker::ACTION_END]) {
foreach ($catch_context->vars_in_scope as $var_id => $type) {
if ($context->hasVariable($var_id)
Expand Down Expand Up @@ -312,6 +363,8 @@ function ($fq_catch_class) use ($codebase) {
}
}

$context->possibly_thrown_exceptions += $existing_thrown_exceptions;

return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,10 @@ public static function analyze(
}
}

if ($function_storage && $context->collect_exceptions) {
$context->possibly_thrown_exceptions += $function_storage->throws;
}

try {
if ($function_storage && $function_storage->return_type) {
$return_type = clone $function_storage->return_type;
Expand Down
4 changes: 4 additions & 0 deletions src/Psalm/Checker/Statements/Expression/CallChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ protected static function checkMethodArgs(
}

$method_storage = $declaring_class_storage->methods[strtolower($declaring_method_name)];

if ($context->collect_exceptions) {
$context->possibly_thrown_exceptions += $method_storage->throws;
}
}

if (!$class_storage->user_defined) {
Expand Down
6 changes: 6 additions & 0 deletions src/Psalm/Checker/Statements/ThrowChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ public static function analyze(
)) {
return false;
}
} elseif ($context->collect_exceptions) {
foreach ($throw_type->getTypes() as $throw_atomic_type) {
if ($throw_atomic_type instanceof TNamedObject) {
$context->possibly_thrown_exceptions[$throw_atomic_type->value] = true;
}
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Psalm/Checker/StatementsChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -451,11 +451,11 @@ function ($line) {
if (!$project_checker->codebase->register_global_functions) {
$function_id = strtolower($stmt->name->name);
$function_context = new Context($context->self);
$config = Config::getInstance();
$function_context->collect_references = $project_checker->codebase->collect_references;
$function_context->collect_exceptions = $config->check_for_throws_docblock;
$this->function_checkers[$function_id]->analyze($function_context, $context);

$config = Config::getInstance();

if ($config->reportIssueInFile('InvalidReturnType', $this->getFilePath())) {
$method_id = $this->function_checkers[$function_id]->getMethodId();

Expand Down
10 changes: 10 additions & 0 deletions src/Psalm/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ class Config
*/
public $add_param_default_to_docblock_type = false;

/**
* @var bool
*/
public $check_for_throws_docblock = false;

/**
* @var string[]
*/
Expand Down Expand Up @@ -529,6 +534,11 @@ public static function loadFromXML($base_dir, $file_contents)
$config->add_param_default_to_docblock_type = $attribute_text === 'true' || $attribute_text === '1';
}

if (isset($config_xml['checkForThrowsDocblock'])) {
$attribute_text = (string) $config_xml['checkForThrowsDocblock'];
$config->check_for_throws_docblock = $attribute_text === 'true' || $attribute_text === '1';
}

if (isset($config_xml->projectFiles)) {
$config->project_files = ProjectFileFilter::loadFromXMLElement($config_xml->projectFiles, $base_dir, true);
}
Expand Down
14 changes: 14 additions & 0 deletions src/Psalm/Context.php
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,13 @@ class Context
*/
public $collect_references = false;

/**
* Whether or not to track exceptions
*
* @var bool
*/
public $collect_exceptions = false;

/**
* A list of variables that have been referenced
*
Expand Down Expand Up @@ -194,6 +201,13 @@ class Context
*/
public $possibly_assigned_var_ids = [];

/**
* A list of classes or interfaces that may have been thrown
*
* @var array<string, bool>
*/
public $possibly_thrown_exceptions = [];

/**
* @var bool
*/
Expand Down
Loading

0 comments on commit e3ae1bf

Please sign in to comment.