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

Fix checkbashisms download ans SCA violations #6298

Merged
merged 1 commit into from Feb 18, 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
2 changes: 1 addition & 1 deletion dev-tools/install.sh
Expand Up @@ -20,7 +20,7 @@ cd "$(dirname "$0")"

mkdir -p bin

VERSION_CB="2.21.7"
VERSION_CB="2.21.7~bpo11+1"
VERSION_SC="stable"

echo λλλ checkbashisms
Expand Down
2 changes: 1 addition & 1 deletion src/Console/ConfigurationResolver.php
Expand Up @@ -348,7 +348,7 @@ static function (FixerInterface $fixer): bool {
public function getLinter(): LinterInterface
{
if (null === $this->linter) {
$this->linter = new Linter($this->getConfig()->getPhpExecutable());
$this->linter = new Linter();
}

return $this->linter;
Expand Down
16 changes: 7 additions & 9 deletions src/Doctrine/Annotation/Tokens.php
Expand Up @@ -240,23 +240,21 @@ public function insertAt(int $index, Token $token): void
$this[$index] = $token;
}

/**
* {@inheritdoc}
*
* @throws \InvalidArgumentException
*/
public function offsetSet($index, $token): void
{
// @phpstan-ignore-next-line as we type checking here
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @phpstan-ignore-next-line as we type checking here
/** @var mixed $token */

Also, there's treatPhpDocTypesAsCertain option in PHPStan configuration, which would prevent it from reporting runtime type checks for variables that are typed in phpdocs, although given that the entire project is statically analyzed, I think turning it off would do more harm than good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching it off on raises a 3 items on this PR, maybe it is worth doing that? Not sure....

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. In most methods we trust PHPDoc types. If there are only a few places where we want to validate them at runtime, it's safer to add @var mixed, as that is how we treat this variable in this case. Switching treatPhpDocTypesAsCertain off would prevent PHPStan from reporting truly unnecessary checks that are there by mistake.

if (null === $token) {
throw new \InvalidArgumentException('Token must be an instance of PhpCsFixer\\Doctrine\\Annotation\\Token, "null" given.');
}

if (!$token instanceof Token) {
$type = \gettype($token);

if ('object' === $type) {
$type = \get_class($token);
}

throw new \InvalidArgumentException(sprintf(
'Token must be an instance of PhpCsFixer\\Doctrine\\Annotation\\Token, %s given.',
$type
));
throw new \InvalidArgumentException(sprintf('Token must be an instance of PhpCsFixer\\Doctrine\\Annotation\\Token, "%s" given.', $type));
}

parent::offsetSet($index, $token);
Expand Down
22 changes: 6 additions & 16 deletions src/Linter/Linter.php
Expand Up @@ -23,44 +23,34 @@
*/
final class Linter implements LinterInterface
{
/**
* @var LinterInterface
*/
private $sublinter;
private LinterInterface $subLinter;

/**
* @param null|string $executable PHP executable, null for autodetection
*/
public function __construct(?string $executable = null)
public function __construct()
{
try {
$this->sublinter = new TokenizerLinter();
} catch (UnavailableLinterException $e) {
$this->sublinter = new ProcessLinter($executable);
}
$this->subLinter = new TokenizerLinter();
}

/**
* {@inheritdoc}
*/
public function isAsync(): bool
{
return $this->sublinter->isAsync();
return $this->subLinter->isAsync();
}

/**
* {@inheritdoc}
*/
public function lintFile(string $path): LintingResultInterface
{
return $this->sublinter->lintFile($path);
return $this->subLinter->lintFile($path);
}

/**
* {@inheritdoc}
*/
public function lintSource(string $source): LintingResultInterface
{
return $this->sublinter->lintSource($source);
return $this->subLinter->lintSource($source);
}
}
8 changes: 2 additions & 6 deletions src/Tokenizer/Analyzer/Analysis/ArgumentAnalysis.php
Expand Up @@ -31,17 +31,13 @@ final class ArgumentAnalysis

/**
* The default value of the argument.
*
* @var null|string
*/
private $default;
private ?string $default;

/**
* The type analysis of the argument.
*
* @var ?TypeAnalysis
*/
private $typeAnalysis;
private ?TypeAnalysis $typeAnalysis;

public function __construct(string $name, int $nameIndex, ?string $default, ?TypeAnalysis $typeAnalysis = null)
{
Expand Down
5 changes: 1 addition & 4 deletions src/Tokenizer/Analyzer/Analysis/MatchAnalysis.php
Expand Up @@ -19,10 +19,7 @@
*/
final class MatchAnalysis extends AbstractControlCaseStructuresAnalysis
{
/**
* @var null|DefaultAnalysis
*/
private $defaultAnalysis;
private ?DefaultAnalysis $defaultAnalysis;

public function __construct(int $index, int $open, int $close, ?DefaultAnalysis $defaultAnalysis)
{
Expand Down
5 changes: 1 addition & 4 deletions src/Tokenizer/Analyzer/Analysis/TypeAnalysis.php
Expand Up @@ -47,10 +47,7 @@ final class TypeAnalysis implements StartEndTokenAwareAnalysis
'void',
];

/**
* @var string
*/
private $name;
private string $name;

private int $startIndex;

Expand Down
30 changes: 30 additions & 0 deletions tests/Doctrine/Annotation/TokensTest.php
Expand Up @@ -35,4 +35,34 @@ public function testCreateFromEmptyPhpdocComment(): void
static::assertCount(1, $tokens);
static::assertSame($docComment, $tokens->getCode());
}

/**
* @dataProvider provideOffSetOtherThanTokenCases
*/
public function testOffSetOtherThanToken(string $message, ?string $wrongType): void
{
$docComment = '/** */';

$token = new Token([T_DOC_COMMENT, $docComment]);
$tokens = Tokens::createFromDocComment($token);

$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage($message);

// @phpstan-ignore-next-line as we are testing the type error
$tokens[1] = $wrongType;
}

public function provideOffSetOtherThanTokenCases(): iterable
{
yield [
'Token must be an instance of PhpCsFixer\Doctrine\Annotation\Token, "null" given.',
null,
];

yield [
'Token must be an instance of PhpCsFixer\Doctrine\Annotation\Token, "string" given.',
'foo',
];
}
}
8 changes: 2 additions & 6 deletions tests/Fixer/Alias/SetTypeToCastFixerTest.php
Expand Up @@ -74,7 +74,7 @@ public function provideFixCases(): array
'<?php $foo = (bool) $foo;',
'<?php settype ( $foo , "Bool");',
],
'boolean' => [
'boolean with extra space' => [
'<?php $foo = (bool) $foo;',
'<?php settype ( $foo , "boolean");',
],
Expand Down Expand Up @@ -218,15 +218,11 @@ function settype($a, $b){} // "
'unknown type II' => [
'<?php settype($foo, "stringX");',
],
'null cast' => [
SpacePossum marked this conversation as resolved.
Show resolved Hide resolved
'<?php $foo = null;',
'<?php settype($foo, "null");',
],
'boolean' => [
'<?php $foo = (bool) $foo;',
'<?php settype($foo, "boolean", );',
],
'comments with line breaks' => [
'comments with line breaks II' => [
'<?php #0
#1
$foo = (int) $foo#2
Expand Down
2 changes: 1 addition & 1 deletion tests/Fixer/Phpdoc/NoSuperfluousPhpdocTagsFixerTest.php
Expand Up @@ -1809,7 +1809,7 @@ class Foo {
null,
['allow_mixed' => false],
],
'allow_mixed=>true' => [
'allow_mixed=>true ||' => [
'<?php
class Foo {
/**
Expand Down
13 changes: 8 additions & 5 deletions tests/Tokenizer/TokensTest.php
Expand Up @@ -282,10 +282,11 @@ public function provideFindSequenceCases(): array
*/
public function testFindSequenceException(string $message, array $sequence): void
{
$tokens = Tokens::fromCode('<?php $x = 1;');

$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage($message);

$tokens = Tokens::fromCode('<?php $x = 1;');
$tokens->findSequence($sequence);
}

Expand Down Expand Up @@ -732,21 +733,23 @@ public function provideFindBlockEnd80Cases(): array

public function testFindBlockEndInvalidType(): void
{
Tokens::clearCache();
$tokens = Tokens::fromCode('<?php ');

$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessageMatches('/^Invalid param type: "-1"\.$/');

Tokens::clearCache();
$tokens = Tokens::fromCode('<?php ');
$tokens->findBlockEnd(-1, 0);
}

public function testFindBlockEndInvalidStart(): void
{
Tokens::clearCache();
$tokens = Tokens::fromCode('<?php ');

$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessageMatches('/^Invalid param \$startIndex - not a proper block "start"\.$/');

Tokens::clearCache();
$tokens = Tokens::fromCode('<?php ');
$tokens->findBlockEnd(Tokens::BLOCK_TYPE_DYNAMIC_VAR_BRACE, 0);
}

Expand Down