Skip to content

gen_stub: various simplifications and clean up (4) #18109

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

Merged
merged 17 commits into from
May 8, 2025

Conversation

DanielEScherzer
Copy link
Member

Each commit can be reviewed independently, and should have no functional changes

@DanielEScherzer
Copy link
Member Author

@kocsismate do you think you might have a chance to review this soon? If not, I now have merge access and can just merge things myself, but I figured a second pair of eyes would help since you seem to be responsible for most of that script.

@kocsismate
Copy link
Member

@DanielEScherzer Yes, definitely! I got some chance to review things these days, and your PR will be the next thing - hopefully tonight :)

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

I had to stop at the gen_stub: deduplicate and simplify DocCommentTag processing commit for today. I'll continue next time

@@ -4271,6 +4271,150 @@ public function getMinimumPhpVersionIdCompatibility(): ?int {
public function shouldGenerateLegacyArginfo(): bool {
return $this->minimumPhpVersionIdCompatibility !== null && $this->minimumPhpVersionIdCompatibility < PHP_80_VERSION_ID;
}

public function handleStatements(array $stmts, PrettyPrinterAbstract $prettyPrinter) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit hesitant about this. Originally, most global functions were added because they dealt with parsing. This way, classes were mostly independent from PHP-Parser symbols (emphasis on "mostly") and basically from most side-effects (i.e. file reading/writing) if I still remember correctly.

I liked this separation, but as far as I see the recent changes, this starts to go away. I still have to digest if I'm fine with the process. Even if we will eventually only have classes, then please still separate classes that have a side-effect and classes that don't (only have business logic). Having PHP-Parser related stuff within the business logic is a bit less of a concern for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

My goal was to only have classes, but the side-effects will be kept separate

@DanielEScherzer
Copy link
Member Author

I had to stop at the gen_stub: deduplicate and simplify DocCommentTag processing commit for today. I'll continue next time

To be clear, I'm waiting until the review is finished before addressing the existing feedback because I'd force-push the history and don't want to confuse things for the remaining commits to review

@kocsismate
Copy link
Member

kocsismate commented May 7, 2025

To be clear, I'm waiting until the review is finished before addressing the existing feedback because I'd force-push the history and don't want to confuse things for the remaining commits to review

Sorry, the missing commits were opened in my browser, but I couldn't finish them due to the last touches before starting the URL API RFC... Now, I reviewed them, and they all seemed fine.

P.S. I supposed that you would push fixup commits for the commits to be amended, so that the changes are easily reviewable + they don't rewrite the history. And when the PR is approved you can interactively rebase them so that your commits remain meaningful.

Otherwise GitHub's syntax highlighting treats it as the end of the code and
stops highlighting
Deduplicates the setting up of the `zend_string_init_interned()` call, removes
the need for `ExposedDocComment::getLength()` and so that method is removed.
* Return a string rather than an array, all callers just immediately used
`implode()` to join the elements in the array with nothing between them
* In the callers, inline some single-use variables with the template for the
version-dependent code
* Remove the callback to `array_filter` specifying that only items that are not
`empty()` be removed - this is the default behavior
There is no need to add special handling for the default value of `null`, since
it is not loosely-equals to any of the strings 'UNKNOWN', 'false', 'true', or
'null' it will just be returned directly anyway.
Move the logic out of `funcInfoToCode()` and update it. In the process, make
`ArgInfo::getDefaultValueAsArginfoString()` private.
The vast majority of the decisions about the use of `ZEND_BEGIN_ARG_INFO_EX` or
one of its variations are based on the return information of the function - is
the type builtin, is the return information tentative, does it include an
object mask, etc. Accordingly, move the logic into the `ReturnInfo` class.

The logic is actually moved into two methods, `ReturnInfo::beginArgInfo()`,
which needs to handle the case of tentative returns being used when PHP < 8.1
is supported, and `::beginArgInfoCompatible()`, which can assume that PHP 8.1+
is supported and thus make use of early returns and guard clauses. Further
improvements to the logic will be made in a subsequent commit.

In the process, make `ReturnInfo::$byRef` private.
Reduce the number of global functions by moving it to instance method
`FuncInfo::toArgInfoCode()`.

In the process, make `FuncInfo::$numRequiredArgs` private.
The following parameters were either unused before this commit or became unused
as part of updating callers to stop passing unused parameters to other
functions updated in this commit:
* `FuncInfo::getMethodSynopsisDocument()` - `$funcMap`, `$aliasMap`
* `FuncInfo::getMethodSynopsisElement()` - `$funcMap`, `$aliasMap`
* `ConstInfo::getGlobalConstDeclaration()` - `$allConstInfos`
* `generateMethodSynopses()` - `$aliasMap`
* `replaceMethodSynopses()` - `$aliasMap`
* Use `@param` instead of `@var` for parameters
* Fix type of `$attributeGroups` in `AttributeInfo::createFromGroups()`
* Remove extra documentation of `$allConstInfo` for
`ClassInfo::getClassSynopsisDocument()`, it is already documented under the
correct name `$allConstInfos`
* Remove unneeded `@throws`
Reduce the number of global functions by moving it to instance method
`FileInfo::handleStatements()`.
Reduce the number of global functions by moving it to static method
`FileInfo::handlePreprocessorConditions()`. Since it is only used by
`FileInfo::handleStatements()`, also make it private.
Reduce the number of global functions by moving it to static method
`DocCommentTag::parseDocComments()`.
For a lot of the structures, the parsing of doc comment tags is based on if a
specific tag is present, or the value that it has if it is. Add a new helper
method, `DocCommentTag::makeTagMap()`, that turns an array of tag instances
into a map from tag name to value (the last value, if there are multiple uses
of the same tag name). Then, for the simple cases where just a tag's presence
is all that is checked, or just the (last) value is used, check the map instead
of using a loop through all of the tags present.
Separate out the creation of a legacy version of a FileInfo object, which has
information for old versions of PHP discarded, from its subsequent use in
`processStubFile()`.

In the process, make `FileInfo::$legacyArginfoGeneration` private, and inline
the single use of `FileInfo::getAllClassInfos()`, removing that method.
The following properties are made private:
* `ArgInfo::$phpDocType`
* `ClassInfo::$flags`, `::$attributes`, `::$extends`, `::$implements`
* `FileInfo::$isUndocumentable`

The following are made protected:
* `VariableLike::$flags`
Reduce the number of global functions by moving it to static method
`FileInfo::parseStubFile()`. Additionally, make `FileInfo::handleStatements()`
private now that the only caller is part of the class.
@DanielEScherzer
Copy link
Member Author

Interactive rebase with just the fixes requested: https://github.com/php/php-src/compare/858872133c3594cbbfc3661a6167cc725a2d6229..2b1352289fd4fecdd7504e771fbc5bfed489fded
And then rebased to pick up latest gen_stub changes and confirm that nothing breaks (tested after each of the commits that nothing changes when running build/gen_stub.php -f --generate-optimizer-info --verify) and good thing I did, 940ee1a would have broken things, needed to update another caller of generateVersionDependentFlagCode() https://github.com/php/php-src/compare/2b1352289fd4fecdd7504e771fbc5bfed489fded..87d7cefb30341b1c3de01b34c097deefa027704a#diff-5df7a541a3649c4a1ad7870312ebdcd836b6c36dc1dc65c8fda8271c210e014d

@DanielEScherzer DanielEScherzer merged commit 2b0cb76 into php:master May 8, 2025
9 checks passed
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.

2 participants