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

Property not set in constructor when using trait in abstract class #6087

Open
gianiaz opened this issue Jul 14, 2021 · 14 comments
Open

Property not set in constructor when using trait in abstract class #6087

gianiaz opened this issue Jul 14, 2021 · 14 comments

Comments

@gianiaz
Copy link

gianiaz commented Jul 14, 2021

I think I've found a strange behaviour, in this snippet psalm complains about a property not set in constructor when the concrete class extends an abstract class using a trait.

https://psalm.dev/r/5a9fe1453e

If I take rid of the abstract class everything is fine.

https://psalm.dev/r/13cce84db7

It's a bug?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/5a9fe1453e
<?php

trait FooTrait {
    protected \DateTimeImmutable $creationDate;
    
    public function getCreationDate(): \DateTimeImmutable {
        return $this->creationDate;
    }
    
    protected function initTimestamp(\DateTimeImmutable $creationDate): void {
        $this->creationDate = $creationDate;
    }
}

abstract class Foo {
    
    use FooTrait;
    
    public function __construct(\DateTimeImmutable $creationDate) {
        $this->initTimestamp($creationDate);
    }
}

class Bar extends Foo {
}
Psalm output (using commit 601c8ca):

ERROR: PropertyNotSetInConstructor - 24:7 - Property Bar::$creationDate is not defined in constructor of Bar or in any methods called in the constructor
https://psalm.dev/r/13cce84db7
<?php

trait FooTrait {
    protected \DateTimeImmutable $creationDate;
    
    public function getCreationDate(): \DateTimeImmutable {
        return $this->creationDate;
    }
    
    protected function initTimestamp(\DateTimeImmutable $creationDate): void {
        $this->creationDate = $creationDate;
    }
}


class Bar  {
    use FooTrait;
    
    public function __construct(\DateTimeImmutable $creationDate) {
        $this->initTimestamp($creationDate);
    }
}
Psalm output (using commit 601c8ca):

No issues!

@Jean85
Copy link
Contributor

Jean85 commented Jul 14, 2021

Issue is still present event with an explicit call to parent::__construct(...): https://psalm.dev/r/f177e0b295

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/f177e0b295
<?php

trait FooTrait {
    protected \DateTimeImmutable $creationDate;
    
    public function getCreationDate(): \DateTimeImmutable {
        return $this->creationDate;
    }
    
    protected function initTimestamp(\DateTimeImmutable $creationDate): void {
        $this->creationDate = $creationDate;
    }
}

abstract class Foo {
    
    use FooTrait;
    
    public function __construct(\DateTimeImmutable $creationDate) {
        $this->initTimestamp($creationDate);
    }
}

class Bar extends Foo {
    public function __construct(\DateTimeImmutable $creationDate) {
        parent::__construct($creationDate);
    }
}
Psalm output (using commit 601c8ca):

ERROR: PropertyNotSetInConstructor - 24:7 - Property Bar::$creationDate is not defined in constructor of Bar or in any methods called in the constructor

@Jean85
Copy link
Contributor

Jean85 commented Jul 14, 2021

It seems related to #4393

@weirdan
Copy link
Collaborator

weirdan commented Jul 14, 2021

The issue is valid because descendants can override initTimestamp() (https://3v4l.org/gOaFu). But I think it should be reported on Foo rather than on Bar. Also, marking initTimestamp() final should prevent it, but it doesn't seem to happen currently: https://psalm.dev/r/1041e80bc6

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/1041e80bc6
<?php

trait FooTrait {
    protected DateTimeImmutable $creationDate;
    
    public function getCreationDate(): \DateTimeImmutable {
        return $this->creationDate;
    }
    
    protected final function initTimestamp(\DateTimeImmutable $creationDate): void {
        $this->creationDate = $creationDate;
    }
}

abstract class Foo {
    
    use FooTrait;
    
    public function __construct(\DateTimeImmutable $creationDate) {
        $this->initTimestamp($creationDate);
    }
}

class Bar extends Foo {
}
Psalm output (using commit 09b0167):

ERROR: PropertyNotSetInConstructor - 24:7 - Property Bar::$creationDate is not defined in constructor of Bar or in any methods called in the constructor

@weirdan weirdan added the bug label Jul 14, 2021
@Jean85
Copy link
Contributor

Jean85 commented Jul 14, 2021

You probably would need to make the constructor final too, but the issue is still present there too: https://psalm.dev/r/e0c6c4b20f

Maybe the analysis should change approach and determine if the call sequence in any class does end with the initialization of such properties..

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/e0c6c4b20f
<?php

trait FooTrait {
    protected DateTimeImmutable $creationDate;
    
    public function getCreationDate(): \DateTimeImmutable {
        return $this->creationDate;
    }
    
    protected final function initTimestamp(\DateTimeImmutable $creationDate): void {
        $this->creationDate = $creationDate;
    }
}

abstract class Foo {
    
    use FooTrait;
    
    public final function __construct(\DateTimeImmutable $creationDate) {
        $this->initTimestamp($creationDate);
    }
}

class Bar extends Foo {
}
Psalm output (using commit 09b0167):

ERROR: PropertyNotSetInConstructor - 24:7 - Property Bar::$creationDate is not defined in constructor of Bar or in any methods called in the constructor

@rickogden
Copy link

I've also been having this issue when attempting to write a Psalm plugin for Doctrine ODM for MongoDB. It's slightly different, as the constructor itself is in the trait, but the error is the same.

Example: https://psalm.dev/r/0f71ca2564

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/0f71ca2564
<?php

class DocumentRepository {
    public string $foo;
	public function __construct(string $foo)
    {
        $this->foo = $foo;
    }
}

trait ServiceRepositoryTrait {
	public function __construct()
    {
    	parent::__construct('hello world');
    }
}

abstract class ServiceDocumentRepository extends DocumentRepository {
	use ServiceRepositoryTrait;
}

class MyRepository extends ServiceDocumentRepository
{
}
Psalm output (using commit 358f83d):

ERROR: PropertyNotSetInConstructor - 22:7 - Property MyRepository::$foo is not defined in constructor of MyRepository or in any methods called in the constructor

@rulatir
Copy link

rulatir commented Sep 30, 2021

The issue is valid because descendants can override initTimestamp() (https://3v4l.org/gOaFu). But I think it should be reported on Foo rather than on Bar. Also, marking initTimestamp() final should prevent it, but it doesn't seem to happen currently: https://psalm.dev/r/1041e80bc6

No, the issue is not valid. There is no can in "descendants can override". A descendant either does initialize the creationDate property in its constructor somehow, or it does not. If it does, all is fine. If it doesn't (because it overrides initTimestamp(), or inherits an overriden initTimestamp() from an "intercestor", or fails to call the parent constructor) then the problem is with this very descendant that doesn't (and the "intercestor" if applicable). 100% of the information needed to determine which is the case is available to Psalm statically when analyzing the descendant.

@psalm-github-bot
Copy link

psalm-github-bot bot commented Sep 30, 2021

I found these snippets:

https://psalm.dev/r/1041e80bc6
<?php

trait FooTrait {
    protected DateTimeImmutable $creationDate;
    
    public function getCreationDate(): \DateTimeImmutable {
        return $this->creationDate;
    }
    
    protected final function initTimestamp(\DateTimeImmutable $creationDate): void {
        $this->creationDate = $creationDate;
    }
}

abstract class Foo {
    
    use FooTrait;
    
    public function __construct(\DateTimeImmutable $creationDate) {
        $this->initTimestamp($creationDate);
    }
}

class Bar extends Foo {
}
Psalm output (using commit 10f1af1):

ERROR: PropertyNotSetInConstructor - 24:7 - Property Bar::$creationDate is not defined in constructor of Bar or in any methods called in the constructor

@orklah orklah added the traits label Sep 30, 2021
@jaikdean
Copy link

Are there any additional test cases or anything else that would help with this issue? I'd like to contribute but have next to no understanding of how Psalm's internals work.

@orklah
Copy link
Collaborator

orklah commented Nov 19, 2021

Mainly we need contributors with a good understanding of how Traits works. I barely use them myself so I'm not very motivated to dig into this subject on top of being almost ignorant about the syntax and how people use them.

If you want to take a shot, you can try following those instructions: #6937 (comment) and report your findings, we'll try to help if you get stuck

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

No branches or pull requests

7 participants