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

Fix template parameter collection for child classes with fewer parameters (fixes #6734) #6742

Merged
merged 5 commits into from
Oct 28, 2021

Conversation

danog
Copy link
Collaborator

@danog danog commented Oct 26, 2021

No description provided.

@orklah orklah linked an issue Oct 26, 2021 that may be closed by this pull request
@orklah
Copy link
Collaborator

orklah commented Oct 26, 2021

Can you explain in a few words what was the issue and how you fixed it? It helps with the review. I'm not familiar with a lot of those classes, particularly for templates

Thanks :)

@danog
Copy link
Collaborator Author

danog commented Oct 27, 2021

This is actually my first time touching this part of psalm's codebase, but I think I pinpointed&fixed the issue: some extremely weird and non-generic logic was used when resolving template parameters, which completely failed especially with recursive templates with different parameter names, or when the order or number of the parameters was different in child classes, or when a union of template types was used when extending a parent template type: https://psalm.dev/r/aa45e698db

This PR fixes all of the issues listed above :)

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/aa45e698db
<?php
                    class Real {}
                    
                    class RealE extends Real {}
                    
                    /**
                     * @template TKey as array-key
                     * @template TValue as object
                     */
                    class a {
                        /** 
                         * @param TKey $key
                         * @param TValue $real
                         */
                        public function __construct(public int|string $key, public object $real) {}
                        /**
                         * @return TValue
                         */
                        public function ret(): object {
                            return $this->real;
                        }
                    }
                    /**
                     * @template TTKey as array-key
                     * @template TTValue as object
                     *
                     * @extends a<TTKey, TTValue>
                     */
                    class b extends a {
                    }
                    
                    /**
                     * @template TObject as Real
                     *
                     * @extends b<string, TObject>
                     */
                    class c1 extends b {
                        /**
                         * @param TObject $real
                         */
                        public function __construct(object $real) {
                            parent::__construct("", $real);
                        }
                    }
                    
                    /**
                     * @template TObject as Real
                     * @template TOther
                     *
                     * @extends b<string, TObject>
                     */
                    class c2 extends b {
                        /**
                         * @param TOther $other
                         * @param TObject $real
                         */
                        public function __construct($other, object $real) {
                            parent::__construct("", $real);
                        }
                    }
                    
                    /**
                     * @template TOther as object
                     * @template TObject as Real
                     *
                     * @extends b<string, TObject|TOther>
                     */
                    class c3 extends b {
                        /**
                         * @param TOther $other
                         * @param TObject $real
                         */
                        public function __construct(object $other, object $real) {
                            parent::__construct("", $real);
                        }
                    }
                    
                    $a = new a(123, new RealE);
/** @psalm-trace $resultA */
                    $resultA = $a->ret();
                    
                    $b = new b(123, new RealE);

/** @psalm-trace $resultB */
                    $resultB = $b->ret();
                    
                    $c1 = new c1(new RealE);

/** @psalm-trace $resultC1 */
                    $resultC1 = $c1->ret();
                    
                    $c2 = new c2(false, new RealE);

/** @psalm-trace $resultC2 */
                    $resultC2 = $c2->ret();
                    
                    
                    class Secondary {}
                    
                    $c3 = new c3(new Secondary, new RealE);
/** @psalm-trace $resultC3 */
                    $resultC3 = $c3->ret();
Psalm output (using commit 6fba5eb):

INFO: Trace - 80:21 - $resultA: RealE

INFO: Trace - 85:21 - $resultB: RealE

INFO: Trace - 90:21 - $resultC1: object

INFO: Trace - 95:21 - $resultC2: false

INFO: Trace - 102:21 - $resultC3: RealE

INFO: UnusedVariable - 80:21 - $resultA is never referenced or the value is not used

INFO: UnusedVariable - 85:21 - $resultB is never referenced or the value is not used

INFO: UnusedVariable - 90:21 - $resultC1 is never referenced or the value is not used

INFO: UnusedVariable - 95:21 - $resultC2 is never referenced or the value is not used

INFO: UnusedVariable - 102:21 - $resultC3 is never referenced or the value is not used

@orklah orklah merged commit b9effdb into vimeo:master Oct 28, 2021
@orklah
Copy link
Collaborator

orklah commented Oct 28, 2021

Thanks!

@weirdan weirdan added the release:fix The PR will be included in 'Fixes' section of the release notes label Oct 31, 2021
@danog danog deleted the fix_fewer_template_parameters branch October 31, 2021 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird template behavior
3 participants