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

PHP type declarations #2027

Closed
ojwb opened this issue May 26, 2021 · 18 comments · Fixed by #2148
Closed

PHP type declarations #2027

ojwb opened this issue May 26, 2021 · 18 comments · Fixed by #2148
Labels
Milestone

Comments

@ojwb
Copy link
Member

ojwb commented May 26, 2021

(Splitting out from #1529)

Modern PHP supports type hints on function parameters and return types. It'd be nice if SWIG could add these (probably ideally automatically with potential to override manually.)

We require PHP7 now, so we don't need to worry about versions without type hints, but PHP8 added the ability to have or-ed hints, which has a different C API which complicates the code we'd need to generate.

I'm going to soft-target this at SWIG 4.1, as doing this in the same release as the other major PHP backend updates seems desirable. I may not have time, but we'll see. If the hints were opt-in it could probably go in any SWIG release, so maybe that's a better way to go.

@ojwb ojwb added the PHP label May 26, 2021
@ojwb ojwb added this to the swig-4.1 milestone May 26, 2021
@geographika
Copy link
Contributor

Sounds a nice feature for PHP users. Is the plan to annotate using the PHP types rather than C/C++ ones?
Will keep an eye on any developments to see if it provides an approach that could be used for Python (see #735 discussions).

@ojwb
Copy link
Member Author

ojwb commented May 29, 2021

PHP type hints specify the allowed type(s). For PHP < 8, you can only specify one type, but there are some options to allow multiple types (e.g. object, iterable, callable). PHP 8 allows specifying alternatives, so something like int|array is possible.

My idea so far is to specify these via typemaps, so you might have:

%typemap(typehint) char []
 "MAY_BE_STRING"

%typemap(typehint) char *, char *&
 "MAY_BE_STRING|MAY_BE_NULL"
 
%typemap(typehint) SWIGTYPE *const&
 "MAY_BE_OBJECT|MAY_BE_NULL"

(These MAY_BE_ constants are part of PHP's C API - we no longer generate PHP code, so we need MAY_BE_STRING which is equivalent to writing string in PHP code.)

@ojwb
Copy link
Member Author

ojwb commented May 31, 2021

@geographika After a quick look at Python annotations, perhaps the Python equivalent would look something like:

%typemap(annotation) char []
 "str"

(Not sure how/if "None-able" types are specified/supported - PEP3107 seems to be mostly about the framework rather than how it's used and I couldn't see another PEP building on it.)

If there's an approach that would work better for Python annotations and similarly well for PHP type hints I'm happy to discuss as having a common approach between languages has benefits, but soon would be good if this is to make SWIG 4.1.

@ojwb
Copy link
Member Author

ojwb commented Dec 10, 2021

I've pushed some initial work to the php-type-hints branch.

This annotates most of SWIG/PHP's in typemaps with phptype attributes, and emits them in the generated wrapper for non-overloaded functions. This seems to work - at least it doesn't break the test suite in local testing.

More work is needed to properly handle overloaded functions (just enabling the current code would emit the type info for the last overloaded form of each overloaded function - that's incorrect, though doesn't cause the testsuite to fail - probably we need more _runme.php files for overloading testcases).

We should also annotate with a particular class name rather than just "object" where appropriate.

I've not attempted return type annotations yet - probably needs similar annotations on out typemaps.

Probably directorout should be done like in (and directorin like out).

And I've only attempted to do this for PHP8 so far as it seems to make sense to focus efforts there at least until we have this working for PHP8.

PHP7 is less versatile as it doesn't allow alternatives in type specifications (only a single type though that can be nullable), or at least not if you write PHP code - I've been assuming that's true via the API though haven't checked. If that assumption is correct, to support this for PHP7 we'd have to essentially emit two arginfo versions for functions which take alternate types, and select the appropriate one at C/C++ compile time.

PHP7.4 (the final 7.x) is now out of active upstream support and has security support for about 11 months (until 28 Nov 2022) so I'm leaning towards this being a PHP8+ only feature.

@ojwb
Copy link
Member Author

ojwb commented Dec 10, 2021

Working on adding return type annotations, I've come across the issue that doing this can breaks PHP code subclasses wrapped classes, e.g.:

Declaration of MyFoo::ping($s) must be compatible with Foo::ping(string $arg1): string in /home/olly/git/swig/Examples/test-suite/php/director_stl_runme.php on line 13

This clearly means we can't just turn this on all the time, and probably argues for allowing control of it per class or even per method.

@ojwb
Copy link
Member Author

ojwb commented Dec 11, 2021

I've updated the branch and it now generates return type declarations for non-directed methods.

To do:

  • Properly handle overloaded functions which currently don't get any type annotations (just enabling the current code would emit the type info for the last overloaded form of each overloaded function - that's incorrect, though doesn't cause the testsuite to fail).
  • Add missing _runme.php files for overloading testcases.
  • We should also annotate with a particular class name rather than just "object" where appropriate.
  • Allow enabling return type annotations for directed methods.
  • Handle directorout like in?
  • Handle directorin like out?
  • Support for class properties? Is that possible for how SWIG wraps these?
  • Document.
  • Generate code which works for PHP7 (though probably without type declarations)
  • Allow selectively enabling/disabling generation of type declarations
  • Support type declarations for argout OUTPUT typemaps which modify the return value - fiddly to update correctly I think - if there's also a return value or more than one argout typemap then the type declaration should be array.
  • The generated foo_set() which wraps a variable foo currently seems to lack argument type declarations; foo_set() and foo_get() do both have return type declarations though. It looks like the in typemap gets used here but doesn't get attached to the parameter entry.

@ojwb
Copy link
Member Author

ojwb commented Dec 11, 2021

It seems the PHP docs now call these "type declarations", which makes more sense as passing the wrong type gets you a TypeError exception, so they aren't just a "hint":

Type declarations can be added to function arguments, return values, and, as of PHP 7.4.0, class properties

I wonder if we can support these for class properties. I'll add a note to the to do list above.

@ojwb ojwb changed the title PHP type hints PHP type declarations Dec 11, 2021
@ojwb
Copy link
Member Author

ojwb commented Dec 12, 2021

My idea so far is to specify these via typemaps, so you might have:

%typemap(typehint) char []
 "MAY_BE_STRING"

%typemap(typehint) char *, char *&
 "MAY_BE_STRING|MAY_BE_NULL"
 
%typemap(typehint) SWIGTYPE *const&
 "MAY_BE_OBJECT|MAY_BE_NULL"

I've gone for these being a phptype parameter on the corresponding in or out (and perhaps other) typemaps, and the value specified is now what you'd write in PHP code as that's more natural for users, and naturally supports the same things that PHP code does (for example, specifying class names).

So for example:

%typemap(out, phptype="?string") char *
// ...

and

%typemap(in, phptype="int|string") TYPE *REFERENCE (unsigned long long lvalue)
// ...

(These MAY_BE_ constants are part of PHP's C API - we no longer generate PHP code, so we need MAY_BE_STRING which is equivalent to writing string in PHP code.)

This is now handled internally via a lookup table from the PHP type name to the corresponding C constant name.

@ojwb
Copy link
Member Author

ojwb commented Dec 15, 2021

I've got overloaded functions and automatically using class names (rather than just object) working now (well enough for the test suite as-is to pass at least).

@ojwb
Copy link
Member Author

ojwb commented Dec 18, 2021

I've made the new generated code work with PHP 7 (tested with 7.4 locally as that's all I have to hand) and have re-enabled CI for all PHP versions.

This works by simply defining the PHP API macros for specifying type declarations which were new in PHP 8.0 to the equivalents without type declarations if we're building for PHP 7.x.

@ojwb
Copy link
Member Author

ojwb commented Dec 18, 2021

I should probably explicitly state that I'm currently viewing the php-type-hints branch as essentially a personal sandbox which I may rebase and force push without warning. I'm pushing it mostly to allow CI testing (particularly with more PHP versions than I have to hand) and to have an up-to-date remote backup of my work.

I'd be delighted if anyone else is interested enough in this to collaborate (filling in missing _runme.php files would be particularly useful for example). If that's you please speak up and I'll treat the branch with a bit more care, and we can coordinate the work a bit.

@ojwb
Copy link
Member Author

ojwb commented Dec 22, 2021

1b22fef implements most of the missing overload_*_runme.php files.

@ojwb
Copy link
Member Author

ojwb commented Dec 22, 2021

After looking into it, I've concluded that directorout and directorin typemaps aren't relevant here. They are very like in and out typemaps in terms of converting arguments/return values, but we don't generate a PHP function which uses them (they go into a C++ method) so there isn't a use for a phptype annotation.

@ojwb
Copy link
Member Author

ojwb commented Dec 22, 2021

I've investigated types properties, and as far as I can see these aren't going to be possible (unless PHP adds a mechanism).

The key issue here is that we need to map to an existing place where the value is stored (e.g. a member variable in a struct or class.

You can declare a typed property with zend_declare_typed_property() but only as a zval and there's no equivalent of Perl's tie mechanism to call a function on reads/writes to the zval, nor a way to specify an C/C++ pointer to indirect through.

The current way we implement PHP properties is by implementing the "magic" (PHP's term) methods __get(), __set() and __isset() which are called when there's an attempted access to a property which wasn't declared. These don't provide a way to specify type information.

An alternative way we could implement PHP properties is via the zend_object_handlers, but again I can't see any way to specify type information.

ojwb added a commit that referenced this issue Dec 22, 2021
With C++ comments changed to C comments, these are now identical to
the two cases just above, aside from the `2` suffix on the names.

Follow-on for #2027.
@ojwb
Copy link
Member Author

ojwb commented Dec 23, 2021

Checking through the typemaps I spotted a few omissions in more obscure cases which I'll push fixes for.

I also noticed that argout typemaps should really support annotations too (since they modify what's returned). Where t_output_helper is used it's either array (if there's also a return value, or there are multiple OUTPUT parameters) or the actual type (if there's no return value and only one OUTPUT parameter). So that should be determinable when we generate the wrapper code, but it's seems fiddly to achieve.

Looking at these typemaps there also looks to be a bug in t_output_helper for PHP where for multiple OUTPUT parameters the returned array doesn't include any leading NULL values (or if all returned values are NULL, just NULL is returned), which doesn't seem helpful. I'll try to come up with a reproducer and fix if I'm right.

ojwb added a commit that referenced this issue Dec 23, 2021
@ojwb
Copy link
Member Author

ojwb commented Dec 23, 2021

Looking at these typemaps there also looks to be a bug in t_output_helper for PHP where for multiple OUTPUT parameters the returned array doesn't include any leading NULL values (or if all returned values are NULL, just NULL is returned), which doesn't seem helpful. I'll try to come up with a reproducer and fix if I'm right.

Well, it's genuine, though not triggerable without writing a custom typemap as all the shipped OUTPUT typemap sets always give non-NULL.

It seems hard to fix without adding some sort of counting of how many argout typemaps we've processed so far for a function and taking into account if there's a C/C++ return value to include or not. It seems Python has essentially the same issue too, so I'm going to ignore this I think.

@ojwb
Copy link
Member Author

ojwb commented Dec 23, 2021

152a24a adds support for %feature("php:type") which controls generation of type declarations and also provides a way enable return type declarations for directed virtual methods.

The semantics of this are:

    If unset or set to "0" then no type declarations are generated.
    
    If set to "1" then type declarations are generated for both
    parameters and return types.
    
    Setting it to "compat" is the same as "1" except no return type
    declarations are generated for virtual methods for which directors
    are enabled.  This provides better compatibility for PHP subclasses
    of wrapped virtual methods in existing SWIG-generated bindings

Currently I'm defaulting this in Lib/php/php.swg like so:

%feature("php:type", "compat");

@wsfulton Does that seem a reasonable approach?

It seems to work, but I couldn't seem to find an existing %feature which defaults to a non-empty value like this (though it's not very easy to search the source code for instances as %feature is often wrapped by another macro).

I could make the feature work with the unset setting meaning "compat", but that seems less easy to understand than the semantics above.

@ojwb
Copy link
Member Author

ojwb commented Dec 24, 2021

I've pushed a commit to document this in the PHP chapter of the manual.

The remaining two items on the tick-list above don't need to block the merge of this - currently we're missing type declarations in these cases, but the fall back is that PHP accepts any type so everything works (the same as things work with PHP7 where we don't support type declarations at all).

I'd really like to get feedback on how the feature to control this works, but that really just needs to be before 4.1.0 is released and doesn't need to block the merge of this, so I'll probably do a pass or two over these changes to look for anything I've missed and then merge so I can work on #2125 as that will touch some of the same places as these changes.

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

Successfully merging a pull request may close this issue.

2 participants