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 @psalm-internal and prevent usage of IssueBuffer::add(). #8165

Merged
merged 1 commit into from
Jun 25, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/Psalm/Internal/Analyzer/ClassAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ public function __construct(PhpParser\Node\Stmt $class, SourceAnalyzer $source,
}
}

/** @return non-empty-string */
public static function getAnonymousClassName(PhpParser\Node\Stmt\Class_ $class, string $file_path): string
{
return preg_replace('/[^A-Za-z0-9]/', '_', $file_path)
Expand Down Expand Up @@ -251,7 +252,7 @@ public function analyze(
}

foreach ($storage->docblock_issues as $docblock_issue) {
IssueBuffer::add($docblock_issue);
IssueBuffer::maybeAdd($docblock_issue);
}

$classlike_storage_provider = $codebase->classlike_storage_provider;
Expand Down Expand Up @@ -1647,7 +1648,7 @@ private function analyzeClassMethod(
$config = Config::getInstance();

if ($stmt->stmts === null && !$stmt->isAbstract()) {
IssueBuffer::add(
IssueBuffer::maybeAdd(
new ParseError(
'Non-abstract class method must have statements',
new CodeLocation($this, $stmt)
Expand All @@ -1660,7 +1661,7 @@ private function analyzeClassMethod(
try {
$method_analyzer = new MethodAnalyzer($stmt, $source);
} catch (UnexpectedValueException $e) {
IssueBuffer::add(
IssueBuffer::maybeAdd(
new ParseError(
'Problem loading method: ' . $e->getMessage(),
new CodeLocation($this, $stmt)
Expand Down Expand Up @@ -2506,11 +2507,12 @@ private function checkParentClass(
);
}

if (!NamespaceAnalyzer::isWithin($fq_class_name, $parent_class_storage->internal)) {
if (!NamespaceAnalyzer::isWithinAny($fq_class_name, $parent_class_storage->internal)) {
IssueBuffer::maybeAdd(
new InternalClass(
$parent_fq_class_name . ' is internal to ' . $parent_class_storage->internal
. ' but called from ' . $fq_class_name,
$parent_fq_class_name . ' is internal to '
. InternalClass::listToPhrase($parent_class_storage->internal)
. ' but called from ' . $fq_class_name,
$code_location,
$parent_fq_class_name
),
Expand Down
11 changes: 2 additions & 9 deletions src/Psalm/Internal/Analyzer/CommentAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Psalm\Exception\IncorrectDocblockException;
use Psalm\Exception\TypeParseTreeException;
use Psalm\FileSource;
use Psalm\Internal\Scanner\DocblockParser;
use Psalm\Internal\Scanner\ParsedDocblock;
use Psalm\Internal\Scanner\VarDocblockComment;
use Psalm\Internal\Type\TypeAlias;
Expand All @@ -21,7 +22,6 @@
use function preg_match;
use function preg_replace;
use function preg_split;
use function reset;
use function rtrim;
use function str_replace;
use function strlen;
Expand Down Expand Up @@ -236,14 +236,7 @@ private static function decorateVarDocblockComment(
}
}

if (isset($parsed_docblock->tags['psalm-internal'])) {
$psalm_internal = trim(reset($parsed_docblock->tags['psalm-internal']));

if (!$psalm_internal) {
throw new DocblockParseException('psalm-internal annotation used without specifying namespace');
}

$var_comment->psalm_internal = $psalm_internal;
if (count($var_comment->psalm_internal = DocblockParser::handlePsalmInternal($parsed_docblock)) !== 0) {
$var_comment->internal = true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/FileAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public function analyze(
$statements_analyzer = new StatementsAnalyzer($this, $this->node_data);

foreach ($file_storage->docblock_issues as $docblock_issue) {
IssueBuffer::add($docblock_issue);
IssueBuffer::maybeAdd($docblock_issue);
}

// if there are any leftover statements, evaluate them,
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ public function analyze(
}

foreach ($storage->docblock_issues as $docblock_issue) {
IssueBuffer::add($docblock_issue);
IssueBuffer::maybeAdd($docblock_issue);
}

$function_information = $this->getFunctionInformation(
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public function analyze(): void
);
}
} elseif ($stmt instanceof PhpParser\Node\Stmt\Property) {
IssueBuffer::add(
IssueBuffer::maybeAdd(
new ParseError(
'Interfaces cannot have properties',
new CodeLocation($this, $stmt)
Expand Down
120 changes: 108 additions & 12 deletions src/Psalm/Internal/Analyzer/NamespaceAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
use ReflectionProperty;
use UnexpectedValueException;

use function assert;
use function count;
use function implode;
use function preg_replace;
use function strpos;
use function strtolower;
use function trim;
use function substr;

/**
* @internal
Expand Down Expand Up @@ -152,33 +154,127 @@ public function getFileAnalyzer(): FileAnalyzer
}

/**
* Returns true if $className is the same as, or starts with $namespace, in a case-insensitive comparison.
* Returns true if $calling_identifier is the same as, or is within with $identifier, in a
* case-insensitive comparison. Identifiers can be namespaces, classlikes, functions, or methods.
*
* @psalm-pure
*
* @throws InvalidArgumentException if $identifier is not a valid identifier
*/
public static function isWithin(string $calling_identifier, string $identifier): bool
{
$normalized_calling_ident = self::normalizeIdentifier($calling_identifier);
$normalized_ident = self::normalizeIdentifier($identifier);

if ($normalized_calling_ident === $normalized_ident) {
return true;
}

$normalized_calling_ident_parts = self::getIdentifierParts($normalized_calling_ident);
$normalized_ident_parts = self::getIdentifierParts($normalized_ident);

if (count($normalized_calling_ident_parts) < count($normalized_ident_parts)) {
return false;
}

for ($i = 0; $i < count($normalized_ident_parts); ++$i) {
if ($normalized_ident_parts[$i] !== $normalized_calling_ident_parts[$i]) {
return false;
}
}

return true;
}

/**
* Returns true if $calling_identifier is the same as or is within any identifier
* in $identifiers in a case-insensitive comparison, or if $identifiers is empty.
* Identifiers can be namespaces, classlikes, functions, or methods.
*
* @psalm-pure
*
* @psalm-assert-if-false !empty $identifiers
*
* @param list<string> $identifiers
*/
public static function isWithin(string $calling_namespace, string $namespace): bool
public static function isWithinAny(string $calling_identifier, array $identifiers): bool
{
if ($namespace === '') {
return true; // required to prevent a warning from strpos with empty needle in PHP < 8
if (count($identifiers) === 0) {
return true;
}

$calling_namespace = strtolower(trim($calling_namespace, '\\') . '\\');
$namespace = strtolower(trim($namespace, '\\') . '\\');
foreach ($identifiers as $identifier) {
if (self::isWithin($calling_identifier, $identifier)) {
return true;
}
}

return $calling_namespace === $namespace
|| strpos($calling_namespace, $namespace) === 0;
return false;
}

/**
* @param string $fullyQualifiedClassName, e.g. '\Psalm\Internal\Analyzer\NamespaceAnalyzer'
* @param non-empty-string $fullyQualifiedClassName, e.g. '\Psalm\Internal\Analyzer\NamespaceAnalyzer'
*
* @return string , e.g. 'Psalm'
* @return non-empty-string , e.g. 'Psalm'
*
* @psalm-pure
*/
public static function getNameSpaceRoot(string $fullyQualifiedClassName): string
{
return preg_replace('/^([^\\\]+).*/', '$1', $fullyQualifiedClassName);
$root_namespace = preg_replace('/^([^\\\]+).*/', '$1', $fullyQualifiedClassName);
if ($root_namespace === "") {
throw new InvalidArgumentException("Invalid classname \"$fullyQualifiedClassName\"");
}
return $root_namespace;
}

/**
* @return ($lowercase is true ? lowercase-string : string)
*
* @psalm-pure
*/
public static function normalizeIdentifier(string $identifier, bool $lowercase = true): string
{
if ($identifier === "") {
return "";
}

$identifier = $identifier[0] === "\\" ? substr($identifier, 1) : $identifier;
return $lowercase ? strtolower($identifier) : $identifier;
}

/**
* Splits an identifier into parts, eg `Foo\Bar::baz` becomes ["Foo", "\\", "Bar", "::", "baz"].
*
* @return list<non-empty-string>
*
* @psalm-pure
*/
public static function getIdentifierParts(string $identifier): array
{
$parts = [];
while (($pos = strpos($identifier, "\\")) !== false) {
if ($pos > 0) {
$part = substr($identifier, 0, $pos);
assert($part !== "");
$parts[] = $part;
}
$parts[] = "\\";
$identifier = substr($identifier, $pos + 1);
}
if (($pos = strpos($identifier, "::")) !== false) {
if ($pos > 0) {
$part = substr($identifier, 0, $pos);
assert($part !== "");
$parts[] = $part;
}
$parts[] = "::";
$identifier = substr($identifier, $pos + 2);
}
if ($identifier !== "") {
$parts[] = $identifier;
}

return $parts;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public static function analyze(

foreach ($stmt->items as $item) {
if ($item === null) {
IssueBuffer::add(
IssueBuffer::maybeAdd(
new ParseError(
'Array element cannot be empty',
new CodeLocation($statements_analyzer, $stmt)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,7 @@ protected static function processCustomAssertion(
}
} elseif ($assertion->var_id === '$this') {
if (!$expr instanceof PhpParser\Node\Expr\MethodCall) {
IssueBuffer::add(
IssueBuffer::maybeAdd(
new InvalidDocblock(
'Assertion of $this can be done only on method of a class',
new CodeLocation($source, $expr)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use Psalm\Issue\ImplicitToStringCast;
use Psalm\Issue\ImpurePropertyAssignment;
use Psalm\Issue\InaccessibleProperty;
use Psalm\Issue\InternalClass;
use Psalm\Issue\InternalProperty;
use Psalm\Issue\InvalidPropertyAssignment;
use Psalm\Issue\InvalidPropertyAssignmentValue;
Expand Down Expand Up @@ -420,7 +421,7 @@ public static function analyzeStatement(
foreach ($stmt->props as $prop) {
if ($prop->default) {
if ($stmt->isReadonly()) {
IssueBuffer::add(
IssueBuffer::maybeAdd(
new InvalidPropertyAssignment(
'Readonly property ' . $context->self . '::$' . $prop->name->name
. ' cannot have a default',
Expand Down Expand Up @@ -1258,11 +1259,11 @@ private static function analyzeAtomicAssignment(
);
}

if ($context->self && !NamespaceAnalyzer::isWithin($context->self, $property_storage->internal)) {
if ($context->self && !NamespaceAnalyzer::isWithinAny($context->self, $property_storage->internal)) {
IssueBuffer::maybeAdd(
new InternalProperty(
$property_id . ' is internal to ' . $property_storage->internal
. ' but called from ' . $context->self,
$property_id . ' is internal to ' . InternalClass::listToPhrase($property_storage->internal)
. ' but called from ' . $context->self,
new CodeLocation($statements_analyzer->getSource(), $stmt),
$property_id
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public static function analyze(
$codebase,
$context,
$method_id,
$statements_analyzer->getNamespace(),
$statements_analyzer->getFullyQualifiedFunctionMethodOrNamespaceName(),
$name_code_location,
$statements_analyzer->getSuppressedIssues()
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Psalm\Internal\Analyzer\NamespaceAnalyzer;
use Psalm\Internal\MethodIdentifier;
use Psalm\Issue\DeprecatedMethod;
use Psalm\Issue\InternalClass;
use Psalm\Issue\InternalMethod;
use Psalm\IssueBuffer;

Expand All @@ -22,7 +23,7 @@ public static function analyze(
Codebase $codebase,
Context $context,
MethodIdentifier $method_id,
?string $namespace,
?string $caller_identifier,
CodeLocation $code_location,
array $suppressed_issues
): ?bool {
Expand Down Expand Up @@ -51,12 +52,12 @@ public static function analyze(
if (!$context->collect_initializations
&& !$context->collect_mutations
) {
if (!NamespaceAnalyzer::isWithin($namespace ?: '', $storage->internal)) {
if (!NamespaceAnalyzer::isWithinAny($caller_identifier ?? "", $storage->internal)) {
IssueBuffer::maybeAdd(
new InternalMethod(
'The method ' . $codebase_methods->getCasedMethodId($method_id)
. ' is internal to ' . $storage->internal
. ' but called from ' . ($context->self ?: 'root namespace'),
. ' is internal to ' . InternalClass::listToPhrase($storage->internal)
. ' but called from ' . ($caller_identifier ?: 'root namespace'),
$code_location,
(string) $method_id
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -349,12 +349,12 @@ private static function analyzeNamedConstructor(
if ($context->self
&& !$context->collect_initializations
&& !$context->collect_mutations
&& !NamespaceAnalyzer::isWithin($context->self, $storage->internal)
&& !NamespaceAnalyzer::isWithinAny($context->self, $storage->internal)
) {
IssueBuffer::maybeAdd(
new InternalClass(
$fq_class_name . ' is internal to ' . $storage->internal
. ' but called from ' . $context->self,
$fq_class_name . ' is internal to ' . InternalClass::listToPhrase($storage->internal)
. ' but called from ' . $context->self,
new CodeLocation($statements_analyzer->getSource(), $stmt),
$fq_class_name
),
Expand Down Expand Up @@ -411,16 +411,13 @@ private static function analyzeNamedConstructor(
if ($declaring_method_id) {
$method_storage = $codebase->methods->getStorage($declaring_method_id);

$namespace = $statements_analyzer->getNamespace() ?: '';
if (!NamespaceAnalyzer::isWithin(
$namespace,
$method_storage->internal
)) {
$caller_identifier = $statements_analyzer->getFullyQualifiedFunctionMethodOrNamespaceName() ?: '';
if (!NamespaceAnalyzer::isWithinAny($caller_identifier, $method_storage->internal)) {
IssueBuffer::maybeAdd(
new InternalMethod(
'Constructor ' . $codebase->methods->getCasedMethodId($declaring_method_id)
. ' is internal to ' . $method_storage->internal
. ' but called from ' . ($namespace ?: 'root namespace'),
. ' is internal to ' . InternalClass::listToPhrase($method_storage->internal)
. ' but called from ' . ($caller_identifier ?: 'root namespace'),
new CodeLocation($statements_analyzer, $stmt),
(string) $method_id
),
Expand Down
Loading