fix Issue 23445 - Can leak scope variable through delegate context

[WalterBright]

Since this incorporates https://github.com/dlang/dmd/pull/14601, that should be pulled first and so it blocks this PR.

Since the type of a delegate loses information about what outer variables may be returned, the most pragmatic solution is to disallow creating delegates that return outer scoped variables.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23438	normal	leaking address of stack using dip1000 switch
✓	23445	normal	Can leak scope variable through delegate context

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14610"

[WalterBright]

This fails because of:

struct S
{
    char* p;

    char* parseType( ) return scope pure @safe
    {
        char* parseBackrefType(scope char* delegate() pure @safe parseDg) pure @safe
        {
            char* foo() { return parseType(); }
            auto dg = &foo;    // cannot make delegate from `foo` because it returns `this`
            return parseBackrefType( dg );
        }
        return p;
    }
}

Not sure if this is fixable to keep this all @safe instead of having to go with @trusted

[tgehr]

This fails because of:

struct S
{
    char* p;

    char* parseType( ) return scope pure @safe
    {
        char* parseBackrefType(scope char* delegate() pure @safe parseDg) pure @safe
        {
            char* foo() { return parseType(); }
            auto dg = &foo;    // cannot make delegate from `foo` because it returns `this`
            return parseBackrefType( dg );
        }
        return p;
    }
}

Not sure if this is fixable to keep this all @safe instead of having to go with @trusted

Involved lifetimes:

• lifetime of this
• lifetimes of the delegates

The delegates are not returned, so they can be scope. The captured variables have the lifetime of this, so they can be return scope.

This specific example can be tracked with the general approach that DIP1000 has been taking because the lifetime of this is the only lifetime that matters. The way to fix this is to require/infer return scope on the local functions and the delegate pointers.

This is the right way to do this kind of thing. However, note that with DIP1000, this set up is a bit magic in general, because delegates are the only type that is allowed to specify return scope on "internal" data. In particular, it is not possible to create a simple wrapper type for delegates such that this code is still allowed if we use the wrapper instead of the delegate directly.

[WalterBright]

The fundamental problem with this particular issue is that, when a delegate is created from a nested function, the information about what outer variables are returned is lost. This is because the delegate type has no place for that information. The fix here is to not allow the conversion to delegate if there are returns of outer variables while there are parameters marked as "return scope".

This prevents the problem of the delegate signature saying one lifetime while the return of an outer means another lifetime.

While it's a conservative fix, it doesn't break anything in the test suite, so it looks reasonable.

[tgehr]

It's a conservative fix and it's also ad-hoc. It does not follow from return scope/scope semantics on the pointer level. It's both more powerful because you are tracking which variables are returned from which local functions and it is less powerful because you are disallowing code with closures that DIP1000 would in principle be able to allow on the pointer level.

Note that this kind of ad-hoc fix adds complexity and baggage to the language. No D programmer will know how the lifetime tracking actually works. No DMD developer will know what the correct behavior is supposed to be.

Nominally trying to avoid "complexity", the lifetime tracking increasingly becomes a tangled buggy mess of special cases.

This may be fine as a stop-gap solution, but it's not really more than that.

[dkorpel]

I agree.

I think a delegate created from a nested function should be treated the same as a delegate from a struct member function. A stack frame is basically just a struct, and escaping an outer variable is equal to escaping a struct member through the this reference. In both cases, it should infer return or strip away scope from the function type.

This would be less powerful than keeping track of individual local variables, but it's consistent with how all struct members are assumed to have equal lifetime.

[tgehr]

This would be less powerful than keeping track of individual local variables, but it's consistent with how all struct members are assumed to have equal lifetime.

To be very clear, I think that restriction is very serious and bad, but if it is lifted at all it should be done in a principled and consistent way.

[WalterBright]

You're right that a delegate from a nested function, struct member function, and class member function are all the same. So there shouldn't be any trouble here, right?

The compiler does not support a scope pointer to a scope pointer. Struct and class fields cannot be marked scope, so this is not a problem for them. But the "fields" of a nested function can be marked scope. This works fine for nested functions, which have information about where all the uplevel variables are. But when a nested function is converted to a delegate, that information gets lost. Hence the unnoticed escaping going on.

This PR addresses that problem conservatively, by disallowing conversion of a nested function to a delegate, if that nested function returns any scoped fields.

Yes, it would be better if struct fields could be tagged as scope. I don't know how much of a change that would be internal to the compiler, but doubt it would be easy. In the meantime, this PR works and doesn't appear to be disruptive.