[clang] Structure binding (destructuring assignment) does not generate "unused variable" warning when one variable remains unused

[alexandrustinov]

We need a function which returns structure Result with some value and error code, kind of Go language style error handling (see https://go.dev/doc/tutorial/handle-errors for reference).
The result need to be handled using destructuring assignment, C++17 feature (see https://en.cppreference.com/w/cpp/language/structured_binding)

Actual behavior: compiler does not generate warning if error code is not handled, it generates warning when both value and error variable are unused only, considering them as "single variable" [value, error], while those are supposed to be handled as separate variables.

Expected behavior: Compiler should generate warning when either value or error variable is unused. For our case the calling function must handle the result error code before processing the result value. If the error handling is omitted then compiler should notify about that.

#include <stdio.h>

struct Result {
    int value;
    int error;
};

Result func(int arg) {
    if (arg) {
        return {arg, 0};
    }
    else {
        return {0, -1}; // -1 means faulure here
    }
}

int main() {
    auto [value, error] = func(0); // simulating faulure

//    if (!error) // we forgot to handle error code, compiler should generate warning about unused variable, but it does not
        printf("value = %d", value);

}

// clang++ --std=c++17 -Wunused-variable unused_destructure.cpp

[AaronBallman]

This behavior is on purpose: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L1973 which was introduced by 81df9eb.

However, I'm changing the label to enhancement rather than closing, because WG21 has reopened discussion of https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0609r1.pdf and may accept it for C++2y at which point we could potentially diagnose unused structured binding decls.

[alexandrustinov]

Thank you for the code point, it will be good start for custom patch.

This behavior is on purpose: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L1973 which was introduced by 81df9eb.

However, I'm changing the label to enhancement rather than closing, because WG21 has reopened discussion of https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0609r1.pdf and may accept it for C++2y at which point we could potentially diagnose unused structured binding decls.

I'm sorry, could you point/clarify root of the purpose for such behavior?

If developer does not want "unused" warning for such particular variable - it always would be better to either suppress it by [[unused]] attribute or by old good

 #define UNUSED(variable) (void) variable; 

But it is still absolutely not clear why next declaration should produce warnings:

int a = myfunc().a;
int b = myfunc().b; 
printf("%d", a);

while practically the same - should not warn about unused variable b

auto [a, b] = myfunc(); 
printf("%d", a);

Why such implicit warning suppression could be useful? For which tasks, use cases, etc?

[AaronBallman]

However, I'm changing the label to enhancement rather than closing, because WG21 has reopened discussion of https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0609r1.pdf and may accept it for C++2y at which point we could potentially diagnose unused structured binding decls.

FWIW, the discussion did get reopened and the committee approved the paper in principle for C++2y, but I have to produce an update with some corrections before it goes on to Core. So nothing final, but it looks plausible.

Why such implicit warning suppression could be useful? For which tasks, use cases, etc?

When binding to a structure, array, or tuple, there's no reason to believe all of the fields are intended to be used together. So having unused structured bindings is very common; if we warned on them by default, we'd cause quite a lot of diagnostics that the user can't do anything about short of writing an extra expression to silence the warning, which doesn't add any real clarity to the code. So by default, we don't diagnose unused structured bindings so that we keep the -Wunused label generally useful.

Once we allow attributes on structured bindings, then there are TWO ways to address the unused name problem without turning the warning off entirely, and it might be more reasonable for us to start diagnosing by default (we'd have to see how disruptive of a change that would be at this point given how long we've been silent about it).

[alexandrustinov]

Why such implicit warning suppression could be useful? For which tasks, use cases, etc?

When binding to a structure, array, or tuple, there's no reason to believe all of the fields are intended to be used together. So having unused structured bindings is very common; if we warned on them by default, we'd cause quite a lot of diagnostics that the user can't do anything about short of writing an extra expression to silence the warning, which doesn't add any real clarity to the code. So by default, we don't diagnose unused structured bindings so that we keep the -Wunused label generally useful.

Ok, reason about arrays, structures and tuples is clear, but I did not ask to diagnose the unused array elements, unused structure fields or tuple elements, right?
All those things must become untouched due gigibytes of existing code, as well as "by design" reasons.

I asked about unused structure binding variables: after such assignment they should not be treated as single structure (copyable/moveable/etc), such complex structure simply does not exist there anymore, all those variables now are standalone.
And expected behavior there is just the same like about the old good regular ordinal variables, not record/class fields.

If someone don't like that treatment (as standalone and "independent" from each other variables) - he/she can always continue to use the old good complex variables (records, objects, etc), what else is the reason to destructure them?

Kindly please take a look into my first example above once again:

int main() {
    auto [value, error] = func(0); // simulating faulure

//    if (!error) // we forgot to handle error code, compiler should generate warning about unused variable, but it does not
        printf("value = %d", value);

}

That way I really need to force compiler to make warning about the unhandled return error code.
This approach is not new and is not my personal invention - that way behave Go and I mentioned that before: https://go.dev/doc/tutorial/handle-errors

The approach was introduced in Go by purpose - they are trying to eliminate GOTO nature of the Exception handling.
Such warnings force developers to explicitly handle the return error codes as much close to error origins as possible.
Also such diagnostic is especially good approach if errors are enums and we can additionally diagnose missing branches in
switch (error) { .... } statements.

BTW - the switch (error) diagnostic is available in C++, the only missing part - is to diagnose missing statement switch (error) itself.

I hope the Go-like error handing approach is good reason to have such diagnostic.

Alternatively, BTW, it's possible to have -Wunused-structure-binding flag, and to have it disabled by default. That way no one existing code and/or developer will suffer, probably excepting -Wpedantic ones.

Why we can't have the same in C++?

Once we allow attributes on structured bindings, then there are TWO ways to address the unused name problem without turning the warning off entirely, and it might be more reasonable for us to start diagnosing by default (we'd have to see how disruptive of a change that would be at this point given how long we've been silent about it).

And all that is just about warnings, not about final machine code.
The [[unused]] attribute is nice feature and I see no reason to do not have it there, no objections at all, I completely agree there.

But even right now is possible to suppress such warnings by (void) var; construct, the UNUSED(var) macro, I see no potential impact there, we don't need to wait the availability of attributes.

[AaronBallman]

I asked about unused structure binding variables: after such assignment they should not be treated as single structure (copyable/moveable/etc), such complex structure simply does not exist there anymore, all those variables now are standalone.
And expected behavior there is just the same like about the old good regular ordinal variables, not record/class fields.

Perhaps there's some reasonable confusion; structured bindings are a bit odd. Bindings are basically labels back to members of an object -- they are not declaring variables themselves. Taking the easy case of a structure being bound:

struct S { int i, j; } s;

int main() {
  auto [val1, val2] = s;
  return val1 + val2;
}

The way this works under the hood is akin to:

struct S { int i, j; } s;

int main() {
  S __s = s; // This is the decomposition declaration
  return __s.i + __s.j; // And the structured bindings give alternative names to the fields in the decomposition declaration
}

so there's no allocation made for val1 or val2 because they're not really objects, just names. You can see that in: https://godbolt.org/z/sdj3Mvqhb (note the LLVM IR, which is identical between foo and bar aside from debug info).

This makes structured bindings a bit weird -- we will warn if you don't use the decomposition declaration (because then there really is an unused variable involved for the decomposition declaration, as in https://godbolt.org/z/cq7hW484G), but if we warn about the bindings themselves, we put users in a bad position. You cannot bind to arbitrary members; you have to bind them all. So taking my example, if you only wanted the x and y members of the structure, you have to then add (void)index; to silence the unused variable diagnostic because the language gives you no other options. The cast-to-void trick is an a matter of QoI for implementations and not something you can rely on (though IMO it's a pretty safe bet in practice). If there's only one field to ignore, that's reasonable -- but if you have four or five fields to ignore, that's a harder sell. The C++ committee is considering adding placeholder syntax so you can use _ to signal "skip this member entirely" to solve this problem. It has some support behind it, but it's not been approved yet (https://wg21.link/p2169r3).

Alternatively, BTW, it's possible to have -Wunused-structure-binding flag, and to have it disabled by default. That way no one existing code and/or developer will suffer, probably excepting -Wpedantic ones.

We typically do not add any new off-by-default warnings in Clang because our experience over the past decade or so is that not enough people enable them in practice to be worth the cost to develop and maintain them. We will still add them when the situation is sufficient to warrant it (for example, a new analysis pass that requires building a CFG), but this situation isn't about the expense of the diagnostic, it's about the utility until we get better language facilities for addressing the diagnostic. Because if the facilities aren't sufficient, our experience is that folks disable the diagnostic entirely.

That said, it is still a possibility that we add that warning group and that may very well be the approach we take in the future except the diagnostic is on by default. But I think on-by-default without portable language facilities to silence the diagnostic will result in a bad user experience for a not-insignificant portion of our users.