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

Explicit never early terminates #366

Merged

Conversation

b1rdex
Copy link
Contributor

@b1rdex b1rdex commented Nov 2, 2020

@b1rdex b1rdex force-pushed the explicit-never-early-terminates branch from 6f9fd27 to c22522d Compare November 2, 2020 09:57
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This is almost perfect PR :) Please test it in a more modern way - see NodeScopeResolverTest::testFileAsserts - add a new dataProvider in it. You can use assertVariableCertainty function inside the test file. Thanks.

@b1rdex b1rdex force-pushed the explicit-never-early-terminates branch from c22522d to fd6f35c Compare November 3, 2020 02:58
@b1rdex
Copy link
Contributor Author

b1rdex commented Nov 3, 2020

@ondrejmirtes I updated the test

@b1rdex b1rdex force-pushed the explicit-never-early-terminates branch from fd6f35c to f0df73c Compare November 3, 2020 05:49
@@ -10259,6 +10259,12 @@ public function dataBug4016(): array
return $this->gatherAssertTypes(__DIR__ . '/data/bug-4016.php');
}

/** @return array<string, mixed[]> */
public function phpDocNeverEarlyTerminates(): array
Copy link
Member

Choose a reason for hiding this comment

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

Needs to start with data as any other data provider here.

$pre = 1;
Foo::doBarPhpDoc();
$a = 1;
assertVariableCertainty(TrinaryLogic::createYes(), $pre);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not even sure if these are executed. They might not be. Have you seen them fail?

The typical test here should be this instead:

if (rand(0, 1)) {
    $a = 1;
} else {
    Foo::doBarPhpDoc();
}

assertVariableCertainty(TrinaryLogic::createYes(), $a);

@ondrejmirtes
Copy link
Member

Yeah, I just verified that the asserts are not verified because they're "dead code". I'll bring it over the finish line myselfů

@ondrejmirtes ondrejmirtes force-pushed the explicit-never-early-terminates branch from f0df73c to 410c3ef Compare November 4, 2020 07:49
@ondrejmirtes ondrejmirtes merged commit 5f71943 into phpstan:master Nov 4, 2020
@ondrejmirtes
Copy link
Member

Thank you. There needs to be a second part - verifying that a function with @return never actually never returns.

@b1rdex b1rdex deleted the explicit-never-early-terminates branch November 4, 2020 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants