Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make invariants non-const by default. #1073

Closed
wants to merge 1 commit into from

8 participants

Alex Rønne Petersen JakobOvrum Daniel Murphy Hara Kenji Andrej Mitrović Andrei Alexandrescu Marco Leise Михаил Страшун
Alex Rønne Petersen

Poking this to make the auto tester run it on OS X machines.

JakobOvrum

This seems to have been left in the dust.

The implementation is a one line change. Surely it doesn't warrant a 3 month review period?

Daniel Murphy
Collaborator

@WalterBright made this change in the first place, in response to this bug.

Alex Rønne Petersen

@yebblies while not a pretty solution, I think the right thing to do is to ignore constancy when invoking invariants.

I think an invariant is a debugging tool and trying to enforce the type system in them has proven to be more annoying than anything else. In a project consisting of 30.000+ lines it literally has not caught any bugs at all, but has, on the other hand, forced me to use cast() in a lot of cases.

What are your thoughts?

Daniel Murphy
Collaborator

I'm not sure. There are also invariants for shared classes to consider.

JakobOvrum

@WalterBright made this change in the first place, in response to this bug.

It was a two-fold mistake. The first mistake was making a breaking change in the language without any form of consensus or review going on. The second mistake was of course that making invariants always const is actually not a good idea in the first place, which the discussion revealed (linked in the first post of this pull request).

Alex Rønne Petersen

ping

Hara Kenji
Collaborator

I still doubt that this is really necessary...

Alex Rønne Petersen

I don't think I can argue with anything but experience - see above:

In a project consisting of 30.000+ lines it literally has not caught any bugs at all, but has, on the other hand, forced me to use cast() in a lot of cases.

This misfeature is based on the assumption that const is actually applicable as logical constancy when it is in fact only bitwise constancy (as @JakobOvrum puts it).

(Also, it looks like this pull request fails on HEAD - will fix asap.)

Andrej Mitrović
Collaborator

I still doubt that this is really necessary...

They should be non-const by default, but we should allow marking them const if required. So we'll have:

class C
{
    invariant() { /* possibly mutate state */ }
    invariant() const { /* can't mutate state */ }
}

It seems like the simplest solution to satisfy both camps of ideology on what invariant should do.

Alex Rønne Petersen

@AndrejMitrovic agreed.

(By the way, fun fact: The latter declaration won't parse due to parser silliness wrt invariant still meaning immutable when compiling with deprecated features enabled...)

Andrej Mitrović
Collaborator

I've implemented the idea in the comment above as Pull 1560.

Hara Kenji
Collaborator

All contracts must not modify being contracted values. Inside invariant, compiler should guarantee that all member fields never be modified. In this case, "system programming language" cannot become the reason of breaking guarantee.

As evidence of it, we already cannot modify function parameters and returned value inside in and out contracts, .

int foo(int x)
in { x = 1; }  // cannot modify parameter 'x' in contract
out(r) { x = 1; r = 2; }  //  cannot modify parameter 'x' in contract
// cannot modify result 'r' in contract
body { return 10; }

Anyway, using invariant as like a hook of function call is wrong.

Alex Rønne Petersen

In this case, "system programming language" cannot become the reason of breaking guarantee.

Well I guess we're not going to agree here.

As evidence of it, we already cannot modify function parameters and returned value inside in and out contracts

Which was another breaking change with no warnings or documentation or practical rationale...

Alex Rønne Petersen

Closing in favor of @AndrejMitrovic's pull request.

Alex Rønne Petersen alexrp closed this
Hara Kenji
Collaborator

"Modifying object fields inside invariant" is just same as "Making side-effect in the condition expression of assert".

Consider: { auto x = 1; assert((x = 2) != 0); }

In this case, evaluating assert will change the value of x. We can agree that It is definitely a bug. (Note that assert expression will be removed in release build.)

As well, making side effect inside invariant is just a bug.

Hara Kenji
Collaborator

But, I do not deny that providing a way to make such a hole in program explicitly, just only for the debugging.

We already have a feature to breaking purity just for debugging.

int foo(int x) pure { debug { printf("x = %d\n", x); } return x * 2; }

So:

class C {
  int value;
  invariant() {
    value = 10; // disallowed
    debug { value = 10; }  // Making a hole explicitly. the programmer assumes all responsibility.
  }
}

might be acceptable to me.

Alex Rønne Petersen

You are wrongly assuming that I want invariants to be non-const for the sake of side-effects alone. The actual problem here is that making invariants const is simply impractical and ignores our language change process entirely:

  1. D's const cannot represent logical constancy. Like it or not, a lot of real world code depends on lazy initialization which const breaks down with.
  2. The standard library is far from const-friendly enough to enforce invariants to be const now.
  3. There was no discussion or consensus or documentation change or warning or anything else about this change.
  4. This change resulted in hundreds of compilation errors in one of my 30.000+ LoC projects at the whim of a compiler engineer. (Primarily because I hadn't been using const in said project because it's -- as said -- impractical right now.)

Points 1 and 2 are simply practical reasons for why making invariants const now is not reasonable. Points 3 and 4 are just not acceptable in the real world.

(Sorry if I sound aggressive, but I'm trying to drive home a very important point that we need to understand if anyone is going to adopt D for industry use...)

Hara Kenji
Collaborator

OK, I understand your opinion. So can you agree that "we should disallow to modify object fields inside invariant in the future"?

Alex Rønne Petersen

@9rnsr I don't know. I still think the gain is very questionable.

However, this is not something either of us should be deciding in this pull request. It should be discussed on the newsgroup and/or Bugzilla. For now, I'm just in favor of unbreaking the thousands of lines of code that have been broken.

JakobOvrum

@9rnsr, as far as I know, it is not a goal, present or future, for D's const to cover logical constancy. Invariants should not be forced const because bitwise constancy is too strict.

I think we all agree that invariants shouldn't have side effects, but logical constancy should be the measure for side effects unless bitwise constancy has been explicitly requested with const, otherwise invariant is needlessly strict and thus cannot be used in a large number of practical situations in real projects. This means that non-const invariants cannot be guaranteed correct by the compiler, but I think this is a good tradeoff.

The other factors mentioned in the discussion are important too, but only in the short term. For my part, the above argument is what matters in the long term.

Andrei Alexandrescu
Owner

I have a simple and pragmatic view on this matter. invariant is meant as a tool for implementers to ensure good use of an abstraction by the client. Protecting the implementers against themselves seems inappropriate in this case. A bug in invariant is as bad as an electrician getting electrocuted.

Marco Leise

@alexrp If your code isn't marked const at all that is entirely your fault. Don't blame the semantics of invariant(), which I would have supported if I was involved. You are right on the other hand, that "const" is not to be confused with "locigal const". And that's where the logically sound restrictions fall to pieces: Your object with the invariant may have an aggregated object that uses lazy initialization or some system resource for which it is difficult to make a decision on constness.

@andralex Tools for professionals aren't designed without any safety measures. That said, If you want to climb that mountain without a rope, you should be able to do so. So I would support safe-by-default (e.g. const) and !const/mutable/const(false) for the case where you know why you escape it. Mainly because I find it reassuring to see my reasoning ("hey, invariants should not change the data they check, right?") reflected in the language ("ah yes, I cannot change that field here. so my assumption that invariants should not modify state, was correct.").

Btw. for the same reason I don't like how "static" can be used on globals and meaningless modifier combinations are possible. They make it harder for someone learning the language to draw conclusions about the intended use. Just my 2¢.

Alex Rønne Petersen

If your code isn't marked const at all that is entirely your fault. Don't blame the semantics of invariant(), which I would have supported if I was involved.

This claim might seem more plausible if you sit in an ivory tower where everybody writes perfect code and const is applied properly to every single function where it's relevant.

(The aforementioned perfect code is not Phobos.)

Михаил Страшун

Logical const is a myth in D and should not even be attempted. Allowing it would have create a completely different language.

Daniel Murphy
Collaborator

Logical const is a myth in D and should not even be attempted. Allowing it would have create a completely different language.

Logical const works perfectly well in D, it's just not enforced by the language. Most of the language can be used without touching const at all, and invariants should be the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 19, 2012
  1. Alex Rønne Petersen
This page is out of date. Refresh to see the latest.
1  src/func.c
View
@@ -3910,7 +3910,6 @@ void InvariantDeclaration::semantic(Scope *sc)
sc = sc->push();
sc->stc &= ~STCstatic; // not a static invariant
- sc->stc |= STCconst; // invariant() is always const
sc->incontract++;
sc->linkage = LINKd;
5 test/compilable/invariant_mut.d
View
@@ -0,0 +1,5 @@
+class A
+{
+ int x;
+ invariant() { x = 42; }
+}
2  test/fail_compilation/fail7369.d
View
@@ -1,5 +1,5 @@
struct S7369 {
int a;
- invariant() { a += 5; }
+ const invariant() { a += 5; }
}
Something went wrong with that request. Please try again.