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

Ignore complaints about dead code #495

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Sep 4, 2024

In the code of these constructors, we are checking the type of the 'value' property of the '$values' array.
Let us reflect the fact that we are not assuming the type in the phpdoc. I think this is OK because since both of these classes are used as Doctrine annotations, there is no way for such precise types to be useful for the end user anyway. I chose that over creating a baseline for a project that does not have one yet.

We've declared types in phpdoc, but they are not enforced by type
declarations, so we are enforcing them with checks. Psalm complains it
is dead code because it assumes the types can be trusted.

This addresses a Psalm issue I spotted while reviewing #494

@derrabus
Copy link
Member

derrabus commented Sep 4, 2024

What issue does this fix exactly?

@greg0ire
Copy link
Member Author

greg0ire commented Sep 4, 2024

The issue is not very clearly expressed:

ERROR: NoValue - lib/Doctrine/Common/Annotations/Annotation/Enum.php:51:49 - All possible types for this argument were invalidated - This may be dead code (see https://psalm.dev/179)
                    is_object($var) ? get_class($var) : gettype($var)


ERROR: NoValue - lib/Doctrine/Common/Annotations/Annotation/Target.php:77:61 - All possible types for this argument were invalidated - This may be dead code (see https://psalm.dev/179)
                    is_object($values['value']) ? get_class($values['value']) : gettype($values['value'])

Since the phpdoc says scalar, and we are inside a condition on !is_scalar(…), the inferred type is never, and that should never happen, so Psalm suspects there is dead code.

@SenseException
Copy link
Member

While there isn't a baseline file, you can find ignoreErrors directly in phpstan.neon.

@derrabus
Copy link
Member

derrabus commented Sep 5, 2024

But those "errors" happen during input validation where we handle exactly the cases that we declare unacceptable in the doc blocks that you're about to change. Psalm complaining about input validation is just Psalm being Psalm.

I would simply ignore them.

@greg0ire
Copy link
Member Author

greg0ire commented Sep 5, 2024

Ok, let me adjust.

We've declared types in phpdoc, but they are not enforced by type
declarations, so we are enforcing them with checks. Psalm complains it
is dead code because it assumes the types can be trusted.
@greg0ire
Copy link
Member Author

greg0ire commented Sep 5, 2024

@SenseException I did the Psalm equivalent of what you mentioned.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The PR title needs to be adjusted though.

@derrabus derrabus added this to the 1.14.4 milestone Sep 5, 2024
@greg0ire greg0ire changed the title Use wider type declarations Ignore complaints about dead code Sep 5, 2024
@greg0ire greg0ire merged commit af1295b into doctrine:1.14.x Sep 5, 2024
12 checks passed
@greg0ire greg0ire deleted the fix-build branch September 5, 2024 07:20
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.

3 participants