maintainer-strict-api - A tool to check and enforce encapsulation #1508
Conversation
|
I like the idea, but I don't like the name, we should either have a more verbose name like similar to the maintainer zts one or even make it more obscure than a configure option otherwise I'm afraid users will enable it out of curiousity. |
|
@Tyrael. Yes, that's right. 'strict' may confuse people thinking 'stricter' is better. The risk is the same as 'strict' scalar type hints. So, if you don't care about long options, what about sthg like :
While we discuss about the name, I keep fixing the remaining direct access to val/len detected by travis... |
flaupretre
force-pushed
the
zstrict-v2
branch
3 times, most recently
from
95fb8de to
2a0837b
Compare
|
@flaupretre I think we require a more in-depth discussion that this PR generated. Please can you rebase this PR, and start an internals discussion regarding the content, naming, and idea in general. This will help us to build consensus of the kind I think is required to merge this kind of work. Sorry this has sat here for so long (I personally like it, not sure about naming/options either). If you consider this work abandoned, please close this PR yourself. |
Create missing macros: Z_LINENO(), Z_NUM_ARGS(), _Z_VALUE(). Fix non-compliant code to use newly-created macros. Fix gc-related code to use GC_xxx() macros Fix comments in zend_types.h Move ZVAL_OFFSETOF_TYPE macro from zend_operators.h to zend_types.h
flaupretre
force-pushed
the
zstrict-v2
branch
2 times, most recently
from
04e3beb to
b69bbad
Compare
flaupretre
force-pushed
the
zstrict-v2
branch
from
b69bbad to
7a61cf0
Compare
|
@flaupretre did you start an internals discussion about this ? Please respond in a timely manner. |
|
@krakjoe Not yet, I'll start a discussion about it before the end of the week. |
flaupretre
changed the title
|
@flaupretre any plans for the Windows side of things here? |
|
I like this. When I've experimented with modifying the zend_string internal representation, I've done a similar thing, changing the field names so compile errors could tell me what needs fixing. |
|
@flaupretre can you bump your internals thread to the top of the stack in the hope that someone will respond please ? |
|
Comment on behalf of kalle at php.net: Closing due to inactivity |
This pull request is a follow-up to a previous PR, which was probably going too far. The present one focuses on the mechanism I want to introduce, and limits its first application to a basic case.
Parts of the text below are taken from the previous PR.
Why
Some months ago, Anthony Ferrara expressed concerns about PHP 7 and zend_hash growing complexity, combined with a lack of strict encapsulation (tight coupling).
Some replied that the solution is just a mix of comments and documentation, but growing complexity quickly becomes unmanageable if any code can bypass official/published APIs. zend_hash is a critical examples, but not the only one.
While part of the solution lies in documentation, 'quick-and-dirty' behaviors are not exclusively triggered by a lack of information. We also need tools to detect API violations.
An API violation is the fact of accessing resources by a way not respecting the official/published API, for any reason, wanted or not. An obvious case is directly accessing structure elements by their name, without using the available macros. While this code works, it is an example of tight coupling which, going unnoticed, creates a lot of potential issues in the future. For this reason, these violations must be detected and fixed as soon as possible, especially in a huge piece of code shared by many developers.
Protecting structure fields from direct access
The tool implemented here allows to detect direct access to structure fields, when an other mechanism (macro) is available.
From a user's point of view, activating this detection mechanism is very simple :
How does it work ? The option just modifies the field name of explicitely-protected fields. When some code attempts to access these fields using their usual name, an error is issued as this field name does not exist.
In other terms, Including this in 7.0 won't cause any extension maintainer to modify his code.
When the 'strict-api' configure option is not set (the default), the field names are unchanged and any code directly accessing such fields is valid. This way, extension writers can distribute 'non-strict' extensions and take their time to make them strict-compliant.
An example
Just another example to show why such a tool is really needed :
I don't know the reason and it is not important. It just shows that we need a mechanism to detect these violations as soon as possible. Ideally, such commits shouldn't have gone through but, without a mechanism to enforce the rule, it's impossible to detect. In order to solve this, as you will see in the patch, I modified the travis configure command to include the 'strict-api' configure option. So, such API violations will now be checked and rejected by the travis validation.
Protecting a structure field
Protecting a field is simple : everywhere it is legitimate to access the struct field by its name, replace the name with '_ZSTRICT_FIELD(<prefix>,<name>)', where <prefix> is the name of the structure containing the element, and <name> is the element name. For an example, check how we protected zend_string fields in this PR (zend_string.h/zend_types.h).
In general, only the first-level elements need to be protected, as they control access to the sub-elements.
Of course, protecting a struct field implies that an official API (macro/function) is available.
Coming next
The next structure to protect and encapsulate is the HashTable structure. Actually, the combination of tight coupling with the complexity of PHP7 HashTables makes it a critical concern. This really needs to be addressed before it becomes unmanageable.