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

Feature: introduce option to disallow integers for string placeholders #90

Merged
merged 3 commits into from
May 19, 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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Experimental features can be enabled by extending the plugin configuration as fo
```xml
<pluginClass class="Boesing\PsalmPluginStringf\Plugin">
<experimental>
<ReportPossiblyInvalidArgumentForSpecifier/>
<ReportPossiblyInvalidArgumentForSpecifier allowIntegerForString="yes" />
</experimental>
</pluginClass>
```
Expand Down
176 changes: 88 additions & 88 deletions composer.lock

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

final class PossiblyInvalidArgumentForSpecifierValidator implements AfterEveryFunctionCallAnalysisInterface
{
public static bool $allowIntegerForStringPlaceholder = true;

private const FUNCTIONS = [
'sprintf',
'printf',
Expand Down Expand Up @@ -95,7 +97,8 @@ public function assert(
$this->functionName,
$template,
$context,
$phpVersion
$phpVersion,
self::$allowIntegerForStringPlaceholder
);
} catch (InvalidArgumentException $exception) {
return;
Expand Down Expand Up @@ -196,4 +199,16 @@ private function invalidTypeWouldBeCoveredByPsalmItself(Atomic $type): bool

return true;
}

/**
* @param array<non-empty-string,string> $options
*/
public static function applyOptions(array $options): void
{
if (! isset($options['allowIntegerForString']) || $options['allowIntegerForString'] === 'yes') {
return;
}

self::$allowIntegerForStringPlaceholder = false;
}
}
3 changes: 2 additions & 1 deletion src/EventHandler/SprintfFunctionReturnProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
$functionName,
$templateArgument,
$context,
PhpVersion::fromStatementSource($event->getStatementsSource())
PhpVersion::fromStatementSource($event->getStatementsSource()),
false
);
} catch (InvalidArgumentException $exception) {
return null;
Expand Down
3 changes: 2 additions & 1 deletion src/EventHandler/StringfFunctionArgumentValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ private function validate(
$this->functionName,
$template,
$context,
$phpVersion
$phpVersion,
false
);
} catch (InvalidArgumentException $exception) {
return;
Expand Down
3 changes: 2 additions & 1 deletion src/EventHandler/UnnecessaryFunctionCallValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ private function assert(
$this->functionName,
$template,
$context,
$phpVersion
$phpVersion,
false
);
} catch (InvalidArgumentException $exception) {
return;
Expand Down
21 changes: 12 additions & 9 deletions src/Parser/TemplatedStringParser/Placeholder.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,28 @@ final class Placeholder

private ?Union $type;

private bool $allowIntegerForStringPlaceholder;

/**
* @psalm-param non-empty-string $value
* @psalm-param positive-int $position
*/
private function __construct(string $value, int $position)
private function __construct(string $value, int $position, bool $allowIntegerForStringPlaceholder)
{
$this->value = $value;
$this->position = $position;
$this->argumentValueType = null;
$this->type = null;
$this->value = $value;
$this->position = $position;
$this->argumentValueType = null;
$this->type = null;
$this->allowIntegerForStringPlaceholder = $allowIntegerForStringPlaceholder;
}

/**
* @psalm-param non-empty-string $value
* @psalm-param positive-int $position
*/
public static function create(string $value, int $position): self
public static function create(string $value, int $position, bool $allowIntegerForStringPlaceholder): self
{
return new self($value, $position);
return new self($value, $position, $allowIntegerForStringPlaceholder);
}

/**
Expand Down Expand Up @@ -139,13 +142,13 @@ public function getSuggestedType(): ?Union
}

try {
$type = SpecifierTypeGenerator::create($this->value)->getSuggestedType();
$type = SpecifierTypeGenerator::create($this->value, $this->allowIntegerForStringPlaceholder)->getSuggestedType();
} catch (InvalidArgumentException $exception) {
return null;
}

if ($this->repeated === []) {
return $type;
return $this->type = $type;
}

$unions = [$type];
Expand Down
26 changes: 21 additions & 5 deletions src/Parser/TemplatedStringParser/SpecifierTypeGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,30 @@ final class SpecifierTypeGenerator
/** @psalm-var non-empty-string */
private string $specifier;

private bool $allowIntegerForStringPlaceholder;

/**
* @psalm-param non-empty-string $specifier
*/
private function __construct(string $specifier)
private function __construct(string $specifier, bool $allowIntegerForStringPlaceholder)
{
$this->specifier = $this->parse($specifier);
$this->specifier = $this->parse($specifier);
$this->allowIntegerForStringPlaceholder = $allowIntegerForStringPlaceholder;
}

/**
* @psalm-param non-empty-string $specifier
*/
public static function create(string $specifier): self
public static function create(string $specifier, bool $allowIntegerForStringPlaceholder): self
{
return new self($specifier);
return new self($specifier, $allowIntegerForStringPlaceholder);
}

public function getSuggestedType(): Type\Union
{
switch ($this->specifier) {
case 's':
return Type::getString();
return $this->stringable();

case 'd':
case 'f':
Expand Down Expand Up @@ -80,4 +83,17 @@ private function numeric(): Type\Union
new Type\Atomic\TNumericString(),
]);
}

private function stringable(): Type\Union
{
$types = [
new Type\Atomic\TString(),
];

if ($this->allowIntegerForStringPlaceholder) {
$types[] = new Type\Atomic\TInt();
}

return new Type\Union($types);
}
}
20 changes: 14 additions & 6 deletions src/Parser/TemplatedStringParser/TemplatedStringParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,24 +42,29 @@ final class TemplatedStringParser

private string $template;

private bool $allowIntegerForStringPlaceholder;

/**
* @psalm-param positive-int $phpVersion
*/
private function __construct(
string $functionName,
string $template,
int $phpVersion
int $phpVersion,
bool $allowIntegerForStringPlaceholder
) {
$this->template = $template;
$this->templateWithoutPlaceholder = $template;
$this->placeholders = [];
$this->parse($functionName, $template, $phpVersion);
$this->parse($functionName, $template, $phpVersion, $allowIntegerForStringPlaceholder);
$this->allowIntegerForStringPlaceholder = $allowIntegerForStringPlaceholder;
}

private function parse(
string $functionName,
string $template,
int $phpVersion
int $phpVersion,
bool $allowIntegerForStringPlaceholder
): void {
$additionalSpecifierDependingOnPhpVersion = '';
if ($phpVersion >= 80000) {
Expand Down Expand Up @@ -133,7 +138,8 @@ static function (
$initialPlaceholderInstance = $placeholderInstances[$placeholderPosition] ?? null;
$placeholderInstance = Placeholder::create(
$placeholderValue,
$placeholderPosition
$placeholderPosition,
$allowIntegerForStringPlaceholder
);

if ($initialPlaceholderInstance !== null) {
Expand All @@ -153,12 +159,14 @@ public static function fromArgument(
string $functionName,
Arg $templateArgument,
Context $context,
int $phpVersion
int $phpVersion,
bool $allowIntegerForStringPlaceholder
): self {
return new self(
$functionName,
ArgumentValueParser::create($templateArgument->value, $context)->toString(),
$phpVersion
$phpVersion,
$allowIntegerForStringPlaceholder
);
}

Expand Down
38 changes: 34 additions & 4 deletions src/Plugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,25 @@ private function registerExperimentalHooks(RegistrationInterface $registration,
}

foreach ($config->experimental->children() as $element) {
assert($element !== null);
assert($element instanceof SimpleXMLElement);
$name = $element->getName();
if (! isset(self::EXPERIMENTAL_FEATURES[$name])) {
continue;
}

$this->registerFeatureHook($registration, $name);
$options = $this->extractOptionsFromElement($element);
$this->registerFeatureHook($registration, $name, $options);
}
}

private function registerFeatureHook(RegistrationInterface $registration, string $featureName): void
{
/**
* @param array<non-empty-string,string> $options
*/
private function registerFeatureHook(
RegistrationInterface $registration,
string $featureName,
array $options
): void {
$eventHandlerClassName = self::EXPERIMENTAL_FEATURES[$featureName];

$fileName = __DIR__ . sprintf(
Expand All @@ -68,5 +75,28 @@ private function registerFeatureHook(RegistrationInterface $registration, string
require_once $fileName;

$registration->registerHooksFromClass($eventHandlerClassName);
if ($eventHandlerClassName !== PossiblyInvalidArgumentForSpecifierValidator::class) {
return;
}

$eventHandlerClassName::applyOptions($options);
}

/**
* @return array<non-empty-string,string>
*/
private function extractOptionsFromElement(SimpleXMLElement $element): array
{
$options = [];

foreach ($element->attributes() ?? [] as $attribute) {
assert($attribute instanceof SimpleXMLElement);
$name = $attribute->getName();
assert($name !== '');

$options[$name] = (string) $attribute;
}

return $options;
}
}
3 changes: 1 addition & 2 deletions tests/acceptance/PrintfArgumentType.feature
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ Feature: printf argument type verification
When I run Psalm
Then I see these errors
| Type | Message |
| PossiblyInvalidArgument | Argument 1 inferred as "1" does not match (any of) the suggested type(s) "string" |
| PossiblyInvalidArgument | Argument 1 inferred as "float(1.035)" does not match (any of) the suggested type(s) "string" |
| PossiblyInvalidArgument | Argument 1 inferred as "float(1.035)" does not match (any of) the suggested type(s) "int\|string" |
And I see no other errors

Scenario: template requires double but invalid values are passed
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
Feature: Integer is not allowed to be passed to printf string placeholder when option is set

Background:
Given I have the following config
"""
<?xml version="1.0"?>
<psalm errorLevel="1" findUnusedPsalmSuppress="true">
<projectFiles>
<directory name="."/>
</projectFiles>
<issueHandlers>
<UnusedFunctionCall>
<errorLevel type="suppress">
<directory name="."/>
</errorLevel>
</UnusedFunctionCall>
</issueHandlers>
<plugins>
<pluginClass class="Boesing\PsalmPluginStringf\Plugin">
<experimental>
<ReportPossiblyInvalidArgumentForSpecifier allowIntegerForString="no"/>
</experimental>
</pluginClass>
</plugins>
</psalm>
"""
And I have the following code preamble
"""
<?php declare(strict_types=1);

"""

Scenario: template requires string but integer is passed
Given I have the following code
"""
printf('%s', 1);
sprintf('%s', 1);
"""
When I run Psalm
Then I see these errors
| Type | Message |
| PossiblyInvalidArgument | Argument 1 inferred as "1" does not match (any of) the suggested type(s) "string" |
| PossiblyInvalidArgument | Argument 1 inferred as "1" does not match (any of) the suggested type(s) "string" |
And I see no other errors