PHP-CS-Fixer: Rule no_unneeded_curly_braces does not understand annotation scoping

Bug report

PHP-CS-Fixer’s no_unneeded_curly_braces removes curly braces that are actually needed. Specifically, those that are defining the scope of annotations.

PHP version: 7.4.21
PHP CS Fixer version: 2.18.4
the command used to run PHP CS Fixer (run with -vvv): php-cs-fixer my-file.php --config=.php_cs.dist --allow-risky=yes
the configuration (file) you used:

<?php

$finder = PhpCsFixer\Finder::create()->in(__DIR__);

return PhpCsFixer\Config::create()
    ->setRules([
        '@Symfony'               => true,
        'binary_operator_spaces' => [
            'operators' => [
                '=>' => 'align_single_space_minimal',
            ],
        ],
        'echo_tag_syntax'             => false,
        'ordered_class_elements'      => true,
        'ordered_imports'             => true,
        'yoda_style'                  => false,
        'blank_line_before_statement' => true,
        'class_attributes_separation' => [
            'elements' => [
                'const',
                'method',
                'property'
            ],
        ],
        'no_alternative_syntax'  => false,
        'phpdoc_no_empty_return' => true,
        'concat_space'           => [
            'spacing' => 'one'
        ],
        'phpdoc_no_alias_tag'    => [
            'type' => 'var',
            'link' => 'see',
        ],
        'single_line_throw'      => false
    ])
    ->setFinder($finder)->setUsingCache(false);

Code snippet that reproduces the problem

        /* @noinspection PhpFieldAssignmentTypeMismatchInspection */
        {
            $this->qa = $env->getContext(QualityAssurance::class);
            $this->store = $env->getContext(StoreContext::class);
            $this->web = $env->getContext(WebContext::class);
        }

Expected output:

        /* @noinspection PhpFieldAssignmentTypeMismatchInspection */
        {
            $this->qa = $env->getContext(QualityAssurance::class);
            $this->store = $env->getContext(StoreContext::class);
            $this->web = $env->getContext(WebContext::class);
        }

Actual output:

        /* @noinspection PhpFieldAssignmentTypeMismatchInspection */

        $this->qa = $env->getContext(QualityAssurance::class);
        $this->store = $env->getContext(StoreContext::class);
        $this->web = $env->getContext(WebContext::class);

Repurcussions of the problem

This results in the @noinspection annotation only being applied to the first line:

Before:

Screen Shot 2021-08-26 at 7 51 54 AM

After:

Screen Shot 2021-08-26 at 7 53 01 AM

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 18 (9 by maintainers)

Most upvoted comments

I never knew this was a thing, so I didn’t consider it when making the rule. If the PHPDoc (annotation) followed by a curly block for scoping is something people really use I see no reason not to support it. It will likely harder to stop the other rule from turning the PHPDocs into comments, than stopping this rule from removing the braces. You would need both as I would not support scoped comments, only PHPDocs, but that is just my preference.

SpacePossum on Sep 2, 2021

What about situations such as how Eloquent delegates to other classes?

Even using a simple where() call on an Eloquent Model results in PHPStorm complaining that the method does not exist, because it is not actually defined on the Model class, but delegated to the Builder class via the magic __call method:

Screen Shot 2021-08-30 at 12 49 53 PM

So, I could use Lab::query()->where() here, but that only kicks the can further down the road, because there are plenty of other scenarios where you can get tangled on Eloquent’s web and not have a way to make the type system happy without significantly complicating your query by separating it into variables with @var annotations.

So instead, to keep the code simple and clean in the spirit of Laravel, we can use the same scoping here:

Screen Shot 2021-08-30 at 12 52 35 PM

In the interests of completion, let’s continue the experiment and try to use query():

Screen Shot 2021-08-30 at 12 56 43 PM

PHPStorm is happy now because I am coincidentally only using methods that exist on the Builder returned by query(). But if the Lab uses the SoftDeletes trait, I also have access to the withTrashed method. So let’s try to use that:

Screen Shot 2021-08-30 at 12 58 13 PM

Well crap, now PHPStorm is unhappy again. And this is particularly challenging because Builder does not have a withTrashed method, but it happily proxies the call to the Lab class which does have the method:

Screen Shot 2021-08-30 at 1 03 20 PM

So now we have two further problems:

We are lying about the type of $labQuery because it is a Builder, not a Lab
We still can’t make PHPStorm happy, because withTrashed is declared as static

I could use /** @var Builder|Lab $labQuery */, but that’s still wrong because Builder will only proxy called to query scopes, not all methods on the Lab class.

TL:DR: yes, you are right that some of the things I have to do are not ideal. But I am working within the constraints of the tools and frameworks that I have no option but to work with. So I am trying to do the best I can with it. I need @noinspection to allow me to remove some of the “noise” from the warnings so that developers can have faith that the warnings they see are legitimate. If I do not fix these, then they will lose faith in the warnings, and legitimate problems will get overlooked.

Chekote on Aug 30, 2021