"Match arm is unreachable because previous comparison is always true"... too strict?

[gremo]

Bug report

I can understand why this is happening: I'm covering all AttendanceCountMetric in match. As soon as I add a new enum value, the error disappear.

Is this an intended behaviour or a bug?

enum AttendanceCountMetric: string
{
    case TODAY_NET_ATTENDANCES = 'total_net_attendances';
    case TODAY_UNIQUE_INS = 'today_unique_ins';
    case TODAY_UNIQUE_OUTS = 'today_unique_outs';
}

Match arm is unreachable because previous comparison is always true:

return match ($metric) {
    AttendanceCountMetric::TODAY_NET_ATTENDANCES
        => $connector->getTodayAttendancesCount(),
    AttendanceCountMetric::TODAY_UNIQUE_INS
        => $connector->getTodayAttendancesCountByAction(AttendanceAction::IN),
    AttendanceCountMetric::TODAY_UNIQUE_OUTS
        => $connector->getTodayAttendancesCountByAction(AttendanceAction::OUT),
    default => throw new \RuntimeException(
        sprintf('Unknown metric "%s" running operation "%s".', $metric->name, get_class($this))
    )
};

I don't think that PHPStan should do this check in a way that seems too strict. Why? If I remove the default arm now and then, in the future, add a new enum value, I will get a PHP error but not my clean exception. Of course PHPStan will warn me: Match expression does not handle remaining value.

Code snippet that reproduces the problem

https://phpstan.org/r/111fc61d-ba17-4b76-a536-66b0cd398c1a

Expected output

No errors regarding match.

Did PHPStan help you today? Did it make you happy in any way?

No response

[ondrejmirtes]

This is preferred code:

        return match ($metric) {
            AttendanceCountMetric::TODAY_NET_ATTENDANCES
                => $connector->getTodayAttendancesCount(),
            AttendanceCountMetric::TODAY_UNIQUE_INS
                => $connector->getTodayAttendancesCountByAction(AttendanceAction::IN),
            AttendanceCountMetric::TODAY_UNIQUE_OUTS
                => $connector->getTodayAttendancesCountByAction(AttendanceAction::OUT),
        };

When you add a new enum value, PHPStan tells you if you need to handle it here. That's preferred because you instantly know all the places where you need to handle the new value.

With the default branch, if you add a new enum case, you'll start getting runtime exceptions in your production logs. And SA will not tell you about the places where you need to handle the new value. That's suboptimal.

Also if you enable bleeding edge, the error message guides you what to do: https://phpstan.org/r/88cf5298-ab8b-4fc3-bbfc-04bc2a2f605b

Match arm comparison between AttendanceCountMetric::TODAY_UNIQUE_OUTS and AttendanceCountMetric::TODAY_UNIQUE_OUTS is always true.
Remove remaining cases below this one and this error will disappear too.

[gremo]

@ondrejmirtes Exactly what I wrote. It's ok then!

[github-actions]

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.