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

Built-in assertion and mock type definitions #3708

Closed
wants to merge 18 commits into from
Closed

Built-in assertion and mock type definitions #3708

wants to merge 18 commits into from

Conversation

@Ocramius
Copy link
Contributor

@Ocramius Ocramius commented May 26, 2019

As discussed briefly with @sebastianbergmann, adding a few type declarations to the codebase.

The final aim (not sure if fully to be reached in this PR) is to:

  • identify redundant assertions - self::assertTrue(true); should likely become some sort of type check error out of current patch scope
  • identify impossible type errors, such as if (! $a instanceof B) { self::fail(); } typeMismatch($a /* requires B */);
  • allow type-checking MockObject instances wherever an explicit class name is given
  • make the build green

Notes:

  • Prophecy is not yet covered - patches need to be applied upstream in order to make a Prophecy instance templated
  • I don't yet have a strategy for testing that the added type checks do detect (or not) type errors: thinking of using .phpt files

Upstream issues currently causing false positives here:

composer.json Outdated Show resolved Hide resolved
Loading
src/Framework/Assert.php Outdated Show resolved Hide resolved
Loading
src/Framework/MockObject/MockObject.php Outdated Show resolved Hide resolved
Loading
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented May 26, 2019

@weirdan can you give this a skim, since you worked on a lot of this stuff?

Loading

@weirdan
Copy link

@weirdan weirdan commented May 27, 2019

I don't yet have a strategy for testing that the added type checks do detect (or not) type errors: thinking of using .phpt files

I've used Codeception gherkin tests (with custom modules) in plugins, however using it here would introduce potential dependency problems (Codeception depends on PHPUnit). PHPT runner extracts the code from .phpt file and runs it with php - you would have to find a way to pass this code to Psalm instead.

Another point to consider is what Psalm's target version is going to be. Psalm is moving fast, and the code used to be passing with flying colors yesterday not necessarily will pass today. Error messages also change sometimes. Unfortunately composer does not allow specifying version constraints on suggested packages, so there's no good way to declare peer dependency constraints (as npm calls it).

Loading

@Slamdunk
Copy link
Contributor

@Slamdunk Slamdunk commented May 27, 2019

The final aim (not sure if fully to be reached in this PR) is to:

  • identify redundant assertions - self::assertTrue(true); should likely become some sort of type check error
  • identify impossible type errors, such as if (! $a instanceof B) { self::fail(); } typeMismatch($a /* requires B */);
  • allow type-checking MockObject instances wherever an explicit class name is given

I'm sorry if I ask dumb questions, but what are the benefits of coupling this goals with a specific tool? Until now I've successfully used @phpstan phpunit extension, which requires zero modification of this library and, AFAIK, provides the same functionalities.

Is this intended to be used with Roave/you-are-using-it-wrong? If yes, again, what would be the benefits? you-are-using-it-wrong only do the checks during installation, while psalm and phpstan do the works every run.

@vimeo Psalm is a great tool, but I'm worried when I see tool-specific edits to libraries, look such a huge burden from a maintainer perspective, even if they are just docblocks 😟

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented May 27, 2019

@Slamdunk no dependencies to be added here: this would just make type checks work OOTB (without any plugins) downstream for those using psalm. Phpstan is working on supporting these annotations too, but isn't quite there yet. I strongly believe that no plugin should be needed for what is to be provided by clear declarations in the upstream sources.

Similar discussion to doctrine/collections#177

Loading

src/Framework/Assert.php Outdated Show resolved Hide resolved
Loading
@ondrejmirtes
Copy link

@ondrejmirtes ondrejmirtes commented Jun 4, 2019

I can get behind generics, but I don't understand why PHPUnit's code needs to be littered with @psalm-assert annotations. I prefer using external extension for this... Adding annotations to PHPUnit so that Psalm can work out of the box seems to me like a dependency direction with an arrow on the wrong side...

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 4, 2019

I don't understand why PHPUnit's code needs to be littered with @psalm-assert annotations.

Sources dictate types, not external runtime extensions. I've already explained it at #3708 (comment)

Besides that, no explicit dependency, since prefixed annotations are ignored by tooling ;-)

Loading

@ondrejmirtes
Copy link

@ondrejmirtes ondrejmirtes commented Jun 4, 2019

Also, PHPStan plans to support generics with exactly this syntax (@template etc. with optional vendor prefix) because it's pretty easy, but this syntax is too obscure and proprietary:

  • @psalm-assert TEmptyMixed $actual
  • @psalm-assert !=ExpectedType

If I didn't know what assertNotSame actually does, I wouldn't be able to figure out what the annotation does. (PHPStan's phpstan-phpunit extension already understands these method calls.)

Loading

@ondrejmirtes
Copy link

@ondrejmirtes ondrejmirtes commented Jun 4, 2019

I get it it's not a technical dependency, but if only Psalm can (and will ever) benefit from these annotations, should they really be maintained as part of PHPUnit source code?

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 4, 2019

I think so, yes. It is a win-win from a technical/engineering perspective:

  • works out of the box
  • no runtime overhead
  • no added dependencies
  • ignored by tooling that is unaware of these
  • immediate technical benefits for downstream projects

Note that the @psalm-* prefix is precisely what makes this extremely viable: if it was un-prefixed, this would likely lead to massive headaches downstream (because people do a lot of crazy stuff with annotations).

Loading

@muglug
Copy link
Contributor

@muglug muglug commented Jun 4, 2019

@ondrejmirtes if you build equivalent support for @phpstan-assert, I promise you Psalm will support that too.

I absolutely believe they should be part of the codebase, though. TypeScript has the concept of type predicates which essentially do the same thing.

Loading

@ondrejmirtes
Copy link

@ondrejmirtes ondrejmirtes commented Jun 4, 2019

Yeah, I can also do that (it would be doable even with the current public API - TypeSpecifyingExtension - anyone could build an extension that reads and interprets @psalm-assert), but the current syntax is really weird - I guess that names like TEmptyMixed are internal Psalm class names, or what is it? Is there a list of all possible asserts-after-method-call?

Loading

@muglug
Copy link
Contributor

@muglug muglug commented Jun 4, 2019

TEmptyMixed is not accepted docblock syntax anywhere. @psalm-assert empty $foo is.

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 4, 2019

I've mentioned that in #3708 (comment) - will get rid of most of these internal specs as I go forward and write tests ;-)

Loading

@ondrejmirtes
Copy link

@ondrejmirtes ondrejmirtes commented Jun 4, 2019

OK :)

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 5, 2019

The scalar issue was incorrect in my own definitions.

Here's an updated report:


INFO: PropertyNotSetInConstructor - tests/static-analysis/TestUsingMocks.php:26:13 - Property PHPUnit\Framework\TestCase::$backupGlobals is not defined in constructor of PHPUnit\StaticAnalysis\TestUsingMocks or in any methods called in the constructor
final class TestUsingMocks extends TestCase

ERROR: InvalidReturnType - tests/static-analysis/happy-path/assert-empty.php:7:13 - The declared return type 'false' for PHPUnit\Framework\StaticAnalysis\HappyPath\consume is incorrect, got 'bool'
/** @return false */

ERROR: DocblockTypeContradiction - tests/static-analysis/happy-path/assert-empty.php:12:12 - string(a non-empty string) does not contain string()|string(0)
    return $value === 'a non-empty string';

ERROR: InvalidReturnStatement - tests/static-analysis/happy-path/assert-empty.php:12:12 - The type 'bool' does not match the declared return type 'false' for PHPUnit\Framework\StaticAnalysis\HappyPath\consume
    return $value === 'a non-empty string';

ERROR: InvalidReturnType - tests/static-analysis/happy-path/assert-not-empty.php:7:13 - The declared return type 'false' for PHPUnit\Framework\StaticAnalysis\HappyPath\AssertNotEmpty\consume is incorrect, got 'bool'
/** @return false */

ERROR: InvalidReturnStatement - tests/static-analysis/happy-path/assert-not-empty.php:12:12 - The type 'bool' does not match the declared return type 'false' for PHPUnit\Framework\StaticAnalysis\HappyPath\AssertNotEmpty\consume
    return $value === '';

------------------------------
5 errors found
------------------------------
1 other issues found.
You can hide them with --show-info=false
------------------------------

Checks took 0.40 seconds and used 95.418MB of memory
Psalm was able to infer types for 100% of the codebase

Loading

This is a workaround to not require vimeo/psalm#1743
to be fixed first
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 5, 2019

Fixed all \o/


INFO: PropertyNotSetInConstructor - tests/static-analysis/TestUsingMocks.php:26:13 - Property PHPUnit\Framework\TestCase::$backupGlobals is not defined in constructor of PHPUnit\StaticAnalysis\TestUsingMocks or in any methods called in the constructor
final class TestUsingMocks extends TestCase

------------------------------
No errors found!
------------------------------
1 other issues found.
You can hide them with --show-info=false
------------------------------

Checks took 0.36 seconds and used 95.410MB of memory
Psalm was able to infer types for 100% of the codebase

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 5, 2019

@sebastianbergmann how do you feel about fixing PHPUnit\Framework\TestCase#$backupGlobals not having a default value? Should I change its documented type to bool|null, assign it false by default, or leave it alone?

Loading

Ocramius added 2 commits Jun 5, 2019
`master` was being used instead, but no such version is present in the build
matrix.
@psalm-shepherd
Copy link

@psalm-shepherd psalm-shepherd commented Jun 5, 2019

Psalm didn’t find any errors!

Loading

Repository owner deleted a comment from muglug Jun 5, 2019
Annotations such as `@return numeric` are not recognized by this tooling,
so for now this test suite is off-limits for `php-cs-fixer`.
src/Framework/TestCase.php Outdated Show resolved Hide resolved
Loading
@Ocramius Ocramius changed the title WIP: built-in assertion and mock type definitions Built-in assertion and mock type definitions Jun 5, 2019
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 5, 2019

Green!

@sebastianbergmann up to you now - need also opinion on #3708 (comment)

Loading

muglug
muglug approved these changes Jun 5, 2019
Copy link
Contributor

@muglug muglug left a comment

Looks awesome!

Loading

</projectFiles>

<issueHandlers>
<LessSpecificReturnType errorLevel="error" />
Copy link
Contributor

@muglug muglug Jun 5, 2019

Choose a reason for hiding this comment

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

you can remove all of these - error is the default level

Loading

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Jun 6, 2019

I would leave PHPUnit\Framework\TestCase#$backupGlobals alone for now.

Loading

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Jun 6, 2019

Merged, thanks!

Loading

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Jun 6, 2019

\o/

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants