Various fixes for extgen - #2596
Open
alexandre-daubois wants to merge 17 commits into
Open
Conversation
A single ast.Field declares every parameter that shares a type, so "func add(a, b int64)" was counted as one parameter. Any exported function or method using grouped parameters was rejected with a bogus count mismatch and silently dropped from the generated extension, and the per-parameter type check compared PHP parameters against the wrong Go types. Flatten the field list into one entry per parameter before counting and indexing.
phpTypeToGoType exempts string, array and callable from the extra pointer level it adds for nullable types, but not mixed, so "?mixed $v" was validated as **C.zval while the generated C declares "zval *v = NULL" and passes a single pointer. Writing the correct *C.zval got the function dropped with a type mismatch; obeying the validator produced a cgo pointer mismatch. A zval already carries IS_NULL, so mixed belongs with the other pointer types.
The callable local was the only generated declaration without an initializer. ZEND_PARSE_PARAMETERS leaves the variable untouched when an optional argument is omitted, so "?callable $cb = null" handed an indeterminate pointer to the Go export, which frankenphp.CallPHPCallable then dereferences. Also switch the surrounding case to the phpCallable constant used by every sibling branch, so a rename cannot silently disable it.
Method wrappers were exported as "<method>_wrapper", so two exported classes declaring a method with the same PHP name emitted the same cgo symbol. An extension with User::getName() and Group::getName() failed to build with "getName_wrapper redeclared in this block". Prefix the symbol with the PHP class name, which the validator already constrains to a valid identifier.
…ses them The Go wrapper template hand-rolls its own PHP-to-Go parameter mapping, and it disagreed with what the C template actually passes: an array parameter was declared *C.zval while C passes a zend_array*, and mixed had no branch at all, so the C template emitted neither a declaration nor an argument and produced "Box_filter_wrapper(intern->go_handle, )". Type arrays as *C.zend_array and give mixed the zval branches it needs on both sides, matching what paramparser.go already does for standalone functions.
mixed is in supportedTypes, but generateReturnCode had no case for it, so a PHP_FUNCTION declared "zval *result = go_x();" and then dropped it: PHP always received null, plus an unused-variable warning. Class methods had no mixed return branch at all. The Go side could not work either, since phpReturnTypeToGoType mapped mixed to "any", which cgo refuses to export. Map mixed returns to unsafe.Pointer like string and array, and emit RETURN_COPY_VALUE on both the function and the method path.
The wrapper called structObj.{{.Name | title}}, guessing the Go method
name from the PHP one, while nothing ever compares the two names. A
method exported as Cache::get_value() backed by (*Cache).GetValue()
generated structObj.Get_Value() -- sprig's title capitalizes after every
non-letter -- and the extension failed to build.
Use the Go source already captured on the method, and teach
extractGoFunctionName to skip a receiver so it works for methods.
Four places mapped PHP types to Go types: phpToGoTypeMap, the validator's phpTypeToGoType and phpReturnTypeToGoType, and a per-type if-chain in the wrapper template that shadowed the map for exactly the types where they disagreed. That drift is what typed array parameters as *C.zval and left mixed with no branch at all. Call the validator's mapping straight from the template, so the generator cannot emit a signature the validator would reject. phpToGoTypeMap goes away with it, along with the test that pinned its "string" and "*frankenphp.Array" entries -- neither ever appeared in generated output.
CValue only rewrote the "0o" prefix, so digit separators and binary
literals reached the generated C verbatim: "const MAX = 1_000_000" emitted
#define MAX 1_000_000 and REGISTER_LONG_CONSTANT("MAX", 1_000_000, ...),
both syntax errors. determineConstantType accepts them via ParseInt base
0, so they were typed as int and passed straight through.
Reformat the spellings C cannot read as decimal; hexadecimal and C octal
are left as written.
determineConstantType classifies a backquoted Go raw string as phpString,
but the literal was stored and emitted unchanged, giving
REGISTER_STRING_CONSTANT("GREETING", `hi`, ...) in C -- backticks are not
a string delimiter there -- and a PHP stub that gen_stub.php cannot parse.
Re-quote raw strings as regular double-quoted strings, which both
languages accept.
Both constant regexes required the name to be followed directly by "=", so a constant declared with an explicit Go type matched neither. "//export_php:const" above "const Perm os.FileMode = 0o755" aborted the whole generation with "invalid constant declaration at line N", and inside a const block "SMALL int64 = 1" was dropped without a warning. Allow an optional type between the name and "=".
A method is only attached to a class whose name matches an //export_php:class directive; a method naming any other class was dropped without a warning, while an orphan class directive is already a hard error. Renaming a struct's exported class and forgetting one //export_php:method produced an extension where that method simply did not exist, and generation still reported success. Match methods against the declared class directives, which also puts the directive's class name -- collected but never read until now -- to use.
parse built a full AST, then parseMethods read the file again and built a second AST of the same source with its own FileSet, so line numbers from the two halves were not comparable by construction. Pass the source and the AST down instead. While there, drop the double regexp pass over the directive comment: findDirective's capture was discarded and the comment re-scanned with findMatchingComment. Returning the *ast.Comment gives both the payload and the position in one match.
Four warning sites wrote to os.Stderr directly instead of the package's warnf helper, which exists so tests can capture warnings via warnOut. The whole warn-and-skip path of the class parser was therefore untestable, and its warnings landed on a different stream than the function parser's.
Parse forwarded verbatim to parse and had no caller: parser.go and every test use the unexported one.
extractVariables and extractInternalFunctions fed goTemplateData.Variables, InternalFunctions and Constants, none of which extension.go.tpl ever references: roughly 110 lines of hand-rolled brace and paren counting -- which mis-parses braces inside strings and comments, and only looks back five lines for a directive -- computed and thrown away on every run, plus 450 lines of tests. The generated file lives in the same package as the source, so copying those declarations would be a redeclaration anyway. analyze now returns just the package name it is actually used for, and the defensive copy of Classes goes with it since the template only reads the slice.
The "Class methods support" column marked mixed and callable as unsupported. callable already worked in class methods before this branch and mixed works now, so the table steered users away from working features. The Go column also gave `any` for mixed where the signature must be *C.zval, unlike the string and callable rows which give the real signature type. The French and Brazilian Portuguese tables predated the callable row entirely, so it is added there; pt-br also had a duplicated `?bool` row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commits to be reviewed separately to understand each fix