Do not put name of the MemberUsageExcluder in the error message

[ruudk]

Today I'm struggling with this wonderful library. I'm on version 1.1.0 (we can't upgrade to 1.1.1 because we need #356).

I'm suddenly getting these errors suddenly, on a branch I'm working on.

Property App\Shared\Infrastructure\Messaging\MessageBus\Symfony\Middleware\Stub\StubCommand::$id is never read
🔖 shipmonk.deadProperty.neverRead
↳ [tests/Shared/Infrastructure/.../Stub/StubCommand.php:10]

While I had this in my baseline:

$ignoreErrors[] = [
	'rawMessage' => 'Property App\\Shared\\Infrastructure\\Messaging\\MessageBus\\Symfony\\Middleware\\Stub\\StubCommand::$id is never read (all usages excluded by usageOverMixed excluder)',
	'identifier' => 'shipmonk.deadProperty.neverRead',
	'count' => 1,
	'path' => __DIR__ . '/tests/Shared/Infrastructure/Messaging/MessageBus/Symfony/Middleware/Stub/StubCommand.php',
];

On thing that caught my attention is that the baseline talks about (all usages excluded by usageOverMixed excluder) while the new error does not have that.

But there are many more errors, that are similar, in inverse:

	'rawMessage' => 'Property App\\Shared\\Infrastructure\\DraftTicketFields::$id is never read (all usages excluded by tests, usageOverMixed excluders)',

vs

Property App\Shared\Infrastructure\DraftTicketFields::$id is never read (all usages excluded by tests excluder)

I have these errors randomly on my branch, but also have seen it on other branches.

There must be something going on when how these excluders are executed. Maybe the order of them is also not deterministic.

I think we should not include these references to the excluders, because it proofs that things can go flaky.

But maybe we should focus on finding what causes it to go flaky.

The branch I work on has nothing to do with any of these files, that is what makes it so weird.

[janedbal]

This is weird, it should be deterministic.

Ideally, you need to be able to replicate both behaviours and use internal debugger to see which usages are excluded.

[janedbal]

I thought about this once again. It looks like you improved some part of code that used $mixed->id access (which was snapshotted in your baseline - all $id properties were likely used there, so all those ::$id is never read had the trailer excluded by usageOverMixed excluder). Now you likely made that $known->id which changed all the error messages of all baselined id property accesses. So you just need to regenerate baseline and you should see removal of usageOverMixed trailers in the messages.

This is my best guess and I think it is correct behaviour. Just one mixed call/access affects all equaly named members.

[ruudk]

I thought about this once again. It looks like you improved some part of code that used $mixed->id access (which was snapshotted in your baseline - all $id properties were likely used there, so all those ::$id is never read had the trailer excluded by usageOverMixed excluder). Now you likely made that $known->id which changed all the error messages of all baselined id property accesses. So you just need to regenerate baseline and you should see removal of usageOverMixed trailers in the messages.

Hmm this can make sense. But I think every person using this library will have the same confusion as I have. It shows that adding those things to the errors is brittle. You need to be an expert on this library to know about the nitty gritty. Most people are end-users that have a project where somebody configured PHPStan with these extensions.

If the error was the same, then this would be a non-issue.

In the end, there is nothing I can do about these errors other than refreshing the baseline to take in the new errors. It shows this is flawed.

[janedbal]

I see your point. This "one-affects-many" issue only relates to usageOverMixed usage (which is opt-in), so it probably affects only a few users. I'll consider the options (and likely wait to see if more complaints come) - any error message change is an unpleasant BC break.

[ruudk]

Ok thanks.

Maybe I should turn off usageOverMixed because it's pretty naive now that I think about it.

[janedbal]

because it's pretty naive now that I think about it.

What makes you think so?

The idea is described in readme - it should help you spot unwanted calls/fetches over unknown type (I believe strict codebases should have exactly zero of those).

[ruudk]

From what I understand:

$mixed->ruud

It will then always mark any property called ruud as used?

[janedbal]

$mixed->ruud It will then always mark any property called ruud as used?

Yes, unless you exclude this usage via usageOverMixed

[ruudk]

Ok, I have usageOverMixed excluder enabled. So that's not related to my issue.

[janedbal]

So that's not related to my issue.

I'm confused. I think this still applies. If you would have usageOverMixed excluder disabled, those entries in your baseline wont be there (as the mixed usage would be counted). And if you would fix it (no mixed anymore), many new violations would appear - so a similar problem one-affects-many. Just not by message changes, but a list of new errors.

[ruudk]

Hmm, so I have the usageOverMixed excluded enabled, which is the best thing to do for a properly typed codebase, right? As in, that's what everyone should strive for?

I don't understand it anymore either. I think we should have a config option to turn off these excluder suffixes in error messages, and make that a default in v2.

When I change something in one part of the codebase, I don't expect errors in another part of the codebase, just because it somehow used a same property name (id is pretty common) and one place used to be mixed.

[janedbal]

usageOverMixed excluder enabled, which is the best thing to do for a properly typed codebase

Hard to say, nothing is silver bullet. It helps you discover unwanted mixed access right away. But you may have different apporaches to eliminate those.

I don't expect errors in another part of the codebase, just because it somehow used a same property name (id is pretty common) and one place used to be mixed.

This is just what you get when using truely codebase-wide analysis. PHPStan without DCD do not behave like that, so it may seem weird. But it is actually correct. Changing messages is up to discussion, but the generic problem still remain.

[ruudk]

Some more info, maybe this is already clear to you, but for my own understanding. You probably already have come to the same conclusion and solution going forward.

Currently my baseline holds:

Property Fields::$priceInCents is never read
shipmonk.deadProperty.neverRead

In another unrelated script I write:

echo $mixed->priceInCents

I now get 2 errors:

Undefined variable: $mixed
🔖 variable.undefined

Property Fields::$priceInCents is never read (all usages excluded by usageOverMixed excluder)
🔖 shipmonk.deadProperty.neverRead

Seeing this, it does not make any sense to update the existing baselined error to append (all usages excluded by usageOverMixed excluder).

The whole goal of the excluder is to not count those. To keep those excluded.

Can I open a PR where we introduce a new feature flag to disable those suffixes?

[janedbal]

The whole goal of the excluder is to not count those. To keep those excluded.

Yes, but the visibility is there for a reason - once somebody starts verifying if it really IS dead or not, they take some IDE and search for usages. That is why DCD has this in the message: "I see the usages, but those are not counted". Without it, you would had much harder time to know if the DCD is misbehaving (not seeing your usage you see in IDE), or it got excluded somewhere.

---

Looking at your issues, consider:

• disabling usageOverMixed excluder (going back to this)
• guarding mixed usages by a rule (forbidMethodCallOnMixed, forbidFetchOnMixed)
  • and never baselining those (fix is typically very easy)

[ruudk]

That is why DCD has this in the message: "I see the usages, but those are not counted".

Can't we use error tips for this purpose? If it's to hint developers.