ClassMustBeFinal: BC break

[Ocramius]

Looking at various composer.lock upgrades from last night:

• Lock file maintenance Roave/BetterReflection#1489
• Lock file maintenance Ocramius/CodeGenerationUtils#437
• Lock file maintenance Roave/SecurityAdvisoriesBuilder#763

Although I violently agree with making things final, vimeo/psalm:6.6.0 introduced a breaking change in what's considered valid and not (and it's not just a shift due to type refinements):

https://psalm.dev/docs/annotating_code/supported_annotations/#api-psalm-api

Specifically, it seems like Psalm now requires things to be marked as API, whilst, tbh, that's only a Symfony thing.
The rest of us uses @internal for what isn't @api: see for example https://github.com/Roave/BackwardCompatibilityCheck/blob/448b845a54afd41a54d93c4403900211abda4b3c/src/DetectChanges/BCBreak/ClassBased/MethodRemoved.php#L61-L66

Unless I'm misunderstanding something, I'd recommend having the default psalm profile not having this requirement, and requiring to do so in a new major version, if it's a design decision.

If @api is only used for discovering unused code, then I'm fine with it: at least tests should be written, but in such case, the error message reporting ClassMustBeFinal is probably wrong.

[Ocramius]

Also, to clarify: I agree that this is a good change, just not with how it was introduced, if it was intentional.

[danog]

The requirement to explicitly use @apior @internal is something that was already present in Psalm v6 due to unused code detection being turned on by default.

Generally, I do not consider the addition of issues (& the requirement to change some code due to their addition) to be a breaking change for Psalm: in fact, I'm preparing #11283 which will turn on a lot of previously hidden features in a minor (feature) tag, which makes sense because minors are for new features, and new features in Psalm inherently mean more issues.

[Ocramius]

I would advise against that: from a maintainer PoV that uses Psalm, this will just mean that every psalm update is a major version.

This already happens with PHP itself, and it hurts, a lot, every year.

It's fine if you declare it as such (by releasing major SemVer versions), but please do consider BC boundaries for your libraries/tools carefully, because this really hurts people that put some real effort in both advocating for psalm, and making codebases as bug-free as possible.

If you do not have BC boundaries of any type here, and things like #11283 become the norm, then I'm OK with migrating my OSS projects to PHPStan, with some effort, and I'm not really happy to do so either, since I firmly believe that psalm has an edge in taint and purity analysis.

[danog]

This was never the case with Psalm, new issues were always introduced in minors, never in majors: majors were only used for major API changes (which affect plugins).

I understand phpstan uses a different approach with bleeding edge, but this was never the case with Psalm, and I see no reason to change this.

[Ocramius]

Fair enough, perhaps we got spoiled by a long period of seeming stability and bugfixing-only.

[iquito]

@Ocramius You can just suppress this new issue in the psalm config (link to documentation). Personally I don't think this new issue makes much sense for libraries (and should be suppressed, which I will do, rather than be forced to put final and @api everywhere), while it does usually make sense for applications.

(I would prefer if such new issues were introduced as opt-in rather than opt-out too, though, such that projects are not surprised by new issues in possibly every new minor version - seems fragile)

[Ocramius]

@iquito I'm well aware that there's:

• suppressing
• baselining
• spreading @api like dung on a field

The problem is mostly the direction that is being taken, which worries me much more: this issue is a data point.