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

Implement ReturnTypeWillChange attributes #40

Merged
merged 1 commit into from
Aug 13, 2021
Merged

Implement ReturnTypeWillChange attributes #40

merged 1 commit into from
Aug 13, 2021

Conversation

driesvints
Copy link
Contributor

@driesvints driesvints commented Aug 12, 2021

Adds ReturnTypeWillChange to suppress PHP 8.1 warnings. At the moment this is blocking the laravel/framework build from running the full test suite on PHP 8.1. Would appreciate a review, merge and tag if possible! Thanks :)

@driesvints
Copy link
Contributor Author

Seems like tests are failing for an unrelated reasoning. Perhaps coverage borking because I'm sending from a fork?

@simensen
Copy link
Member

@driesvints I don't know enough about this ReturnTypeWillChange solution. Would you say this would be considered a BC break or no? :) Wondering if this should be a minor release or a major one.

I enabled tests to run for you so you should be able to see them going forward. :) I'm not sure why the Psalm test is failing? Is this possibly because whatever is in this code now isn't compatible with PHP 7.1 and Psalm for some reason? If you want to test adding an upgrade to the PHP version selected for the Psalm job we can see if that fixes this PR.

Happy to tag and release. Should probably get @colinodell to check in on it, too, to make sure I haven't missed something.

@driesvints
Copy link
Contributor Author

Hey @simensen, thanks for the review.

Would you say this would be considered a BC break or no?

No, attributes act like comments so nothing is breaking here. In fact, we've already began to roll them out on the current Laravel framework stable release as well.

I'm not sure why the Psalm test is failing?

I suspect this has nothing to do with this PR. Try re-triggering the actions on main and I suspect it'll fail as well.

Copy link
Contributor

@colinodell colinodell left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm comfortable with merging, tagging, and releasing this as-is despite the Psalm failure as there are no significant changes to the code and all other tests are passing. I'll try to carve out some time to investigate and resolve the Psalm issues this weekend.

@driesvints
Copy link
Contributor Author

Thanks @colinodell!

@simensen simensen merged commit bcec53d into dflydev:main Aug 13, 2021
@driesvints driesvints deleted the php81-attributes branch August 13, 2021 13:04
@simensen
Copy link
Member

@driesvints Version 3.0.1 has been released. Please let me know if it's causing any issues. Thanks!

@colinodell Thanks for the quick turnaround on approval. :)

@driesvints
Copy link
Contributor Author

Thanks all!

@Jubeki
Copy link

Jubeki commented Aug 13, 2021

Maybe this minimal example will help while debugging?
https://psalm.dev/r/c00f19cb39

Input:

<?php

#[\ReturnTypeWillChange]
function f(string $key) : string
{
    return $key;
}

f('TEST');

Output:

INFO: UndefinedAttributeClass - 3:3 - Attribute class ReturnTypeWillChange does not exist

This still doesn't explain the error which comes from the CI:

Error: Syntax error, unexpected '=' on line 125

@simensen
Copy link
Member

I'm going to run a quick test. This really seems like it might be a PHP 8.1 thing, because my guess is that Psalm is actively trying to resolve the ReturnTypeWillChange "class" which very well may exist for PHP 8.1 but not for any other version of PHP. Thanks for the tip @Jubeki.

@simensen
Copy link
Member

I just tried to get PHP 8.1 up and running on my local and I now get a slightly different error. However, it seems to be closer to working and seems to somewhat confirm that some part of this is due to PHP 8.1 actually being the thing that can provide this class.

Seems like this isn't a problem at runtime, but possibly going to cause issues for people running some static analysis tools. :-/

I'm out of time for more digging on this for now. If anyone else wants to pick it up and see if we can figure out how to fix Psalm (even if by suppressing this somehow) I'd be all for it. :)

ERROR: InvalidAttribute - k.php:3:3 - The class ReturnTypeWillChange doesn’t have the Attribute attribute (see https://psalm.dev/242)
#[ReturnTypeWillChange]


------------------------------
1 errors found
------------------------------

dhrrgn added a commit to dhrrgn/dflydev-dot-access-data that referenced this pull request Oct 3, 2021
The mbstring polyfill added ??= which causes errors with Psalm 3. See: symfony/polyfill-mbstring@11d0d87#r45855304

This also removes the use of references in Data->has, because Psalm (rightly) considers that impure.
@dhrrgn
Copy link
Contributor

dhrrgn commented Oct 3, 2021

I found the Psalm issue, and put up a PR to fix it: #41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants