New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore unsafe and impure code in any contract and catch all Exceptions (fixes issue 4995 and regression 10981) #2155

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@yebblies

This comment has been minimized.

Member

yebblies commented Jun 10, 2013

Or should we enforce that the invariant is nothrow, when any of the public member functions are? Are the other attributes enforced for invariant calls? (@safe/pure)

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jun 10, 2013

Or should we enforce that the invariant is nothrow, when any of the public member functions are?

This fix takes Jonathan's second road. And I think it is the right one.

Are the other attributes enforced for invariant calls? (@safe/pure)

Right now, the invariant calls are implicit, see AssertExp::toElem, which is in the glue layer. This fix calls the invariant directly using CallExp. So the other attributes are enforced. This is why the test is failing.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jun 10, 2013

I don't know what to do now, though. Fix all invariant signatures in Phobos?

@9rnsr

This comment has been minimized.

Member

9rnsr commented Jun 10, 2013

Let's see the enumerated ways.

  1. Nothing to do
    An invariant throwing exception will break nothrow-ness of member functions. Definitely bad.
  2. Eat all thrown Exceptions from invariant body, and throw AssertError instead. (same as current this pull request way)
    A little bad, because the thrown exception will be lost implicitly.
    And, this semantics implicitly changes invariant blcok to nothow, even if it is not marked as nothrow.
    Currently we can mark invariant nothrow. In other words, invariant is throwable on default.
    So it looks to me a general rule violation that a function may throw Exception on default if it is not annotated with nothrow.
  3. Make a compile time error for the combination of nothrow member function and throwable invariant.
    If a nothrow-ness violation occurs, compiler just reports it as error.
    Against the error, programmer can choose methods, fixing invariant code to nothrow, or manually eating thrown Exceptions by try-catch, or inserting scope(failure) at the beginning of the invariant.
  4. Enforce nothrow-ness for invariant block. (suggested by @yebblies)
    This is essentially not bad, but may be too strong enforcement for system programming.
    As a side talk, currently invariant block is marked const on default. I'm taking stance keeping it as-is, but it's still debatable issue.
    If we take this semantics, we should also apply this to in and out contract blocks for the consistency.
    And, this semantics has an issue for @safe attribute - we cannot enforce @safe for invariant generally.
    (If a class does system-related working, we cannot define invariant?)

I think 3. is better than others, because it gives the control opportunity for the exception handling to the programmer.

@andralex

This comment has been minimized.

Member

andralex commented Jun 10, 2013

I like 3 too. As a note, regarding (2) we can translate any exception into an assertion error without loss of information by setting the original exception as chained from the assertion error.

@yebblies

This comment has been minimized.

Member

yebblies commented Jun 10, 2013

Actually I'm in favor of 3, despite my phrasing.

@9rnsr

This comment has been minimized.

Member

9rnsr commented Jun 10, 2013

Thanks, @andralex and @yebblies. It might break existing user code, but we can help fixing the appeared bugs by providing more strict @safe/pure/nothrow Phobos. I'm sure that it is possible, from the experience of my recent Phobos contribution and related compiler bug fixes.

@hpohl, Would you have any opinion about that?

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jun 10, 2013

I don't like the idea of enforcing nothrowness of the invariant depending on the existance of a nothrow method. A little unrelated change (adding a nothrow method) may break all invariant code.
Invariants ensure the correctness of the program, if they fail, there should be an Error, not an Exception. So in my oppinion every call to an invariant is nothrow. Thus, it needs to collect all thrown Exceptions and throw an Error. And as Andrei said, there isn't any information loss. There should be no way to mark an invariant as nothrow, because it is redunant.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jun 10, 2013

And this should not depend on the nothrowness of the function where the invariant is called in.. I will change that.
If you really want to go with 3, I can implement that, though.

@Infiltrator

This comment has been minimized.

Infiltrator commented Jun 10, 2013

@hpohl raises a very good point about invariants throwing Errors and not Exceptions. It's part of contracts, and so, like in and out, should only assert to throw Errors.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jun 13, 2013

It's a bad idea to let any Exception escape a contract, because it may be catched by an error handler that is not (it cannot be) intended to catch logic errors. We need to fix in and out contracts as well. What do you think?

@9rnsr

View changes

src/func.c Outdated
@@ -4459,6 +4436,9 @@ void InvariantDeclaration::semantic(Scope *sc)
if (!type)
type = new TypeFunction(NULL, Type::tvoid, FALSE, LINKd, storage_class);

if ((storage_class & STCnothrow) || (sc->stc & STCnothrow))
::error(loc, "invariants are implicitly nothrow");

This comment has been minimized.

@9rnsr

9rnsr Jun 13, 2013

Member

This is worse than before. It will break existing code, such as:

class C {
nothrow:
    /* many methods */

    invariant() { ... }  // impliitly annotated with nothrow
}

This comment has been minimized.

@hpohl

hpohl Jun 13, 2013

Contributor

The parser doesn't distinguish nothrow: invariant() { } and nothrow invariant() { }. How to resolve that? And even if it does break code, it is easy to fix.

This comment has been minimized.

@9rnsr

9rnsr Jun 13, 2013

Member

It's current design. So there is no solution.
Additionally, even if it is possible, breaking existing nothrow invariant {} is also bad idea.

This comment has been minimized.

@hpohl

hpohl Jun 13, 2013

Contributor

So you think marking an invariant with nothrow even though no Exception will ever escape is okay? It still forces the invariant body to not throw, though.

This comment has been minimized.

@9rnsr

9rnsr Jun 13, 2013

Member

I think nothrow invariant should be enforced only when it is necessary.

class C1 { void foo(){}  invariant(){} }  // nothrow invariant is not necessary
class C2 { void foo1(){}  void foo2(){}  invariant(){} }  // same

class C3 { void foo1(){}  nothrow void foo2(){}  invariant(){} }  // invariant should be nothrow

Compiler should raise an error only for C3.

This comment has been minimized.

@hpohl

hpohl Jun 13, 2013

Contributor

Why do you want Exceptions to be able to escape from contracts?

This comment has been minimized.

@hpohl

hpohl Jun 13, 2013

Contributor

Anyway, you're right, the check is bad, I will remove it.

This comment has been minimized.

@9rnsr

9rnsr Jun 13, 2013

Member

Because normal function can throw Exception on default.
Some D users will surely imagine an invariant as a special function, that is implicitly called at the start and the end of public member functions. For them, implicit nothrow enforcement for invariant may be unacceptable behavior.

I don't say that current this change is essentially wrong, but I'm much worried that a built-in language semantics will reject system programming, especially if it requires runtime overhead. We should become very very carefully to add such runtime behavior.

This comment has been minimized.

@hpohl

hpohl Jun 13, 2013

Contributor

Some D users will surely imagine an invariant as a special function, that is implicitly called at the start and
the end of public member functions. For them, implicit nothrow enforcement for invariant may be unacceptable behavior.

That usage of invariants is unacceptable. Maybe they make use of a library which uses contract programming the right way and recieve a giant loss of performance caused by unneeded checks in the library. D has other devices to achieve the same "invariant" effect.

I don't say that current this change is essentially wrong, but I'm much worried that a built-in language semantics will reject system programming, especially if it requires runtime overhead. We should become very very carefully to add such runtime behavior.

There is no additional runtime overhead in release builds. Can you outline an example where these semantics reject system programming?

This comment has been minimized.

@hpohl

hpohl Jun 13, 2013

Contributor

For them, implicit nothrow enforcement for invariant may be unacceptable behavior.

I'm not going to enforce nothrow invariants, because all Exceptions get caught. You do so.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jul 23, 2013

Okay, let's bring this back to life. Given that catching bugs is the only valid use of invariants, I think that:


Exceptions

All Exceptions thrown by an invariant should be catched and an Error (containing the Exception) should be thrown.

Reasons:
  • Program bugs catched by invariants may be silently eaten or can even change the program behavior depending on compilation mode:
class Address {
    private bool valid() { /* ... */ }

    string value() { /* ... */ }

    invariant() {
        // 'valid' may throw an exception.
        assert(valid);
    }
}

void foo(Address a) {
    // ...
    auto v = a.value;
    // ...
}

void bar(Address b) {
    // ...
    try {
        b.foo;
    } catch (Exception) {
        // Silently eat the exception.
    }
}
  • There is no reason to let any Exception escape a contract.
  • Functions which validate input and throw Exceptions can now be used in contracts without catching the Exceptions and rethrowing them as an Error.
/**
 * Validates a string.
 * Throws: Exception if the string is invalid.
 */
 void validate(string s) { /* ... */ }

 struct S {
    string val;

    invariant() {
        val.validate;
    }
 }
  • It reduces the possibility to use invariants incorrectly.

Purity

Purity should be ignored when calling invariants.

Reasons:
  • Contracts (invariants in this case) are not part of the function implementation.
  • The purity of the function call should not depend on the way the implementer manages the inner correctness of the aggregate.
  • The debug statement is already doing this and has the same task as contracts.

Safety

I'm not sure about this but I think the rules of Purity apply here, too.


So you are able to call a throwing, impure and @system invariant from a nothrow, pure and @safe method. This will break no code and will cause an easy-to-fix program behavior change.

@Infiltrator

This comment has been minimized.

Infiltrator commented Jul 24, 2013

I agree completely. invariants are really only for debugging and not part of the "real" run of the code, and so should not be restricted by nothrow, pure, and @safe even when the functions being debugged are restricted by those.

Having said that, in, out, and debug also should not be restricted by these; but I suppose that that is probably a discussion for another place.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jul 27, 2013

And again I'd like to hear what @andralex, @WalterBright and @9rnsr think about this so I can get something implemented.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Aug 1, 2013

And again I'd like to hear what @andralex, @WalterBright and @9rnsr think about this so I can get something implemented.

I really do.

@hpohl

This comment has been minimized.

Contributor

hpohl commented Sep 6, 2013

@WalterBright

This comment has been minimized.

Member

WalterBright commented Jun 1, 2014

This PR has a number of possible approaches. The difficulty is there are a number of attributes:

nothrow, @safe, pure, @nogc, const, immutable, shared

that may apply to invariants. Currently, const is applied. The PR applies pure (but does not check it) and @trusted, and adds code to silently convert thrown Exceptions to Errors inside of nothrow functions.

I'm uneasy with this. We also have backwards compatibility to be concerned about.

  1. I can't think of a legitimate case where invariant can throw an Exception. Calling invariants or not should not be altering the normal behavior of a program. Hence, invariant should be implicitly nothrow.
  2. Removal of checks for @System and purity is a hidden escape from the static guarantees of the language - seems wrong to me.
  3. I think we can require the existence of a function body for invariants, and then do attribute inference on it.
@andralex

This comment has been minimized.

Member

andralex commented Jan 27, 2015

this has kinda bitrotten... please rebase?

@hpohl

This comment has been minimized.

Contributor

hpohl commented Jan 30, 2015

Don't have the time to rebase all that stuff. Feel free to close. May come back at some point.

@wilzbach

This comment has been minimized.

Member

wilzbach commented Mar 15, 2018

As this is still based on the C++ frontend, it might take a serious effort to revive it.
Though there's some automation - see #4922 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment