[boost] [Review:Contract] Andrzej's review

Subject: [boost] [Review:Contract] Andrzej's review
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2012-08-29 17:48:24

Hi,
First, my vote is "YES": Contract should be accepted into Boost. Now for
some details...

=== MOST IMPORTANT THINGS ===

(1)
Let me start from the most important arguments. "Contract" enables us to
use DbC design and tools in C++. This is the only library in C++ that I am
aware of that takes DbC seriously, in every detail. In particular, unlike
any "simple contract library," it observes that: (a) function pre- and
post-conditions need to be part of declaration rather than function
implementation, (b) a precondition is something different than a
postcondition, (c) It is the execution of destructors of automatic objects
that may (re)establish invariants or satisfy postconditions (thus, they
need to be checked after destructors have been executed). "Contract" deals
with things more detailed than my above list; but even this list is not
always addressed in "simple contract libraries". This already gives me the
confidence that Lorenzo has a very good understanding of the domain.

I believe the community needs BbC framework: it is necessary in order to be
able to program consciously. If the responsibilities and expectations are
expressed clearly not only is it faster to track bugs, but also programmers
would not plant (I think) some bugs from the outset. Of course, contract
framework should ideally be provided as language extension; but in the
absence of such support we are left with the attempts to implement it via a
(possibly too clever) library. "Contract" may also enable and/or speed up
the adoption of DbC as language feature in C++, as it will be possible to
have a real-world implementation of the framework that people can play
with, experiment and assess its usability.

(2)
An obvious and serious issue with the library is the burdensome syntax you
have to use. Given the ambitious task of wrapping each function call with
with tests executed before and after, and still leaving the bodies intact,
I realize this is not avoidable. In fact I believe Lorenzo made the
impossible to make the EDSL as close to normal declaration syntax as
possible. Yet, all this may not be enough to call the interface
"acceptable". There is a number of issues with it: you have to learn yet
another language; compile times increase; if you  mis-spell something you
have horrible error messages; you cannot use some C++ constructs that you
would otherwise use; your IDE and the debugger may be confused (if your
contract fails, the bug may be either in your code or in the testing code).
I cannot imagine telling my coworkers "from today on we are using
Boost.Contract; start learning the new EDSL".

I believe that it is not possible to significantly improve the current
interface; its limitation is the natural consequence of the nature of the
problem. The cost cannot be reduced any more, so the library can only offer
even more value. And this is what it does, by (for instance) simplifying
the integration with Boost.Concept or adding virtual specifiers.

(3)
I see it as a useful tool for personal projects, experimentation, learning,
validation of usefulness of DbC, a proof of concept for proposal N1962. But
the question is, should "Concept" be included into Boost.

"Contract" needs inclusion into Boost to gain visibility and some sort of
"attestation" and "certification". Only proposing "Contract" for addition
to Boost brought attention to DbC: at least it prompted me to learn more.

But does Boost need "Contract"? Similar questions have been asked during
the review of Boost.LocalFunction. I am not aware of any objective criteria
that would help decide in this case. Is this library of "high quality"? Can
library with such interface be called "high quality"? But there is no other
way to implement DbC in C++ (I believe)? But is it strong enough the
argument? Perhaps we should treat some problems as "not implementable" and
not include libraries only because there is no cleaner way to implement the
tool. The decision to vote "yes" is not easily made. This is a subjective
weighing between the offered value and the imposed cost. Personally, I
would like to have this library available whenever I download Boost.

=== GENERAL OBSERVATIONS ===

(4)
The library has very vast and very thorough documentation. It contains
tutorials that teach you step by step how to use the library, the
introduction into DbC, very detailed rationale, good reference, lots of
examples. It is a good "reference documentation".

(5)
In order to define concept the library defines EDSL for declaring (and
defining) classes and functions. This framework as the potential to do
other useful things: enable support for concept definition and checking,
base class aliases (see N2881), override control, named parameters,
noexcept, simplified enable_if (or static if), etc... In fact, perhaps it
could be used as a more general EDSL for declarations. This is my loose
suggestion to change macro names from BOOST_CONCEPT_CLASS to
BOOST_DECL_CLASS, etc, in case someone wants to use the framework for
purposes other than contract enforcements.

=== ISSUES ===

Here I list a couple of random issues. I do not consider them critical for
decision whether to adopt "Contract" to Boost or not.

(6)
Static assertions: is there any value in providing them, given that we
already have this ability in Boost? I see that N1962 decided to abandon
them; I am pretty sure the reason was that static_assert was already there.
They somehow do not fit conceptually into the model of preconditions,
postconditions and invariants. Precondition, for instance tries to detect a
run-time condition occurring immediately before a function is called this
time. Static assertion is not even executed then. What point is there in
putting such assertion into the precondition? Documentation says " static
assertions can be selectively disabled depending on where they are
specified [...] so it is still important to logically associate them with
preconditions, postconditions, etc." -- technically this sentence makes
logical sense, but can you think of a single example where this selective
disabling of assertions would be useful? What is the point in disabling
such assertions at all, if they cost you nothing in run-time? I propose to
remove them and give a sort of recommendation that software quality is
assured by using a number of fairly independent tools: DbC, concepts,
static assertions.

(7)
Loop variants: They are definitely a good thing to have (and I started
using them immediately I learned about them in your documentation!). But
they seem to have little to do with "contracts". A contract is (by
definition -- I think) something that involves two parties. For instance,
precondition implies two parties: one ensures something, the other expects
something. Invariants are harder to explain this way, but to me they are
like "automated postconditions" (I explain this later on). However with
loop variants the situation is different: they are put inside the the code,
so no-one expect function author knows about them. The programmer
guarantees something only to himself. It is more like assertion (in the
sense of C macro assert()), perhaps with better configuration. I only want
to make an observation here that this could be proposed irrespectively of
contracts (pre-, post-conditions, invariants and subcontracting) -- but I
am not proposing this.

(8)
Block invariants: Same remark as above: they appear to be separate from
contracts. Another question is, what is the difference between block
invariant and an assertion (like C-style macro assert())? I know, I know:
conditional assertion, requirements, selectively disabling them,
customizing run-time behavior... But if this is superior to BOOST_ASSERT,
would it be correct to say that with Boost.Contract BOOST_ASSERT is no
longer needed (except for backward compatibility)?

(9)
Class invariants: I have an observation, not about yuor library but the way
invariants are handled in DbC methodology. As a "contract" (involving two
parties) they make sense when checked after any (public) function call:
they are a sort of postcondition that is the same for every function: class
implementer "ensures" and class user "expects". But when checked before any
function execution they are more like a C-assert: the function "expects",
but who is supposed to "ensure"? Only any other public function of that
class -- but this is already verified by checking postconditions. Somehow
checking invariants before function execution is checking the same thing
twice. If you find my reasoning even a bit convincing, perhaps it is worth
considering adding another config macro:
CONTRACT_CONFIG_NO_CLASS_INVARIANTS_ON_FUNCTION_ENTRY, that would disable
invariant checking before function or destructor call, but still check them
after function or constructor call?

(10)
It looks that when I include header <contract/class.hpp>, I also include
<boost/concept/requires.hpp> even if I do not intend to use concepts. Would
it be possible to avoid this somehow? (An additional header that also
allows me to use macros: like CONTRACT_FUNCTION, but does not include
<contract/class.hpp>, and if I use keyword "requires" the compilation
fails).

(11)
The library supports almost all of C++03's declaration syntax, but I
suppose it may not support some essential declarations from C++11, e.g.:
noexcept(condition) or trailing return types. I hope it is doable in the
future?

(12)
In the documentation (Introduction) we read that "This library is
implemented for the C++03 <http://en.wikipedia.org/wiki/C%2B%2B> standard
and it does not require C++11 <http://en.wikipedia.org/wiki/C%2B%2B11>." I
think it is a bit imprecise, because many examples in the docs silently
assume that variadic macros, which are not part of C++03, are available. I
believe something like this would be more accurate "This library is
designed to work on C++03 compilers that support variadic macros; no other
C++11 support is necessary. Additionally, if variadic macros are not
supported, this library works in >>strict C++03<< mode with slightly
different syntax."

(13)
The documentation says that you can use protected and private members in
contracts because "C++ provides programmers ways around access level
restrictions (e.g., friend and function pointers)". Next, we read that
"only public member functions shall check class invariants. Private and
protected member functions are allowed to brake class invariants because
private and protected member are part of the class implementation and not
of its specification". To me, these two statements appear incompatible:
either we say that only public members constitute class'es interface, or
even protected and private members constitute class interface, e.g. for
befriended functions. And either we both contract-check only public
functions and allow only public members in contracts, or we both
contract-check all functions and allow all members in contract. I prefer
the former. Or am I missing something? If there are some technical
obstacles in checking member access in assertions, then I accept such
library limitation, but the current argumentation in the docs appears
inconsistent.

(14)
The docs say that in order to make disabling of nested assertions
thread-safe in multi-threaded programs, the library needs to use a global
lock. Sorry if I am talking nonsense now (I have no intuition of using DbC
in multi-threaded apps), but is it not possible to use thread_local storage
for this purpose? Is it possible that executing an assertion in one thread
would cause the execution of another (nested) assertion in another thread?

(15)
Unfortunately, I will not have time to check it, so I will only ask you:
are any special considerations required in contract-checking classes that
inherit virtually (e.g. class hierarchies with diamond inheritance
structure). Order of object layout and construction is very particular in
those classes, perhaps the order of contract verification and
subcontracting should be special a well?

(16)
Functions like set_precondition_broken() are defined with dynamic exception
specification: throw(). Is this necessary? throw() is known to be causing
problems, adds little value, and some compilers do not support it; some
other issue warnings on dynamic exception specifications; and they are
deprecated.

(17)
According to docs, the framework checks invariants on destructor throw.
This is already tracked:
https://sourceforge.net/apps/trac/contractpp/ticket/66

(18)
"Duplicated globals" problem may cause failure to register custom contract
failure handler. This is already tracked:
https://sourceforge.net/apps/trac/contractpp/ticket/60

(19)
Documentation sometimes uses name "Contract Programming" as though it was a
framework. For instance, "This library implements Contract Programming" or
"Contract Programing checks them". I think it is incorrect. I understand
that "Contract Programming" is a philosophy or a paradigm, whereas what you
mean should be called "Contract Programming Framework" or similar.

(20)
In "Contract Programming Overview" -> "Benefits", in the note we read,
"However, the probability that programmers make a mistake twice (in both
the body *and* the contract) is lower than the probability that the mistake
is made just once (in either the body or the contract)". This implies that
we write contracts in order that the code be duplicated because this
reduces the probability of the bug. I believe that the mechanism of testing
in general is somewhat different (contract-checking is similar to unit
testing in this respect, I think): you want the tests to be much simpler
then the code they test. Therefore when some test fails you know (although
not for sure) that it is the code that is broken rather than the test. For
instance:

double sqrt(double v)
postcondition{ close(return * return = v); };

Testing almost the same thing as function body makes sense because
multiplication is conceptually much simpler than the logic to find the
square root. Conversely, writing this:

bool invert(bool b)
postcondition{ b == !return; };

Would be of little practical value, and I believe if there is a chance I
will implement such function incorrectly, duplicating the logic in the
precondition does not in practice reduce by half  the probability of
leaving the bug in the code.

(21)
Contract failure handlers will likely be customized inside function main().
This means that it will be difficult (although not impossible) to customize
these actions for contracts of constructors of global objects. This is a
very small problem and N1962 also seems to suffer from it and in fact even
C++ suffers from it, when it comes to installing terminate handler with
set_terminate.

(22)
Some random type-os in the docs. (apologies that I bluntly paste my notes
without editing them, but I am too tired to do it):
* " In addition, the library implements" -- don't mention it in the first
sentence.
* " and increasing overall software quality" -- no meaning
* "trailing column symbol :" -> "trailing colon symbol :"
* "First, the class invariants and the function preconditions are checked."
-> "First, the class invariants are checked, then the function
preconditions are checked. "(split it)
* "Differences with C++ Syntax" -> "differences
from"?<http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_0_4_1/doc/html/contract__/grammar.html#contract__.grammar.differences_with_c___syntax>
* "postconditions and class invariants cannot be weaken. " -> weakened
* "This library implement this feature however" -> "This library implements
this feature, however"
* "A *free function*
[8<http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_0_4_1/doc/html/contract__/contract_programming_overview.html#ftn.contract__.contract_programming_overview.free_function_calls.f0>
] call executes the following steps: " -- mention throw
* "Preconditions cannot access the object" -- "you shouldn't" or "it is not
possible"?
* "to ontract" in destructor tutorial

=== STANDARD REVIEW QUESTIONS ===

* What is your evaluation of the design?
* What is your evaluation of the implementation?

Unfortunately, the macro tricks are beyond my mental powers, so I am unable
to evaluate the design and implementation. I could only try to break it or
test if it behaves as documented on examples. I can see that the framework
is customizable (broken contract handlers, disabling the compilation of
handlers). One design flaw I can see is that it is a header-only library
while it requires some globals to store the handlers. But this is already
recorded as a confirmed bug:
http://sourceforge.net/apps/trac/contractpp/ticket/60

* What is your evaluation of the documentation?

Very, very good: easy intro for the beginners, very detailed, describes not
only the library but also the problem domain, lots of examples.

* What is your evaluation of the potential usefulness of the library?

It may be disqualified from production code by many parties due to the
unusual syntax. But it is definitely useful for learning DbC, experiments,
personal projects (where you need not convince others to use the interface
they do not like)

* Did you try to use the library? With what compiler? Did you have any
problems?
* How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I played with the library for three evenings, running basic tests and
trying to break it. I read the documentation. I also had some conversations
with Lorenzo as he was developing the library, and reviewed previous
versions of the docs. I am using VisualStudio 2010.

* Are you knowledgeable about the problem domain?

I know Contract Programming from Meyer's book and ISO C++ proposals (N1613,
N1669, N1773, N1866, N1962) and Lorenzo's documentation. I haven't used it
in practice on a dig scale.

=== FINALLY ===

The work Lorenzo did for this library is incredible. He already contributed
to the knowledge of the limits and the power of the preprocessor.
Supporting contracts in C++ without language extensions appeared (at least
to me) impossible. It is a great piece of work.

Regards,
&rzej