psalm-plugin-phpunit icon indicating copy to clipboard operation
psalm-plugin-phpunit copied to clipboard

calling assertInstanceOf on a property set in TestCase::setUp() should result in RedundantCondition

Open SignpostMarv opened this issue 5 years ago • 4 comments

  • if TestCase::$foo is T|null
  • and TestCase::setUp() sets TestCase::$foo to T
  • and nothing else modifies TestCase::$foo
  • then TestCase::assertInstanceOf(T::class, $this->foo) is redundant

not quite sure on the exact error/test case that should occur, but this is a vague mashup of what I'm thinking:

  Scenario: setUp resolves types
    Given I have the following code
      """
      class MyTestCase extends TestCase
      {
        /** @var \DateTime|null */
        private $date;

        /** @return void */
        public function setUp() {
          $this->date = new \DateTime();
        }

        /** @return void */
        public function testSomething() {
          $this->assertInstanceOf(\DateTime::class, $this->date);
        }
      }
      """
    When I run Psalm
    Then I see these errors
      | Type            | Message                                                                                                     |
      | RedundantConditionGivenDocblockType | Found a redundant condition when evaluating docblock-defined type $this->date and trying to reconcile type 'DateTime' to DateTime |
    And I see no other errors

SignpostMarv avatar Jan 13 '20 16:01 SignpostMarv

I think setUp() method analyzer has to be run with $scope->collect_initializations = true and it'll fix the issue automatically.

weirdan avatar Jan 13 '20 17:01 weirdan

where would I poke around to have that kick in ?

SignpostMarv avatar Jan 13 '20 18:01 SignpostMarv

Depending on the visibility of TestCase::$foo (e.g. not private), this would be runtime behavior. Just saying. Psalm would not be able to detect it unchanged. Even if private PHP has reflection, so again, runtime behavior.

/E: Have you tried to mark the TestCase::$foo property as @psalm immutable? No idea if the phpunit plugin needs to understand Phpunit\TestCase::setUp as eligible for (implicit) setter injection / initialization and immutable has enough introspection on class properties for the class itself, but then technically this should be the outcome (without even taking care of visibility / reflection or runtime behavior)

ktomk avatar Aug 05 '20 00:08 ktomk

have emailed GitHub re the rather unpleasant commit ref spam caused by rebasing- it should be resolved by an interface change in GitHub "approximately 1-2 quarters from now."

bapcltd-marv avatar Aug 10 '20 14:08 bapcltd-marv