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

Fix syntax error when using readonly with dnf #9512

Closed

Conversation

iluuu1994
Copy link
Member

Fixes GH-9500

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Other than the one question LGTM!
Thanks for helping this was really a head scratcher as I had forgotten the BC layer

readonly();
readonly ();
Copy link
Member

Choose a reason for hiding this comment

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

Does using readonly () with the whitespace now break? Or is it unnecessary due to the move to the parser?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just unnecessary since this is no longer special-cased. This should still work. I can keep it if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

No harm keeping it then :)

@iluuu1994
Copy link
Member Author

I have absolutely no clue why the pipeline failed. I could run that script locally with no problem. Rebased to try again.

@Girgias
Copy link
Member

Girgias commented Sep 9, 2022

Is there maybe some caching issue on Travis?

@cmb69
Copy link
Contributor

cmb69 commented Sep 9, 2022

Did you run ext/tokenizer/tokenizer_data_gen.php?

@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 9, 2022

Did you run ext/tokenizer/tokenizer_data_gen.php?

No changes are generated as no token has been added/removed. This script works fine in the CI too. However, build/gen_stub.php fails on line 572.

if ($node instanceof Node\NullableType) {
    return new Type(
        [
            ...Type::fromNode($node->type)->types, // <-- Here
            SimpleType::null(),
        ],
        false
    );
}

PHP Parse error: syntax error, unexpected '...' (T_ELLIPSIS), expecting ']'

Which is very strange because array_pair has not been touched, nor anything else that looks relevant. I'll reset the grammar just to see if this is actually caused by my changes, although it would be a big coincidence if it wasn't.

Edit: Yep, definitely cause by my changes 😅

@iluuu1994 iluuu1994 force-pushed the gh-9500-fix-dnf-with-readonly branch 3 times, most recently from 9fbea74 to 8b18d05 Compare September 9, 2022 19:55
@nikic
Copy link
Member

nikic commented Sep 9, 2022

Ultimately, the core problem here is that gen_stub.php is required to be PHP 7.1 compatible, and someone introduced PHP 7.4 syntax into it. It needs to be changed to either be PHP 7.1 compatible, or the build system needs to be adjusted to not run gen_stub.php if the host PHP version is older than PHP 7.4.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 9, 2022

@nikic Ah, thanks. I guess Travis caches the zend_language_parser.c and only generates it when the grammar has changed. But that still poses the question, how was it generated the first time when non-PHP-7.1 syntax was used? Anyway, this is probably not worth further investigating. @kocsismate Should we bump to PHP-7.4 or remove the ...?

Edit: Ah, of course. The grammar just hasn't been changed since the ... was introduced.

@nikic
Copy link
Member

nikic commented Sep 9, 2022

I'd suggest bumping to 7.4 at this point. One must be two LTS versions behind to have something older.

Though in that case we'd also want to make more thorough use of it (yay property types).

@cmb69
Copy link
Contributor

cmb69 commented Sep 9, 2022

Regardless of whether we bump the PHP version requirement for gen_stubs.php, it probably makes sense to add an explicit version check, and fail fast for too old PHP versions.

@kocsismate
Copy link
Member

My plan was to bump the version requirement of gen_stub.php only for master as soon as I finish the PHP 8.2 related changes. That's why my preference would be to just replace the ... for now. But I don't mind either if we immediately require PHP 7.4 at least for the PHP-8.2 branch as well.

it probably makes sense to add an explicit version check, and fail fast for too old PHP versions.

Very good idea, 100% agreed!

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LG

@iluuu1994 iluuu1994 closed this in 08b7539 Sep 11, 2022
@SpacePossum
Copy link

Thank you all :) 🚀

readonly (B&C)|A $l;
private readonly A|(B&C) $m;
private readonly (B&C)|A $n;
}

Choose a reason for hiding this comment

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

<?php

class A
{
    readonly static (A&B)|C $b; // valid
    static readonly (A&B)|C $a; // invalid
}

these more simple static forms are not in the test, I haven't tested it on this PR, but just to let you know :)

jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Dec 4, 2022
… names

PHP 8.0 introduced `match` as a new reserved keyword and `mixed` as a new "other" reserved keyword.
PHP 8.1 introduced `readonly` as a new reserved keyword, `never` as an "other" reserved keyword and `enum` as a soft reserved keyword.

Note: `readonly` has an exception for when it is used as a function declaration name.

Includes regenerated test case files.

Refs:
* https://wiki.php.net/rfc/match_expression_v2
* https://wiki.php.net/rfc/mixed_type_v2#backward_incompatible_changes
* https://wiki.php.net/rfc/readonly_properties_v2
* https://wiki.php.net/rfc/enumerations
* https://wiki.php.net/rfc/noreturn_type#backwards_incompatible_changes
* php/php-src#7468 (readonly exception in PHP 8.1)
* php/php-src#9512 (readonly exception PHP 8.2 follow-up)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants