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

Improve class constant static analysis #7154

Merged
merged 3 commits into from Jan 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 8 additions & 5 deletions config.xsd
Expand Up @@ -198,9 +198,9 @@

<xs:complexType name="IssueHandlersType">
<xs:choice minOccurs="0" maxOccurs="unbounded">
<xs:element name="PluginIssue" type="PluginIssueHandlerType" minOccurs="0" />
<xs:element name="AbstractInstantiation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="AbstractMethodCall" type="IssueHandlerType" minOccurs="0" />
<xs:element name="AmbiguousConstantInheritance" type="ClassConstantIssueHandlerType" minOccurs="0" />
<xs:element name="ArgumentTypeCoercion" type="ArgumentIssueHandlerType" minOccurs="0" />
<xs:element name="AssignmentToVoid" type="IssueHandlerType" minOccurs="0" />
<xs:element name="CircularReference" type="IssueHandlerType" minOccurs="0" />
Expand Down Expand Up @@ -292,6 +292,7 @@
<xs:element name="InvalidToString" type="IssueHandlerType" minOccurs="0" />
<xs:element name="InvalidTraversableImplementation" type="ClassIssueHandlerType" minOccurs="0" />
<xs:element name="InvalidTypeImport" type="IssueHandlerType" minOccurs="0" />
<xs:element name="LessSpecificClassConstantType" type="ClassConstantIssueHandlerType" minOccurs="0" />
<xs:element name="LessSpecificImplementedReturnType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="LessSpecificReturnStatement" type="IssueHandlerType" minOccurs="0" />
<xs:element name="LessSpecificReturnType" type="IssueHandlerType" minOccurs="0" />
Expand Down Expand Up @@ -338,10 +339,10 @@
<xs:element name="NamedArgumentNotAllowed" type="ArgumentIssueHandlerType" minOccurs="0" />
<xs:element name="NoEnumProperties" type="ClassIssueHandlerType" minOccurs="0" />
<xs:element name="NoInterfaceProperties" type="ClassIssueHandlerType" minOccurs="0" />
<xs:element name="NonStaticSelfCall" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NoValue" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NonInvariantDocblockPropertyType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NonInvariantPropertyType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NonStaticSelfCall" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NoValue" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NullableReturnStatement" type="IssueHandlerType" minOccurs="0" />
<xs:element name="NullArgument" type="ArgumentIssueHandlerType" minOccurs="0" />
<xs:element name="NullArrayAccess" type="IssueHandlerType" minOccurs="0" />
Expand All @@ -352,11 +353,13 @@
<xs:element name="NullPropertyAssignment" type="PropertyIssueHandlerType" minOccurs="0" />
<xs:element name="NullPropertyFetch" type="PropertyIssueHandlerType" minOccurs="0" />
<xs:element name="NullReference" type="IssueHandlerType" minOccurs="0" />
<xs:element name="OverriddenInterfaceConstant" type="ClassConstantIssueHandlerType" minOccurs="0" />
<xs:element name="OverriddenMethodAccess" type="IssueHandlerType" minOccurs="0" />
<xs:element name="OverriddenPropertyAccess" type="PropertyIssueHandlerType" minOccurs="0" />
<xs:element name="ParadoxicalCondition" type="IssueHandlerType" minOccurs="0" />
<xs:element name="ParamNameMismatch" type="IssueHandlerType" minOccurs="0" />
<xs:element name="ParentNotFound" type="IssueHandlerType" minOccurs="0" />
<xs:element name="PluginIssue" type="PluginIssueHandlerType" minOccurs="0" />
<xs:element name="PossibleRawObjectIteration" type="IssueHandlerType" minOccurs="0" />
<xs:element name="PossiblyFalseArgument" type="ArgumentIssueHandlerType" minOccurs="0" />
<xs:element name="PossiblyFalseIterator" type="IssueHandlerType" minOccurs="0" />
Expand Down Expand Up @@ -407,8 +410,8 @@
<xs:element name="RedundantConditionGivenDocblockType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="RedundantFunctionCall" type="IssueHandlerType" minOccurs="0" />
<xs:element name="RedundantFunctionCallGivenDocblockType" type="IssueHandlerType" minOccurs="0" />
<xs:element name="RedundantPropertyInitializationCheck" type="IssueHandlerType" minOccurs="0" />
<xs:element name="RedundantIdentityWithTrue" type="IssueHandlerType" minOccurs="0" />
<xs:element name="RedundantPropertyInitializationCheck" type="IssueHandlerType" minOccurs="0" />
<xs:element name="ReferenceConstraintViolation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="ReservedWord" type="IssueHandlerType" minOccurs="0" />
<xs:element name="StringIncrement" type="IssueHandlerType" minOccurs="0" />
Expand Down Expand Up @@ -466,8 +469,8 @@
<xs:element name="UnrecognizedStatement" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnresolvableConstant" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnresolvableInclude" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnsafeInstantiation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnsafeGenericInstantiation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnsafeInstantiation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnusedClass" type="ClassIssueHandlerType" minOccurs="0" />
<xs:element name="UnusedClosureParam" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnusedConstructor" type="MethodIssueHandlerType" minOccurs="0" />
Expand Down
3 changes: 3 additions & 0 deletions docs/running_psalm/issues.md
Expand Up @@ -2,6 +2,7 @@

- [AbstractInstantiation](issues/AbstractInstantiation.md)
- [AbstractMethodCall](issues/AbstractMethodCall.md)
- [AmbiguousConstantInheritance](issues/AmbiguousConstantInheritance.md)
- [ArgumentTypeCoercion](issues/ArgumentTypeCoercion.md)
- [AssignmentToVoid](issues/AssignmentToVoid.md)
- [CircularReference](issues/CircularReference.md)
Expand Down Expand Up @@ -93,6 +94,7 @@
- [InvalidToString](issues/InvalidToString.md)
- [InvalidTraversableImplementation](issues/InvalidTraversableImplementation.md)
- [InvalidTypeImport](issues/InvalidTypeImport.md)
- [LessSpecificClassConstantType](issues/LessSpecificClassConstantType.md)
- [LessSpecificImplementedReturnType](issues/LessSpecificImplementedReturnType.md)
- [LessSpecificReturnStatement](issues/LessSpecificReturnStatement.md)
- [LessSpecificReturnType](issues/LessSpecificReturnType.md)
Expand Down Expand Up @@ -153,6 +155,7 @@
- [NullPropertyAssignment](issues/NullPropertyAssignment.md)
- [NullPropertyFetch](issues/NullPropertyFetch.md)
- [NullReference](issues/NullReference.md)
- [OverriddenInterfaceConstant](issues/OverriddenInterfaceConstant.md)
- [OverriddenMethodAccess](issues/OverriddenMethodAccess.md)
- [OverriddenPropertyAccess](issues/OverriddenPropertyAccess.md)
- [ParadoxicalCondition](issues/ParadoxicalCondition.md)
Expand Down
41 changes: 41 additions & 0 deletions docs/running_psalm/issues/AmbiguousConstantInheritance.md
@@ -0,0 +1,41 @@
# AmbiguousConstantInheritance

Emitted when a constant is inherited from multiple sources.

```php
<?php

interface Foo
{
/** @var non-empty-string */
public const CONSTANT='foo';
}

interface Bar
{
/**
* @var non-empty-string
*/
public const CONSTANT='bar';
}

interface Baz extends Foo, Bar {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an issue when the type are compatibles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an issue even when it's the same exact thing: https://3v4l.org/tjYML

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I thought "ambiguous" was too tame a word for something that broke at compile time, I was wrong!

```

```php
<?php

interface Foo
{
/** @var non-empty-string */
public const CONSTANT='foo';
}

class Bar
{
/** @var non-empty-string */
public const CONSTANT='bar';
}

class Baz extends Bar implements Foo {}
```
28 changes: 28 additions & 0 deletions docs/running_psalm/issues/LessSpecificClassConstantType.md
@@ -0,0 +1,28 @@
# LessSpecificClassConstantType

Emitted when a constant type in a child is less specific than the type in the parent.

```php
<?php

class Foo
{
/** @var int<1,max> */
public const CONSTANT = 3;

public static function bar(): array
{
return str_split("foobar", static::CONSTANT);
}
}

class Bar extends Foo
{
/** @var int */
public const CONSTANT = -1;
}

Bar::bar(); // Error: str_split argument 2 must be greater than 0
```

This issue will always show up when overriding a constant that doesn't have a docblock type. Psalm will infer the most specific type for the constant that it can, you have to add a type annotation to tell it what type constraint you wish to be applied. Otherwise Psalm has no way of telling if you mean for the constant to be a literal `1`, `int<1, max>`, `int`, `numeric`, etc.
17 changes: 17 additions & 0 deletions docs/running_psalm/issues/OverriddenInterfaceConstant.md
@@ -0,0 +1,17 @@
# OverriddenInterfaceConstant

Emitted when a constant declared on an interface is overridden by a child (illegal in PHP < 8.1).

```php
<?php

interface Foo
{
public const BAR='baz';
}

interface Bar extends Foo
{
public const BAR='foobar';
}
```
3 changes: 3 additions & 0 deletions src/Psalm/Internal/Analyzer/ClassAnalyzer.php
Expand Up @@ -17,6 +17,7 @@
use Psalm\FileManipulation;
use Psalm\Internal\Analyzer\FunctionLike\ReturnTypeAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\Call\ClassTemplateParamCollector;
use Psalm\Internal\Analyzer\Statements\Expression\ClassConstAnalyzer;
use Psalm\Internal\Analyzer\Statements\Expression\Fetch\AtomicPropertyFetchAnalyzer;
use Psalm\Internal\FileManipulation\FileManipulationBuffer;
use Psalm\Internal\FileManipulation\PropertyDocblockManipulator;
Expand Down Expand Up @@ -533,6 +534,8 @@ public function analyze(
$statements_analyzer = new StatementsAnalyzer($this, new NodeDataProvider());
$statements_analyzer->analyze($member_stmts, $class_context, $global_context, true);

ClassConstAnalyzer::analyze($storage, $this->getCodebase());

$config = Config::getInstance();

if ($class instanceof PhpParser\Node\Stmt\Class_) {
Expand Down
36 changes: 36 additions & 0 deletions src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php
Expand Up @@ -7,12 +7,17 @@
use PhpParser;
use Psalm\CodeLocation;
use Psalm\Context;
use Psalm\FileManipulation;
use Psalm\Internal\Analyzer\Statements\Expression\ClassConstAnalyzer;
use Psalm\Internal\FileManipulation\FileManipulationBuffer;
use Psalm\Internal\Provider\NodeDataProvider;
use Psalm\Issue\ParseError;
use Psalm\Issue\UndefinedInterface;
use Psalm\IssueBuffer;
use UnexpectedValueException;

use function strtolower;

/**
* @internal
*/
Expand All @@ -32,6 +37,8 @@ public function analyze(): void
throw new LogicException('Something went badly wrong');
}

$interface_context = new Context($this->fq_class_name);

$project_analyzer = $this->file_analyzer->project_analyzer;
$codebase = $project_analyzer->getCodebase();
$config = $project_analyzer->getConfig();
Expand Down Expand Up @@ -106,6 +113,7 @@ public function analyze(): void
);
}

$member_stmts = [];
foreach ($this->class->stmts as $stmt) {
if ($stmt instanceof PhpParser\Node\Stmt\ClassMethod) {
$method_analyzer = new MethodAnalyzer($stmt, $this);
Expand Down Expand Up @@ -141,7 +149,35 @@ public function analyze(): void
);

return;
} elseif ($stmt instanceof PhpParser\Node\Stmt\ClassConst) {
$member_stmts[] = $stmt;

foreach ($stmt->consts as $const) {
$const_id = strtolower($this->fq_class_name) . '::' . $const->name;

foreach ($codebase->class_constants_to_rename as $original_const_id => $new_const_name) {
if ($const_id === $original_const_id) {
$file_manipulations = [
new FileManipulation(
(int) $const->name->getAttribute('startFilePos'),
(int) $const->name->getAttribute('endFilePos') + 1,
$new_const_name
)
];

FileManipulationBuffer::add(
$this->getFilePath(),
$file_manipulations
);
}
}
}
Comment on lines +152 to +174
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied this part from ClassAnalyzer. Haven't tested it, but I assume it's probably fine.

}
}

$statements_analyzer = new StatementsAnalyzer($this, new NodeDataProvider());
$statements_analyzer->analyze($member_stmts, $interface_context, null, true);

ClassConstAnalyzer::analyze($this->storage, $this->getCodebase());
}
}