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

Ignoring code blocks using PHPDocs #4452

Closed
murtukov opened this issue Jan 28, 2021 · 31 comments
Closed

Ignoring code blocks using PHPDocs #4452

murtukov opened this issue Jan 28, 2021 · 31 comments

Comments

@murtukov
Copy link

murtukov commented Jan 28, 2021

Feature request

Would it be possible to implement @phpstan-ignore-start and @phpstan-ignore-end tags to ignore whole code blocks similar to the @codeCoverageIgnoreStart and @codeCoverageIgnoreEnd tags?

@ondrejmirtes
Copy link
Member

What's the use-case? Can you show a code sample?

@murtukov
Copy link
Author

I mostly faced the need in such tags, when a same error appears in several subsequent lines. The $container->getParameter() returns mixed results, but the constructor of the Config class expects specific ones.

Example:

new Config(
    // @phpstan-ignore-next-line
    $container->getParameter('kernel.debug'),
    // @phpstan-ignore-next-line
    $container->hasParameter('kernel.cache_dir'),
    // @phpstan-ignore-next-line
    $container->hasParameter('kernel.another_param'),
    // @phpstan-ignore-next-line
    $container->hasParameter('kernel.other_param'),
);

This could be written like this:

new Config(
    // @phpstan-ignore-start
    $container->getParameter('kernel.debug'),
    $container->hasParameter('kernel.cache_dir'),
    $container->hasParameter('kernel.another_param'),
    $container->hasParameter('kernel.other_param'),
    // @phpstan-ignore-end
);

@ondrejmirtes
Copy link
Member

What kind of error message are you ignoring with $container->getParameter('kernel.debug')?

@murtukov
Copy link
Author

murtukov commented Feb 2, 2021

@ondrejmirtes

Example:

Parameter #2 $cacheDir of class Config constructor expects string|null, array|bool|float|int|string|null given.

@ondrejmirtes
Copy link
Member

FYI you can write a dynamic return type extension that would resolve a correct type for getParameter().

@FunctionDJ
Copy link

I would also appreciate this. I need to work with data that must be null as a fallback for some fields of the field is falsy, so i'm using the short ternary ?:, which is reported by PHPStan. Since i work with the data in associative arrays, there are blocks in my code where i need to add an "ignore" line for ~40% of the other lines.

I would really like to avoid disabling the rule globally.

@kocoten1992
Copy link

omg, I'm seaching this exact word, phpstan ignore start end, this should really exists

@aario-k24
Copy link

FYI you can write a dynamic return type extension that would resolve a correct type for getParameter().

That is just an example. This feature is really needed.

@vojtech-dobes
Copy link

FYI you can write a dynamic return type extension that would resolve a correct type for getParameter().

That is just an example. This feature is really needed.

@aario-k24 Isn't ignoring via baseline generally more favorable approach? The "annotation" can be generated for you, you will get warned when your the ignoring isn't needed anymore, and it doesn't pollute the code itself (and thus reducing readability).

And if you want definitely annotation in the code, the "single line" variant is at least more scoped and less prone to cover some error.

@kocoten1992
Copy link

One of my usage is with json type in database, I know what in the json in database, phpstan doesn't know my app as well as me. I've to add @phpstan-ignore-line to every line because of Access to an undefined property 😖

@ondrejmirtes
Copy link
Member

@kocoten1992 That can definitely be solved in a better and more typesafe way.

@kocoten1992
Copy link

kocoten1992 commented Nov 7, 2021

Hi, sometimes it the json from third party service as well, I really don't know what to do here, any suggestion is welcome, in gg closure compiler, there is a typedef, https://github.com/google/closure-compiler/wiki/Annotating-Types#typedef allowed me to tell compiler in advance what is in the object - so I don't have to keep ignore line / using property_exists

P/s: look like it my bad, I've found https://phpstan.org/writing-php-code/phpdoc-types#array-shapes, let me try it, thank you very much!

@ondrejmirtes
Copy link
Member

@kocoten1992 It's hard to tell without a code example.

You can take advantage of stub files https://phpstan.org/user-guide/stub-files to override wrong 3rd party PHPDocs and you can also take advantage of all the PHPDoc features https://phpstan.org/writing-php-code/phpdocs-basics and types https://phpstan.org/writing-php-code/phpdoc-types PHPStan offers.

Anyway, feel free to open a new discussion about your specific issues 😊

@NeilMasters
Copy link

NeilMasters commented Nov 18, 2021

https://phpstan.org/r/06c565b1-e501-459f-b846-0f92dec38512

This is boring example code but highlights something that in example code works fine. Yet when you write real world code which is always more complex phpstan frequently has trouble parsing these scenarios. The result? PHPStan whinging at you that x() expects array string but you are giving it array<int, string> or something equally baffling.

Stan, I assure you I am giving you the right bloody data. I even have it constructed right above with a doc block @var... but no. Stan whinging away.

This is why having the ability to ignore an entire block is so useful. Because PHPStan is not infallible and there is several situations where ignoring one line or next line will not work.

So what do I end up doing? Sod it. array mixed or just ignore swaths of code just to get rid of one stan parsing issue.

@ondrejmirtes
Copy link
Member

Yet when you write real world code which is always more complex

Can you show me such example? I'm writing real world code every day and didn't have the need to ignore blocks of code. PHPStan is always giving you feedback something can be improved, so you should take its word for it instead of ignoring it :)

@NeilMasters
Copy link

NeilMasters commented Nov 19, 2021

@ondrejmirtes I can not unfortunately as my employer might have something to say about that :D If I put in some effort i could probably replicate examples with enough junk code.

so you should take its word for it instead of ignoring it

That is completely missing my point that stan is not infallible and a nice way of going with the dev trope of 'what i implemented is perfect, its you that is wrong'. I could say it has x problem or y problem, its not up to you to decide what I ignore. You absolutely can say that you are not implementing this feature and that's your right to.

However I do admit that asking for a feature to be implemented without use cases is not going to get anyone's attention, I will see what I can whip up when i get time.

@ondrejmirtes
Copy link
Member

It should be easy. This will help you: https://phpstan.org/user-guide/troubleshooting-types

@mondrake
Copy link

Something along this lines may be useful in Drupal.

Drupal runs deprecation tests via PHPUnit and the Symfony's PHPUnit bridge. Current policy for runtime deprecated code is to write a @legacy annotated test that explicitly contains calls to deprecated code. PHPStan would now report these calls as errors, which is a duplicated check vs. existing tools. We cannot ignore entire files since it may well be that normal and deprecation tests are part of the same test class.

It would be useful to allow skipping @legacy annotated test in PHPUnit.

@mondrake
Copy link

@ondrejmirtes
Copy link
Member

@mondrake Unrelated to this issue, open a separate one in https://github.com/phpstan/phpstan-deprecation-rules

@mondrake
Copy link

Thanks, opened phpstan/phpstan-deprecation-rules#64

@mv-ics
Copy link

mv-ics commented May 31, 2022

I have another example for a usecase.

public function doSomething(ClassA|ClassB|ClassC|ClassD $someObject) {
    $something = match (get_class($someObject)) {
        ClassA::class => $someObject->some,
        ClassB::class, ClassC::class => $someObject->thing,
        ClassD::class => $someObject->else
    };

    $this->doSomethingWithSomething($something);
}

Sadly PHPStan doesn't resolve the class of the object properly here and reports that some, thing and else are undefined even though they are defined on the respective classes.

Here is the error message (where some is a public property on ClassA):

Access to an undefined property
Full\NameSpace\ClassA|Full\NameSpace\ClassB|Full\NameSpace\ClassC|Full\NameSpace\ClassD::$some

I guess the best solution for my specific problem would be if PHPStan could infer the proper type based on the get_class comparison. But as things are, there are a lot of errors in my case, one for every line in the match where I access some property that is be there based on the actual type. The actual code has some more lines, so I would end up exploding my function body by six or seven comments just to disable PHPStan on all the affected lines.

@mv-ics
Copy link

mv-ics commented May 31, 2022

For people with a similar use case than mine, there at least is a workaround (maybe another missing feature for PHPStan):

Instead of using get_class for the match, I now use match ($someObject::class) which works for the match. PHPStan will then complain: Match expression does not handle remaining value: class-string, but then I can just disable the check for the match line and there won't be any other errors (not sure why though. Is the type properly resolved afterwards? Or is the superior union type used? Anyway.).

So, in code:

public function doSomething(ClassA|ClassB|ClassC|ClassD $someObject) {
    /** @phpstan-ignore-next-line */
    $something = match ($someObject::class) {
        ClassA::class => $someObject->some,
        ClassB::class, ClassC::class => $someObject->thing,
        ClassD::class => $someObject->else
    };

    $this->doSomethingWithSomething($something);
}

The combination of changes gets rid of the errors. I still think PHPStan needs better type resolving inside a match. :-)

@golubovic-m
Copy link

golubovic-m commented Jun 17, 2022

I'm extending class that is defined as internal and php stans shoots error for that. And if I try using @phpstan-ignore-next-line above class definition or @phpstan-ignore-line after class definition php snipper complains, since there are strict rules on how class comments should be made.
I guess that's a good example for @phpstan-ignore-start @phpstan-ignore-end comment styles.

@ondrejmirtes
Copy link
Member

@golubovic-m Can you show this example and errors in question on phpstan.org?

@golubovic-m
Copy link

Actually I managed to squeeze comment link this

/**
 * Class MyClass overrides inline block form.
 *
 * @phpstan-ignore-next-line */
class MyClass extends InlineBlock{

Comment above the class was a bit nicer before, had comment closing mark alone in the last line, but this works too.

@XedinUnknown
Copy link

My use-case is very similar to the one of @golubovic-m above. I am extending Exception, and __construct() does not declare its param types until PHP 8.0, it seems. So, in order to suppress that, I'd have to do that madness with the closing comment line. However, I do not want PHPStan to just stop checking everything in the signature of __construct(). I only want it to stop complaining about the missing types.

I would really like to have the ability to ignore particular errors by type rather than message, in specific lines, and whole files without having to write config, and blocks of code. For example, ignoring a particular error in a whole function, but not whole file. This is declarative and clean, and I took this feature for granted with Psalm and PHPCS, and now I can see how much suffering the lack of something so simple to use (not necessarily to implement) can bring.

@davidhoelzel
Copy link

Another use-case would be developing on different branches and having a Controller/Command/whatever for testing your code which is not and will not be added to git.
So, after switching branches, phpstan will complain about hundreds of errors because of missing classes etc. And I cannot spot the one error I have made after switching the branch and adding 2 lines of code for a bugfix e.g.

@ondrejmirtes
Copy link
Member

Hello, I'm sorry but I feel very close to closing this feature request. To me it looks like every comment here describes a use case that has a proper solution other than ignoring whole blocks of code.

I'm afraid that the ability to ignore whole codeblocks with start/end annotations would support laziness of developers to grasp for easy solutions instead of proper ones and would be detrimental to PHPStan's mission.

@renakdup
Copy link
Sponsor

renakdup commented May 2, 2024

Sounds great to have this opportunity

Copy link

github-actions bot commented Jun 3, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests