strict-api - Proposing a tool to check and enforce encapsulation

9 years ago by francois@php.net — view source

unread

Hi,

I have written a follow-up to my previous 'zstrict' PR. It is now
focused on a simple mechanism to detect API violations :

https://github.com/php/php-src/pull/1508

This version just implements the mechanism and applies it to the
zend_string val/len/h elements. More details in the PR.

Please comment first. Then, when we're OK on the feature, we may talk
about the target version.

Regards

François

L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

9 years ago by Anatol Belski — view source

unread

Hi Francois,

-----Original Message-----
From: François Laupretre [mailto:francois@php.net]
Sent: Tuesday, September 8, 2015 12:41 AM
To: PHP internals internals@lists.php.net
Cc: Ferenc Kovacs tyra3l@gmail.com; Anthony Ferrara
ircmaxell@gmail.com
Subject: [PHP-DEV] strict-api - Proposing a tool to check and enforce
encapsulation

Hi,

I have written a follow-up to my previous 'zstrict' PR. It is now focused on a
simple mechanism to detect API violations :

https://github.com/php/php-src/pull/1508

This version just implements the mechanism and applies it to the zend_string
val/len/h elements. More details in the PR.

Please comment first. Then, when we're OK on the feature, we may talk about
the target version.

Personally I'd be voting yes on the principle. However as the topic brought controversial opinions in the past, IMHO it is better to solve it through an RFC. Especially when the idea expands over zend_string API later, evermore devs and code will be forced by the rules, so it is essential to ensure the majority wants it in first place. And, I wouldn't hurry it into 7.0.

Regards

Anatol

9 years ago by Kalle Sommer Nielsen — view source

unread

2015-09-12 17:57 GMT+02:00 Anatol Belski anatol.php@belski.net:

Personally I'd be voting yes on the principle. However as the topic brought controversial opinions in the past, IMHO it is better to solve it through an RFC. Especially when the idea expands over zend_string API later, evermore devs and code will be forced by the rules, so it is essential to ensure the majority wants it in first place. And, I wouldn't hurry it into 7.0.

I agre, but I don't see any chance of this making it into the 7.0
branch at current state, so I would recommed targetting 7.1 at
earliest

--
regards,

Kalle Sommer Nielsen
kalle@php.net

9 years ago by francois@php.net — view source

unread

Le 12/09/2015 17:57, Anatol Belski a écrit :

Personally I'd be voting yes on the principle. However as the topic brought controversial opinions in the past, IMHO it is better to solve it through an RFC.
Especially when the idea expands over zend_string API later, evermore
devs and code will be forced by the rules, so it is essential to ensure
the majority wants it in first place. And, I wouldn't hurry it into 7.0.

OK. I'll write an RFC targeting 7.1. You're right : as long as it was an
optional tool, incorporating it without an RFC could be discussed, but
deciding to perform travis checks in zstrict mode requires a vote.

About the option name, I proposed some in the PR in reply to Ferenc's
suggestions. Which one do you prefer ?

Regards

François

9 years ago by Bob Weinand — view source

unread

Am 08.09.2015 um 00:41 schrieb François Laupretre francois@php.net:

Hi,

I have written a follow-up to my previous 'zstrict' PR. It is now focused on a simple mechanism to detect API violations :

https://github.com/php/php-src/pull/1508

This version just implements the mechanism and applies it to the zend_string val/len/h elements. More details in the PR.

Please comment first. Then, when we're OK on the feature, we may talk about the target version.

Regards

François

While I don't disagree with the idea of making accidental direct access harder, I see a particular disadvantage for debugging.

Nobody will like to write "p fbc->op_array.filename->zstrict_field_zend_string_val" (I hope I got it right?) just to figure out where the targeted op_array is located. [At least lldb only shows one character of the value when just printing the zend_string, due to the struct hack...]
Now you may say macro? Well, honestly, I don't usually use macros (as .gdbinit isn't compatible with lldb and lldb is more stable than gdb on OS X)

While it brings a minor guarantee that the ZSTR_*() API is really used, it makes debugging much harder; so I prefer to not merge this patch.

Thanks for your efforts here though!

Bob

9 years ago by francois@php.net — view source

unread

Hi Bob,

Le 13/09/2015 00:44, Bob Weinand a écrit :

While I don't disagree with the idea of making accidental direct access harder, I see a particular disadvantage for debugging.

Nobody will like to write "p fbc->op_array.filename->zstrict_field_zend_string_val" (I hope I got it right?) just to figure out where the targeted op_array is located. [At least lldb only shows one character of the value when just printing the zend_string, due to the struct hack...]
Now you may say macro? Well, honestly, I don't usually use macros (as .gdbinit isn't compatible with lldb and lldb is more stable than gdb on OS X)

While it brings a minor guarantee that the ZSTR_*() API is really used, it makes debugging much harder; so I prefer to not merge this patch.

I was probably not clear enough : you will never debug code
configured/compiled in zstrict mode. This mode is reserved for
compile-time checks only. Once you have checked that everything compiles
fine in zstrict mode, the next step is to 'make clean' and throw the
compilation result away. You can 'make test' before, if you want, but
you will never distribute nor debug code compiled in zstrict mode.

The same with travis: the zstrict flag can be set only because the
resulting code will never be distributed nor used for debugging.

So, there's no question of debug macro nor .gdbinit. When debugging your
code, you'll keep using the usual field names
(fbc->op_array.filename->val for instance).

Regards

François

9 years ago by Bob Weinand — view source

unread

Am 13.09.2015 um 03:24 schrieb François Laupretre francois@php.net:

Hi Bob,

Le 13/09/2015 00:44, Bob Weinand a écrit :

While I don't disagree with the idea of making accidental direct access harder, I see a particular disadvantage for debugging.

Nobody will like to write "p fbc->op_array.filename->zstrict_field_zend_string_val" (I hope I got it right?) just to figure out where the targeted op_array is located. [At least lldb only shows one character of the value when just printing the zend_string, due to the struct hack...]
Now you may say macro? Well, honestly, I don't usually use macros (as .gdbinit isn't compatible with lldb and lldb is more stable than gdb on OS X)

While it brings a minor guarantee that the ZSTR_*() API is really used, it makes debugging much harder; so I prefer to not merge this patch.

I was probably not clear enough : you will never debug code configured/compiled in zstrict mode. This mode is reserved for compile-time checks only. Once you have checked that everything compiles fine in zstrict mode, the next step is to 'make clean' and throw the compilation result away. You can 'make test' before, if you want, but you will never distribute nor debug code compiled in zstrict mode.

The same with travis: the zstrict flag can be set only because the resulting code will never be distributed nor used for debugging.

So, there's no question of debug macro nor .gdbinit. When debugging your code, you'll keep using the usual field names (fbc->op_array.filename->val for instance).

Regards

François

Well, that either just makes workflow unnecessarily complex or many devs just won't care too much about it… and these who do may be annoyed to fix others code before it's useful for themselves...
(Also, I don't think like some devs will be particularly eager to recompile before each push … it doesn't take just one minute on every machine)

Additionally, extension devs… They'd have to switch target php install while coding, just to check against zstrict install and no zstrict. Well… that just begins to become insane.

Sorry, but making zstrict optional is just plain annoying of developers...

Bob

9 years ago by francois@php.net — view source

unread

Hi,

Le 13/09/2015 10:41, Bob Weinand a écrit :

Well, that either just makes workflow unnecessarily complex or many devs just won't care too much about it… and these who do may be annoyed to fix others code before it's useful for themselves...
(Also, I don't think like some devs will be particularly eager to recompile before each push … it doesn't take just one minute on every machine)

I'm not sure I understand what you mean. I agree that running travis
checks in zstrict mode introduces additional constraints for core
developers. But, I would say that's the price to pay to maintain some
level of isolation/encapsulation. I know we don't always agree on the
importance of encapsulation but zstrict is all about it.

The decision we need to make is whether core devs are ready to accept
these constraints to maintain some level of isolation in the code. As I
already said, zend_string is not the target. The real target is to
eliminate tight coupling on zend_hash and other complex APIs.

Additionally, extension devs… They'd have to switch target php install while coding, just to check against zstrict install and no zstrict. Well… that just begins to become insane.

Extension devs won't have to switch between zstrict and non-zstrict PHP
installs. If you look at the code, you will see that there are two
configure flags, one for the core, one for extensions. Extensions will
always be configured and compiled against a non-zstrict compiled core,
and zstrict mode will be set in the extension configure command. It is
different from other configure flags inherited from the core. So, once
again, you will never keep nor use a zstrict-compiled install tree
(which would be insane, I agree).

Sorry, but making zstrict optional is just plain annoying of developers...

Regards

François

9 years ago by Pierre Joye — view source

unread

Le 13/09/2015 04:14, Sean DuBois a écrit :

That sounds good to me!

That would double build times though, is that going to be a concern?
This won't hurt development time though, since you would dev in zstrict,
and then switch to non-zstrict when running/testing?

Why would it double build times ? This is an optional tool, developers
will run a zstrict build when they want to check their code against API
'best practices'. Some will do it more often than others, but that's their
choice. Some ext devs will probably never use it, that's their decision.

The only constraint will be for core developers. If travis runs in
zstrict mode, every non-zstrict code will cause it to fail. So, core
changes will need to be 'strict'. That's why this decision needs a vote.

The more I think about it the more I think it should be part of the code
quality analysis process, like static analyzers and the likes.

Maybe a llvm plug-in or we'll configured options for existing analyzers
(like with VC)

Optional build options may not achieve what this proposal does. Except if
we enforce it.