www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 6278] New: 'in' contract inheritance doesn't work with safe code

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278

           Summary: 'in' contract inheritance doesn't work with safe code
           Product: D
           Version: D2
          Platform: Other
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: michel.fortin michelf.com


--- Comment #0 from Michel Fortin <michel.fortin michelf.com> 2011-07-09
22:00:30 EDT ---
This code yields an error about catching Throwable in safe code, yet no
throwable are caught (except maybe in compiler-generated code related to
contracts):

 safe:

class A {
    int test()
    in { assert(0); }
    body { return 1; }
}
class B : A {
    override int test() // Error: can only catch class objects derived from
Exception in  safe code, not 'object.Throwable'
    in { assert(0); }
    body { return 1; }
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 09 2011
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278


yebblies <yebblies gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |rejects-valid
                 CC|                            |yebblies gmail.com
            Summary|'in' contract inheritance   |Regression(2.054 beta):
                   |doesn't work with safe code |'in' contract inheritance
                   |                            |doesn't work with safe code
           Severity|normal                      |regression


--- Comment #1 from yebblies <yebblies gmail.com> 2011-07-10 15:43:47 EST ---
Yep, the compiler generates a bunch of nested try {} catch {} blocks in the
function preconditions.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 09 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #2 from yebblies <yebblies gmail.com> 2011-07-10 15:53:01 EST ---
(In reply to comment #1)

 Yep, the compiler generates a bunch of nested try {} catch {} blocks in the
 function preconditions.


I think the correct change here is to introduce a new exception type
(ContractException?) and hook the assert handler to throw this when inside
contracts.  Currently the compiler will take OutOfMemoryError as a contract
failure and happily continue.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 09 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #3 from Michel Fortin <michel.fortin michelf.com> 2011-07-10
06:28:53 EDT ---
A simple fix for this would be to add a flag for compiler-generated catch
blocks that'd allow bypassing  safe checks when appropriate. I wonder how it
works for scope(failure)...

And yes OutOfMemoryError shouldn't be caught by contracts. Adding a new
exception type would help, but I think ContractException should be
ContractError instead and be a subclass of Error.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 10 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #4 from yebblies <yebblies gmail.com> 2011-07-10 20:47:00 EST ---
(In reply to comment #3)

 A simple fix for this would be to add a flag for compiler-generated catch
 blocks that'd allow bypassing  safe checks when appropriate. I wonder how it
 works for scope(failure)...
 


Simple, yes.  But is it correct?  The whole idea is that Errors can leave the
program in an invalid state, so continuing after one in  safe code is not
allowed.  If there's no chance of leaving the program in an invalid state, why
is it an error in the first place?


 And yes OutOfMemoryError shouldn't be caught by contracts. Adding a new
 exception type would help, but I think ContractException should be
 ContractError instead and be a subclass of Error.


Why would it be an Error?  An in contract failure is allowed, it just means
that a different in contract in the hierarchy should be tried instead.  The
meaning of assert inside in contracts is very different from asserts everywhere
else, where they mean they program has reached a state that shouldn't be
possible.  I think out contracts should throw normal AssertErrors though.

I'm starting to think assertions inside in contracts (for virtual functions
only) should become special contractAssert()s throwing ContractExceptions.

Regardless of the correct fix, is this a blocker for you?  It might be worth
jamming in a workaround before the release is done if it is.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 10 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #5 from Michel Fortin <michel.fortin michelf.com> 2011-07-10
07:02:27 EDT ---

 Simple, yes.  But is it correct?


It's a fix for the issue at hand. It doesn't fix other issues related to
contracts, but it doesn't degrade things either. If you want to fix all the
issues in one step though, I'm not stopping you.


 Why would it be an Error?


Because in general contract violations are errors. There's indeed a special
case for contracts of overriding functions, but does that justify creating a
new error type just for that case? I think it's more consistent if all
contracts throw ContractErrors than if some contracts throw ContractExceptions
and some other throw AssertErrors.


 Regardless of the correct fix, is this a blocker for you?


It's not a blocker: I made the problematic function  trusted for now.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 10 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #6 from yebblies <yebblies gmail.com> 2011-07-10 21:48:16 EST ---

 Because in general contract violations are errors. There's indeed a special
 case for contracts of overriding functions, but does that justify creating a
 new error type just for that case? I think it's more consistent if all
 contracts throw ContractErrors than if some contracts throw ContractExceptions
 and some other throw AssertErrors.


Only the overridden contracts would throw ContractExceptions, the only way you
would ever see them was if you did:
class A {
  void fun() in { try { assert(0); } catch (ContractException) {} } body {}
}
class B {
  void fun() in {} body {}
}

Which is just horrible.

If we disallow scope(exit/failure) and try/catch inside in contracts assert(x)
could be re-written to if (!x) goto contractfail; which bypasses exceptions
altogether.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 10 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #7 from Michel Fortin <michel.fortin michelf.com> 2011-07-10
08:21:02 EDT ---

 Only the overridden contracts would throw ContractExceptions, the only way you
 would ever see them was if you did:
 [...]
 Which is just horrible.


But it would be legal, even in safe code. And it'd be inconsistent since it'd
work only in overriding contracts and not elsewhere, exposing an implementation
detail.

(And it has nothing to do with this bug. I think you should fill a bug about
other errors being caught by contracts and discuss all this there.)


 If we disallow scope(exit/failure) and try/catch inside in contracts assert(x)
 could be re-written to if (!x) goto contractfail; which bypasses exceptions
 altogether.


Wouldn't that lead to other problems? If you had a struct with a destructor as
a local variable inside your contract, that destructor will be called in an
implicit finally block, much like scope(exit), so disabling try/finally will
break that.

Beside, what do we gain by disabling this? I agree it'd a poor coding practice
to catch exceptions in contracts, but I don't think the language should try to
enforce that. It might even be useful if for some reason you need to debug your
contract.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 10 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com


--- Comment #8 from Walter Bright <bugzilla digitalmars.com> 2012-02-01
00:03:28 PST ---
The concept here is that, if a base function and its override both have an IN
contract, then only one of them needs to succeed. This is done by generating:

  void derived.in()
  {
    try
    {
      base.in();
    }
    catch ()
    {
      ... body of derived.in() ...
    }
  }

So if base.in() doesn't throw, derived.in() need not be executed, and the
contract is valid. If base.in() throws, then derived.in()'s body is executed.

As you can see, it swallows any exception generated by the contract. Whether
this is correct or not is certainly debatable.

I think a reasonable solution which will target this particular thing would be
to mark that try/catch as being valid even in safe mode. That'll change it from
a regression to an enhancement request to figure out a better way, as at least
it'll work as documented.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 01 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278


yebblies <yebblies gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Platform|Other                       |All


--- Comment #9 from yebblies <yebblies gmail.com> 2012-02-01 19:18:47 EST ---
The problem with just letting it catch all throwables in safe code is that it
is _not_ safe, so until we have a better solution I think forcing the user to
mark their function as  trusted is actually correct.

Exceptions thrown in contracts are supposed to be recoverable, which makes
AssertError (or any error) inappropriate.  But if asserts inside contracts are
changed to throw something else, called functions may still throw AssertErrors
that were intended to be caught.

On the other hand, an assert hit inside a contract could easily mean the
program is in an invalid state, and completely break the guarantees of  safe.

I think the best option is to make asserts in contracts throw something else,
despite the downsides.

I understand you would like to undo the regression, but I think re-opening this
hole in  safe is much worse than the current state, especially considering how
trivial the workaround is.

If you know how you would like the solution to look I'm happy to implement it.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 01 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #10 from Walter Bright <bugzilla digitalmars.com> 2012-02-01
00:46:33 PST ---
(In reply to comment #9)

 Exceptions thrown in contracts are supposed to be recoverable,


No, they are supposed to exit the program. The thing with in contracts is not
that they're recoverable, it's that they assume that a failing in contract is
not leaving the program in an invalid state.

I agree this is a hole in the safety system, which is why I'm not closing it.
I'm just getting it to not be a regression. I do not know what a completely
correct fix would look like at the moment.

One possibility would be to allow only pure code in a contract. Pure code
cannot modify global state, and so when it throws it wouldn't be leaving
something outside of the contract in an invalid state.

We've talked about making in/out contracts pure before, I've been concerned
that they'd be too restrictive. Maybe require them to be pure when in safe
mode?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 01 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278



--- Comment #11 from github-bugzilla puremagic.com 2012-02-01 00:54:26 PST ---
Commit pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/dc7916071b60739dcc8c09e43ff3da5218b42ff2
partially fix Issue 6278 - Regression(2.054 beta): 'in' contract inheritance
doesn't work with safe code

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 01 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6278


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|regression                  |normal


--- Comment #12 from Walter Bright <bugzilla digitalmars.com> 2012-02-01
00:56:01 PST ---
The bug is now that the in contracts can swallow any Errors and leave the
program in an invalid state.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 01 2012