Untangling setUnsafe()

[WalterBright]

The setUnsafe() function is a horrible mess with multiple overloads and undocumented dependencies on various argument compinations. I'm going to try an untangle it.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#17072"

[WalterBright]

This installment separates out the special case of function into a separate function, with a separate name instead of an overload.

[dkorpel]

I understand the renaming, but I don't think it's a good idea to duplicate this function with partially evaluated arguments: it only adds new confusing overloading, and every call to this function is basically a diagnostic bug and should be replaced with a call to the first overload of setFunctionToUnsafe, which explains to the user why a function is being marked unsafe.

[WalterBright]

The problems are:

1. the multiple different overloadings of setUnsafe
2. the tangle of different behaviors taken based on whether fmt is null or arg is null or both are null
3. fhe absurdly complicated forwarding of the optional arguments

I've spent far too much time trying to trace through the logic of this function and its partner errorSupplementalInferredAttr()

My plan is to replace this rube goldberg machine with proper use of va_list. Problem (2) should split the function into 3 distinct functions. This particular PR factors out the null null case.

It's my fault that va_list wasn't used in the original incarnation of this, and I aim to fix it. The first two steps are done - this one, and improving ErrorSink to support va_list.

[dkorpel]

This particular PR factors out the null null case.

The null null case is a diagnostic bug. Basically all setUnsafe calls without error message were being replaced with ones with error messages to fix Issue 17374 (See also: Spelunking Attribute Inference in D), but there were a few tricky ones remaining (especially the infamous inference of recursive functions problem), so I haven't gotten rid of the default argument for the format string yet.

It's my fault that va_list wasn't used in the original incarnation of this

Actually, that would be me 😮

The reason optional arguments are passed as object parameters is because they are being stored in a AttributeViolation inside a FuncDeclaration. This is the ddoc comment of AttributeViolation in dmd/func.d:

Stores a reason why a function failed to infer a function attribute like @safe or pure

Has two modes:

• a regular safety error, stored in (fmtStr, arg0, arg1)
• a call to a function without the attribute, which is a special case, because in that case,
  that function might recursively also have a AttributeViolation. This way, in case
  of a big call stack, the error can go down all the way to the root cause.
  The FunctionDeclaration is then stored in arg0 and fmtStr must be null.

I agree this is a mess, but it was done for performance reasons: eagerly generating attribute error strings for each attribute for each function could slow down compilation when complex templates / expressions are involved, so AttributeViolation was an attempt to lazily store an error message.

I considered using delegates instead, but that would likely create closures around every call site of setUnsafe, which may be undesirable.

There's other places in dmd where error messages are stored / passed around before being printed (grep pMessage or errorHelper for examples), so a general solution would be really great, I hope you can help out here! But I don't thing va_list is going to work here though, because you can't store it in a struct, right?

I suggest sorting out what to do with AttributeViolation fields in FuncDeclaration first, because that is the bottleneck that the clumsy setUnsafe (and also setImpure etc.) calls are built around.

[WalterBright]

The solution to the arg ? toChars(arg) : null inefficiency is to split the function into two:

1. determine if there is an error
2. if so, then produce the error message

AttributeViolation can use OutBuffer.printf to produce a string, as you are correct in that the va_list cannot be saved.

[dkorpel]

The inefficiency has nothing to do with arg ? toChars(arg) : null. That piece of code is only there because different error messages have a different number of %s arguments.

The problem is that at the point of calling setUnsafe, it isn't yet clear whether there is going to be an error. The error might appear later when a @safe function is trying to call a function that was inferred @system.

AttributeViolation can use OutBuffer.printf to produce a string

You mean on creation? That would indeed simplify it a lot, but that means many error strings are being generated even when there are no errors. I don't mind, it probably won't be a significant performance hit, I just want to make sure we're on the same page here and aware of the consequences.