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

Remove @psalm-generator-return annotation #7853

Merged
merged 1 commit into from Apr 13, 2022

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Apr 9, 2022

While trying to shrink DocumentationTest::WALL_OF_SHAME (#7849, #7851), I stumbled upon @psalm-generator-return annotation.

The only references to it I could find are the aforementioned wall of shame, DocComment::PSALM_ANNOTATIONS and GeneratorTest:

'generatorWithReturn' => [
'<?php
/**
* @return Generator<int,int>
* @psalm-generator-return string
*/
function fooFoo(int $i): Generator {
if ($i === 1) {
return "bash";
}
yield 1;
}',
],

I tried adding assertions to this test, and inferred types don't seem to be affected by @psalm-generator-return presence (I checked fooFoo() return type and fooFoo()->getReturn()).

The oldest reference to this annotation I can find is d71d439 (late 2016).

My guess is that at that point Psalm did not support TSend and TReturn for generators yet, so this annotation was meant to be a replacement for the latter, which ended up not being implemented.

I'm pretty sure that this annotation is no-op and can be safely removed. The only effect of this change should be throwing an unknown annotation error, that's why it's targeted at master.

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Apr 10, 2022

The only effect of this change should be throwing an unknown annotation error, that's why it's targeted at master.

This sort of thing should be safe to do on 4.x. We should probably make it more clear, but Psalm provides no compatibility guarantees as far as issue messages, issues detected, etc between minor versions (and indeed a lot of the minor releases include newly detected issues and fixed false positives). I think we do try to maintain compatibility as far as docblock tags supported, but given that it was never really implemented I don't see it being an issue in this case.

Major changes should target 5.x since there are enough changes there to make larger features difficult to merge, and anything breaking compatibility with the plugin API (including any non-internal classes) needs to target 5.x because we do (try to) keep the plugin API compatible in minor releases.

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 10, 2022

Psalm provides no compatibility guarantees as far as issue messages, issues detected, etc between minor versions

I know, but deleting an annotation (even if it's a broken one) feels like something more suitable for a major version :)
I can rebase this PR onto 4.x if you prefer that.

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Apr 13, 2022
@orklah
Copy link
Collaborator

orklah commented Apr 13, 2022

Thanks!

@orklah orklah merged commit 0d10280 into vimeo:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants