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

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

Open
SignpostMarv opened this issue Jan 13, 2020 · 4 comments

Comments

@SignpostMarv
Copy link
Contributor

  • 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
@weirdan
Copy link
Member

weirdan commented Jan 13, 2020

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

@SignpostMarv
Copy link
Contributor Author

where would I poke around to have that kick in ?

@ktomk
Copy link
Contributor

ktomk commented Aug 5, 2020

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)

@bapcltd-marv
Copy link

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."

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

No branches or pull requests

4 participants