Skip to content

[Config] Do not generate unreachable configuration paths #58995

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

Open
wants to merge 4 commits into
base: 6.4
Choose a base branch
from

Conversation

bobvandevijver
Copy link
Contributor

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

PHPStan was having issues correctly inferring the returned type of a configuration function.

Consider the following messages as example:

Call to an undefined method Symfony\Config\Framework\AnnotationsConfig|Symfony\Config\FrameworkConfig::enabled()

This came from the following generated config class:

    /**
     * @template TValue
     * @param TValue $value
     * annotation configuration
     * @default {"enabled":false,"cache":"php_array","file_cache_dir":"%kernel.cache_dir%\/annotations","debug":true}
     * @return \Symfony\Config\Framework\AnnotationsConfig|$this
     * @psalm-return (TValue is array ? \Symfony\Config\Framework\AnnotationsConfig : static)
     */
    public function annotations(array $value = []): \Symfony\Config\Framework\AnnotationsConfig|static
    {
        if (!\is_array($value)) {
            $this->_usedProperties['annotations'] = true;
            $this->annotations = $value;

            return $this;
        }

        if (!$this->annotations instanceof \Symfony\Config\Framework\AnnotationsConfig) {
            $this->_usedProperties['annotations'] = true;
            $this->annotations = new \Symfony\Config\Framework\AnnotationsConfig($value);
        } elseif (0 < \func_num_args()) {
            throw new InvalidConfigurationException('The node created by "annotations()" has already been initialized. You cannot pass values the second time you call annotations().');
        }

        return $this->annotations;
    }

When the determined parameter type is array, only that type can be passed meaning that the is_array is unnecessary. The same holds for the generated docblock: as only an array can be passed, there is no need to define a template and psalm returns.

With the changes in this PR this method is generated more cleanly:

    /**
     * annotation configuration
     * @default {"enabled":false,"cache":"php_array","file_cache_dir":"%kernel.cache_dir%\/annotations","debug":true}
    */
    public function annotations(array $value = []): \Symfony\Config\Framework\AnnotationsConfig
    {
        if (null === $this->annotations) {
            $this->_usedProperties['annotations'] = true;
            $this->annotations = new \Symfony\Config\Framework\AnnotationsConfig($value);
        } elseif (0 < \func_num_args()) {
            throw new InvalidConfigurationException('The node created by "annotations()" has already been initialized. You cannot pass values the second time you call annotations().');
        }

        return $this->annotations;
    }

A similar issue happens with functions that do accept more than an array value:

Call to an undefined method Symfony\Config\Doctrine\Dbal\TypeConfig|Symfony\Config\Doctrine\DbalConfig::class()

This is caused by the following generated method:

  /**
     * @template TValue
     * @param TValue $value
     * @return \Symfony\Config\Doctrine\Dbal\TypeConfig|$this
     * @psalm-return (TValue is array ? \Symfony\Config\Doctrine\Dbal\TypeConfig : static)
     */
    public function type(string $name, string|array $value = []): \Symfony\Config\Doctrine\Dbal\TypeConfig|static
    {
        if (!\is_array($value)) {
            $this->_usedProperties['types'] = true;
            $this->types[$name] = $value;

            return $this;
        }

        if (!isset($this->types[$name]) || !$this->types[$name] instanceof \Symfony\Config\Doctrine\Dbal\TypeConfig) {
            $this->_usedProperties['types'] = true;
            $this->types[$name] = new \Symfony\Config\Doctrine\Dbal\TypeConfig($value);
        } elseif (1 < \func_num_args()) {
            throw new InvalidConfigurationException('The node created by "type()" has already been initialized. You cannot pass values the second time you call type().');
        }

        return $this->types[$name];
    }

While the method seems fine, the @template definition is not correctly defined, see https://phpstan.org/r/09317897-4cc8-4f67-98ca-8b6da3671b31.

With the changes in this PR the template is now strictly defined so it matches the function signature:

  /**
     * @template TValue of string|array
     * @param TValue $value
     * @return \Symfony\Config\Doctrine\Dbal\TypeConfig|$this
     * @psalm-return (TValue is array ? \Symfony\Config\Doctrine\Dbal\TypeConfig : static)
     */

See https://phpstan.org/r/986db325-9869-4a6f-8587-6af06c0612d4 for the results.

While the second change might actually be enough to fix the errors, I prefer both fixes as it no longers generates code that can not be executed anyways.

When the determined parameter type is `array`, only that type can be passed meaning that the `is_array` in unnecessary.
The same holds for the generated docblock: as only an array can be passed, there is no need to define a template and psalm returns.
@stof
Copy link
Member

stof commented Nov 26, 2024

Tests need to be updated to reflect the new behavior.

@stof
Copy link
Member

stof commented Nov 26, 2024

And new tests should probably be added covering the case being fixed for array parameters.

@bobvandevijver
Copy link
Contributor Author

Yeah, I am working on the updated tests 👍🏻

@bobvandevijver
Copy link
Contributor Author

@stof Tests have been added, I added an new testcase which adds a configuration sample for the cases I hit.

@carsonbot carsonbot changed the title fix: Do not generate unreachable configuration paths [Config] fix: Do not generate unreachable configuration paths Nov 27, 2024
@nicolas-grekas nicolas-grekas changed the title [Config] fix: Do not generate unreachable configuration paths [Config] Do not generate unreachable configuration paths Nov 27, 2024
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
@bobvandevijver bobvandevijver force-pushed the generated-config-docblock branch from d361dc7 to 098586c Compare June 27, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants