Add support for constants in stubs

[kocsismate]

No description provided.

[nikic]

The key problem with constants is that they are usually define to the value of a C constant. I just did a grep of "REGISTER.*CONSTANT", and it looks like the vast majority is of that kind. There's only few cases that define the constants as a fixed value.

In many cases, the value being assigned is not even controlled by PHP, but a symbol exported by some library. E.g. all the curl constants.

[kocsismate]

The key problem with constants is that they are usually define to the value of a C constant. I just did a grep of "REGISTER.*CONSTANT", and it looks like the vast majority is of that kind. There's only few cases that define the constants as a fixed value.
In many cases, the value being assigned is not even controlled by PHP, but a symbol exported by some library. E.g. all the curl constants.

Yeah, I'm aware of this issue, but I think it would be OK to generate constants for values which we can control. We could generate the C constant from the PHP constant by default (DateTimeZone::AFRICA -> PHP_DATE_TIMEZONE_GROUP_AFRICA), either based on convention or configuration (@cname). How does it sound to you?

[nikic]

@kocsismate I don't think this would cover enough use cases. Constant values that we don't control are really common, probably the majority. Even if we do control them, the C constants may have different scope, e.g. be defined in a header and used in multiple files. arginfo.h should only be included once.

The only way I can see this working is if we allow referencing C constants from stubs, like /** @var string */ const MYSQLI_REFRESH_GRANT = c\REFRESH_GRANT or whatever.

But this doesn't really seem better than what we have now. What is the motivation for moving this to stubs?

Also @sgolemon may have some insight here. I think they had some bad experiences with doing this in HHVM...

[kocsismate]

But this doesn't really seem better than what we have now. What is the motivation for moving this to stubs?

My main motivation is that adding support for constants in gen_stubs could improve the DX of developing extensions, and also, we could autogenerate more class synopsis pages without any manual intervention (after the initial sync).

So I'm not saying that we should migrate all the constants of php-src to stubs or support all the use-cases, but IMO a few major extensions (like ext/json, ext/spl, ext/date) would benefit from some kind of autogeneration support, not to mention 3rd party or new extensions.

Even if we do control them, the C constants may have different scope, e.g. be defined in a header and used in multiple files. arginfo.h should only be included once.

I agree that It's a valid concern (and I didn't yet consider it). But maybe we could generate a new header file for the C constants, and reference it during the registration of PHP constants/in the implementation of internal functions in order to solve the problem.

[kocsismate]

I managed to implement a POC, tests should pass now.

[cmb69]

Even if we wouldn't use the constants in the stubs for the implementation (or only some of these constants), this appears to be still useful for IDEs/static analyzers and for the PHP manual, even if the value of the constants would just be UNKNOWN.

[kocsismate]

Even if we wouldn't use the constants in the stubs for the implementation (or only some of these constants), this appears to be still useful for IDEs/static analyzers and for the PHP manual, even if the value of the constants would just be UNKNOWN.

I agree: as it turned out, the majority of the stub-class synopsis diffs comes from the missing class constant support (43 files), so we would make a huge leap towards being able to fully autogenerate class declarations in the manual by adding constant support in stubs, as only ~20-25 files would need to be fixed afterwards.

[nikic]

@kocsismate I mean that the stubs take care of registering the constants (from a PHP perspective), but the actual value of the constant is usually defined by a C constant. I don't really see a risk for discrepancies there.

[kocsismate]

I mean that the stubs take care of registering the constants (from a PHP perspective), but the actual value of the constant is usually defined by a C constant. I don't really see a risk for discrepancies there.

Ok, understood now, and it would make this PR easier to implement in the expense of the class synopses in the manual not being able to list the exact constant default values: as I noted before, I think it would be a moderate to serious regression compared to what we have now, but if necessary, I could live with it if this is the cost of having a consensus regarding this PR. @cmb69 do you have preference/input about class constant values? I'd highly appreciate the input from Bob, Derick, and Ilija as well :)

[cmb69]

[…] it would make this PR easier to implement in the expense of the class synopses in the manual not being able to list the exact constant default values: as I noted before, I think it would be a moderate to serious regression compared to what we have now, […]

Tough call. I agree with Nikita that most of the values should not be documented, but I understand Bob's counterargument. From my experience, some user will submit a note listing the values anyway, so maybe we should actually drop the values from the manual. And we could still switch back if users complain.

[kocsismate]

Thanks, Christoph! If there is no objection, I'll simplify this PR for now according to Nikita's suggestion. Then I can provide followup changes if needed.

[bwoebi]

Another alternative could be saying "constant is THIS_DEFINE, look in the specific documentation", without outright stating the value - for external constants. Internal constants obviously can be trivially documented.

[kocsismate]

I reworked the PR yesterday: now, constants are only allowed to be declared with an UNKNOWN or a constant value. If they are unknown, they must refer to a C constant by using the @cname annotation. This way, there's no need for generating the C constants themselves, and I was able to simplify the code in some edge cases. The only downside I see besides the missing info about the exact values is that the C constant names are still hardcoded in the stubs.

Another alternative which I considered is requiring C constant values to be passed as arguments for the function which registers the PHP constants (i.e. register_php_date_consts()). This solution would have solved the above issue, but it would have caused inconvenience when an extension declared lots of constants (e.g. ext/curl or ext/intl), so finally I decided to drop this option.

[bwoebi]

I'm quite unhappy with it. Nearly all constants will have a C define, and if it's only to consume the value in C as well. Meaning that the stubs effectively will end up as either a) a mere constants registry without values or b) need to duplicate the values hardcoded.

There are quite a few instances where the constants value has not only value in debugging, but also when coding directly. Just as an example from this PR: the date constants describing formats. Currently they're all well described here: https://www.php.net/manual/en/class.datetimeinterface.php#datetimeinterface.synopsis - removing them would be quite the regression. And stubs should be usable in a way exposing these values as well.

I'm currently wondering whether it would make sense to preprocess stubs and substitute C defines. I.e. the generator script would modify the stubs themselves and ensure the correctness of the values. So for example, a stub defined as:

/**
 * @cname DATE_FORMAT_RFC3339
 */
const DATE_ATOM = UNKNOWN;

will search the neighboring c and header files for ^#define\s+DATE_FORMAT_RFC3339 (?<value>.*) and substitute it in the PHP stub directly, resulting in:

/**
 * @cname DATE_FORMAT_RFC3339
 */
const DATE_ATOM = "Y-m-d\TH:i:sP";

(Obviously only applicable if the expression is not complex, but a simple expression or string nor multiply defined (like within #if)... which 99% of the constants are.)

That way round the PHP stubs are directly usable in development environments with cross-referencing. The github actions check verifying that running the stubs generator doesn't change anything will ensure that the values are also up to date.

Obviously, that way, we expose the constants values. But I think this is a net positive.

[kocsismate]

I'm quite unhappy with it.

I also liked the previous version more, but the current one follows a more conservative solution, which we can occasionally improve easily if needed.

Meaning that the stubs effectively will end up as either a) a mere constants registry without values or b) need to duplicate the values hardcoded.

I don't completely disagree with you, but IMO having only constant names and types is still useful. E.g. originally, Ondrej asked us to provide info about constants in the stubs (https://bugs.php.net/bug.php?id=80188) for him to be able to use them in PHPStan. Back then we didn't have such plans, but in the meanwhile I changed my mind because adding constants would make a huge step forward for being able to completely autogenerate the class synopsis manual pages. In my opinion, this is more valuable than displaying the exact values.

I'm currently wondering whether it would make sense to preprocess stubs and substitute C defines.

To be honest, this solution would be way more esoteric than mine used to be, so I don't think we should go this way. However, I also came up with another idea: what about simply hardcoding the exact constant values in the stubs along with the @cnames? Then gen_stub.php could generate asserts to ensure that the C and PHP constant values are equal. Of course, this would only work with very basic values, but maybe it's also worth considering... I admit though that neither this solution is ideal. :)

[nikic]

@bwoebi To be clear, I don't think we should omit all constant values from docs. The date format strings are certainly a case where the value must be included in the documentation. However, the vast majority of the constants we have are really enum-like, where no exact value is guaranteed and often not even controlled by us (but rather a 3rd-party library).

The way I would view this is that the constant value is on the same level as the constant description: Something that needs to be curated by a human, not indiscriminately generated just because we happen to know it (on some PHP version).

[kocsismate]

The date format strings are certainly a case where the value must be included in the documentation.

DateTimeInterface class constants are just "aliases" to global ones, so this very class synopsis page would be autogenerated something like this:

interface DateTimeInterface
{
    public const string ATOM = DATE_ATOM;
    ....

And the exact value of the global constants are documented on the bottom of the very same page: https://www.php.net/manual/en/class.datetimeinterface.php#datetime.constants.types

[bwoebi]

@nikic But still, the way it is now, things are manually curated, if we want a C constant, it may go out of sync with the "truth". Similarly e.g. E_* constants are quite useful to be documented. It's not like we're just changing values anytime. While in a pure academical sense they wouldn't have to be documented or the values considered public API, in a practical sense, it would constitute a practical BC break.

@kocsismate Yeah, the documentation is sort of portraying it like the class constants are the source of truth and the top-level constants aliases for them. Btw. at the bottom of the page are only examples, not the values directly.

In my opinion, this is more valuable than displaying the exact values.

Yes, sure. But I'm just wanting to push one step further :-)

Then gen_stub.php could generate asserts to ensure that the C and PHP constant values are equal.

That sounds like an acceptable compromise :-) Generating a bunch of _StaticAssert(). (At least for any non-nested (i.e. nesting other constants) value)

DateTimeInterface class constants are just "aliases" to global ones, so this very class synopsis page would be autogenerated something like this:

Maybe it should be defined the other way round in the stubs then ... to reflect the manual. Doesn't do any harm I guess :-D

[kocsismate]

OK, I made another significant change: now, constants in the stubs are allowed to have a literal value, which are only exposed for the manual and 3rd party tools, but php-src doesn't use them directly. In order to ensure that the documented values in the stubs match the real values defined in C, I added ZEND_ASSERTs after the registration code. (Until now, I wasn't aware what _StaticAsserts do, or if we can use them at all, but I am also ok to go with compile-time assertions.)

With the current approach, we are able to selectively choose what values to expose for the manual without introducing disruptive changes for the C codebase. The downside we should accept is that some constant values will be duplicated, but IMO this is an acceptable compromise due to the added consistency checks, and at least we don't lose the ability to conveniently document constant values.

Maybe it should be defined the other way round in the stubs then ... to reflect the manual. Doesn't do any harm I guess :-D

I achieved this by recursively finding the first literal value of a constant. So with the latest commit, constant values of DateTimeInterface won't change in the docs.

WDYT?

[bwoebi]

I think what we have now is pretty reasonable :-) Thanks for your effort @kocsismate !

[kocsismate]

@nikic Is the new approach satisfactory for you? If the PR is OK conceptually, and there are no serious concerns/issues with the implementation, I'd like to go ahead with finishing the PR soon... But of course I can provide followup changes and fixes if necessary.

[kocsismate]

I'm going to merge this PR at the end of next week, unless there's still significant problems with the implementation/concept.