[reg] fix Issue 13113 - cannot build druntime's gc.d with -debug=INVARIANT, ba...

[WalterBright]

...d @nogc inference?

Attributes can be inferred for generated functions just like for function literals. This is more accurate than having the parallel method of mergeFuncAttrs(), which was failing in this case because it wasn't taking invariants into account.

https://issues.dlang.org/show_bug.cgi?id=13113

[9rnsr]

Running semantic3 on all generated functions to infer attributes would have bad affect for separate compilation model. I think that mergeFuncAttrs should be improved to handle the issue case.

Tomorrow I'll look at issue 13113.

[WalterBright]

@9rnsr I thought about that, and realized that generated functions are exactly analogous to function literals. Separate compilation won't affect the results.

Essentially what mergeFuncAttrs does is exactly what running semantic3() does, except that it is not kept in sync properly, and so is at high risk for generating different (and wrong) results.

At Andrei's urging, I have been pretty successful at reducing bugs in dmd by using 'lowering', i.e. rewriting higher level constructs into lower level ones, and then reusing the semantics of the lower level ones. That way, only the lower level semantics has to be correctly implemented. By 'lowering' the generated functions into function literals, we get them correct at minimal effort.

[9rnsr]

@WalterBright The root of 13113 is a language design issue, whether invariant block attributes should conflict with each member function attributes.

But today, pre & post virtual invariant calls in normal member function completely bypass attribute check. To fix the regression in 2.066 release, I think that direct invariant calls should also bypass attribute check. See #3775.

[WalterBright]

But today, pre & post virtual invariant calls in normal member function completely bypass attribute check.

True, but I believe that should be regarded as a bug. Propagating that bug is a problem.

Also, again, your fix does not address the issue of lowering as a technique to avoid semantic duplication and bugs. It continues to use two separate but unequal methods of attribute inference.

[WalterBright]

Continuing on, I think it would be very surprising to say "pure" functions are guaranteed to be pure, except if there's an invariant! I don't think that is good behavior at all. The same for other attributes.

[9rnsr]

Lowering itself is a good thing, but too early semantic3 running will cause disaster.
Because of that, I've repeatedly hit many link-failure issues around opEquals and TypeInfo. Don't repeat my mistake.

And, this PR will introduce wrong-code bug.

int count;

struct S
{
    invariant() { ++count; }
    Treap!int tr;
}

struct Treap(E)
{
    ~this() { }
}

void main()
{
    {
        S s;
        // implicitly generated dtor should call invariant.
    }
    assert(count == 1);     // fails!
}

[9rnsr]

it would be very surprising to say "pure" functions are guaranteed to be pure, except if there's an invariant!

It would cause a serious problem that -release switch will totally change the whole semantic result.

And, I think all contracts can be treated like a debug { ... } code. In other words, breaking purity in invariant code would be sometimes useful.

Node* root;

struct Node
{
    int val;
    Node* next;
    this(int n) { val = n; next = root; root = &this; }

    public int get() pure { return val; }

    invariant() /*cannot be pure*/ {
        for (auto n = root; n; n = n.next)
            if (n == &this) return;
        assert(0);  // this node must be reachable from 'root'.
    }
}

If impure invaiant violates the purity of public get function, it looks too restricted to me.

[rainers]

I agree with Kenji:

• debug {} already allows non-pure code in pure functions. Why should the invariant be any different. I'm not so sure about the other attributes, though.
• it seems pretty strong restriction to disallow e.g. nogc on any member just because the invariant concatenates two strings.
• forcing the attributes of the invariant on the members could also change the name mangling depending on building debug or release. So linking a debug build against a release library might cause unresolved symbols.

I'm not very comfortable with the situation, but the best solution to me seems to be: in debug code, all bets are off regarding pure, nogc and safe. I would not make an exception for invariants and contracts.

[yebblies]

The problem is (IMO) that invariants are not logically part of the call, but they are because of an implementation detail. This is the same with in/out contracts.

eg if the invariant was called externally

obj.__invariant();
obj.actualCall();
obj.__invariant();

Then the attributes on the invariant would only matter to the caller.

[WalterBright]

forcing the attributes of the invariant on the members could also change the name mangling depending on building debug or release. So linking a debug build against a release library might cause unresolved symbols.

Attribute checking doesn't work that way, it flows downwards on the call graph, not upwards. I.e. it is contravariant.

[rainers]

Attribute checking doesn't work that way, it flows downwards on the call graph, not upwards. I.e. it is contravariant.

Attribute inference for templates does work this way. With this PR I get different mangling for Treap's __ctor and Gcx's __fileddtor for debug/release depending on the contents of the invariant. It seems ignored for __dtor, though.

OT: the code for the invariant is emitted to the object file even when compiling with -release!?

[WalterBright]

@rainers I see what you mean, you're right. And the invariant is emitted so that other modules compiled with different flags can link against it.

[WalterBright]

Closed because I pulled #3775