-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Conversation
@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. |
@DanielEScherzer Yes, definitely! I got some chance to review things these days, and your PR will be the next thing - hopefully tonight :) |
There was a problem hiding this 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
build/gen_stub.php
Outdated
@@ -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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
8588721
to
2b13522
Compare
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.
2b13522
to
87d7cef
Compare
Interactive rebase with just the fixes requested: https://github.com/php/php-src/compare/858872133c3594cbbfc3661a6167cc725a2d6229..2b1352289fd4fecdd7504e771fbc5bfed489fded |
Each commit can be reviewed independently, and should have no functional changes