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

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

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

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

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

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

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

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

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

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