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

Subject: Re: [boost] [Review:Contract] Andrzej's review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2012-08-30 13:56:33

On Wed, Aug 29, 2012 at 2:48 PM, Andrzej Krzemienski <akrzemi1_at_[hidden]> wrote:
> Hi,
> First, my vote is "YES": Contract should be accepted into Boost. Now for
> some details...

Thanks a lot for writing a review!

> === 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".

Indeed: http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_0_4_1/doc/html/contract__/contract_programming_overview.html#contract__.contract_programming_overview.features

> 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.

I personally agree, that's why I start the documentation with the quote:

“Our field needs more formality, but the profession has not realized it yet.”
--Meyer (see [Meyer97] page 400)

However, my impression is that the general C++ community is more on
the "profession" side ;)

> 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.

That's my hope:
http://contractpp.svn.sourceforge.net/viewvc/contractpp/releases/contractpp_0_4_1/doc/html/index.html#contract__.introduction.language_support

> (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;

True :(

> you cannot use some C++ constructs that you
> would otherwise use;

Like, for example? (Because I made a conscious effort to support "all"
C++ declaration features in the DSEL, if at all possible...) I know of
function pointers, arrays, member initializers in deferred
implementations but there are workaround for those. Anything else?

> 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.

I also share the same concerns: strange syntax, high compilation
times, bad compile-time errors (which would apply to other "crazy"
libraries like this (e.g., Boost.Generic) if they were ever to be
proposed for addition to Boost). I'm truly confident that the review
manager will be able to weight pros and cons correctly.

> === 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.

Indeed, the DSEL is a generic alternative syntax to declare classes
and functions using macros and it has the advantage that you can
inspect /any/ declaration trait at preprocessing time!! (This is
amazingly powerful if you truly think about it, it still amazes me...)
When you write to code expanding the macros, you can program things
like "if this declaration if for a member function and it's virtual
and it's protected and... then do this.. else do that...". Of course
you are not bound to use the DSEL to implement contracts, you could
use it for any other applications you can think of that need access to
class and/or function declaration traits (e.g., Boost.ConceptCheck's
requires and Boost.Parameter named parameters, and more).

> 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.

The CONTRACT macros expand to implement the contract checking code (so
CONTRACT is a good prefix). However, the framework they use, which
parses the DSEL, could be reused by others--that's in
contract/detail/preprocessor/traits and it could be made into a
separate pp library to declare classes and functions accessing all
their declaration traits at preprocessing time.

> === 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.

I will start a new thread to discuss this (given that also Vicente
asked about it).

> (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.

Meyer introduces loop variants as part of the assertion mechanism used
to check software correctness which he calls DbC(TM) (by the way, this
is a tread mark and usually people opt to use Contract Programming
instead). So I support loop variants in the library but you are right
that the contract metaphore doesn't really apply here.

> (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...

Indeed :)

> 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)?

Maybe so...

> (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.

:) I'm glad you raise this issue, I take it that the docs allowed you
to gain a very good understanding of Contract Programming. Here's what
N1613 says to your point:
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2004/n1613.pdf

``
7.2 Can we call the invariant less often?

It could also be argued that the invariant should only be checked as a function
exits. The invariant in the precondition would automatically hold because
of an earlier postcondition. The programmer could be malicious and
by-pass the mechanism by using public data, friends, function pointers and
templated member functions, but why protect against that? Thus we could
reduce the calls to the invariant to a half which improves debug performance.
(Remark: checking the invariant more often would detect memory
corruption errors earlier, but it is debatable whether this is any better since
the error would probably not be due to an error in the class where the invariant
failed.)
``

So yes, you are correct.

> 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?

I will do this, I entered a ticket for it:
https://sourceforge.net/apps/trac/contractpp/ticket/67

> (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).

The headers in contract/... are not independent :( I'll try to improve
that but the docs say to just include contract.hpp instead of the
single headers. Also when contracts are disabled some headers are not
needed so they should not be included to speed up compilation but I
include them anyway for now... These are things to fix with the
headers.

> (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?

Auto-functions can be supported like this:
https://sourceforge.net/apps/trac/contractpp/ticket/59

CONTRACT_FUNCTION(
    template( typename Left, typename Right )
    auto (adding_func) ( (Left const&) left, (Right const&) right )
        return(BOOST_TYPEOF(left + right))
) {
    return left + right;
}

noexcept should be supportable no problem (same C++11 syntax). I'll
support all C++11 declaration features in the future, if anything I'll
have to change the declaration syntax so the pp can deal with it as I
had to do with some C++03 declaration features.

> (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."

This is more correct but it's too verbose for the Introduction
section. Also variadic macros were part of C99, etc before C++11. I
might add a footnote to the statement that say "technically variadic
macros are not part of C++03, if your compiler doesn't support
variadic macros see No Variadic Macros section".

> (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".

This 2nd statement is correct. The 1st statement means that in C++ you
can never prevent people from programming junk like this which will
break the public interface (the same can be done without static):

#include <iostream>

class x
{
    public: static void pub ( void ) { std::cout << "pub" << std::endl; }
    private: static void priv ( void ) { std::cout << "priv" << std::endl; }

    typedef void (*fptr) ( void );
    public: static fptr bad ( void ) { return priv; }
};

int main ( void )
{
    x::pub();
    void (*priv) ( void ) = x::bad();
    priv(); // calls a private function :(
    return 0;
};

The call to priv via the function pointer is done outside the class
implementation but it won't check the invariants--but if you program
"bad" then you're on your own! That's all I wanted to say with the 1st
statement (which is actually an observation from The C++ Programming
Language, Stroustrup). I can clarify this in the docs.

> 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?

I'll think about it. Generally speaking, multi-threading needs to be
handled a level higher than contracts... [Meyer97] also explains how
concurrent programming might fail the contracts (and then he
introduces SCOOP as a solution but Eiffel didn't implement SCOOP until
later). I can provide more information on "contracts and concurrency"
(maybe in a separated email thread) if needed.

> (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?

No special consideration needed to my knowledge, but I will test this more:
https://sourceforge.net/apps/trac/contractpp/ticket/68

> (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.

It's the same for set_terminate that is throw() and it was specified by N1962:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n1962.html#failure-handler-functions

I think you don't want the handler setting function to throw given
that what you are setting is the handler itself which will affect how
you will handle a thrown exception (e.g., while evaluating a contract
assertion).

> (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.

Contract Programming is a methodology, I think. But if I can be a bit
loose without repeating "methodology" everywhere, the docs are easier
to read IMO.

> (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.

No, this is just about probabilities. Say p is the probability to
program a bug in the body and q the probability to program a bug in
the contracts for a given function. Le'ts assume independence of the
events for programming a bug in the implementation and programming a
bug in the contracts for that function. Then the probably r of
programming a bug in both the implementation and the contracts of that
function is given by the product r = p * q. Given that p < 1 and q < 1
then r < p and r < q. This doesn't assume you duplicate code, it's
just that if you do two things independently it's less likely you mess
both of them up :) See Section 3.6 of N1613:
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2004/n1613.pdf

> 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

No, contract programming would ask you to leave the b == !return
postcondition (Meyer is pretty clear about this). Of course you can do
whatever you want but when you write the contracts you don't know
about the implementation (e.g., a complex inversion algorithm that
does some optimized assembly bit operation could be used by the body
but the post still wants b == !return at the end of the day).

> 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

Noted.

> === 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.

Thanks :) And thank you very much also for all your comments and
inputs during the development of the library!

--Lorenzo