llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.68k stars 11.86k forks source link

Clang doesn't require references to be listed in lambda capture #34213

Open 0775a785-4f8d-448e-9da0-14f81adf2169 opened 7 years ago

0775a785-4f8d-448e-9da0-14f81adf2169 commented 7 years ago
Bugzilla Link 34865
Version 5.0
OS Linux
CC @boris-kolpackov,@DougGregor,@cpplearner,@JVApen,@zygoloid

Extended Description

Clang will compile this faulty code without an error:

int var = 0;
int main() {
    int& varRef = var;
    return []{return varRef;}();
}

This also leads to an incorrect warning if varRef is correctly captured (which is how we noticed this issue in the first place):

> clang++ test2.cpp --std=c++17 -Wall 
test2.cpp:4:14: warning: lambda capture 'varRef' is not required to be captured for this use [-Wunused-lambda-capture]
    return [&varRef]{return varRef;}();
             ^

This is a reduced example from this real-world code: https://github.com/mongodb/mongo/blob/r3.5.13/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp#L383-L402 (clang++-5.0 -Wall warns that kRsOplogNamespace is unused even though it is used on line 402)

JVApen commented 2 years ago

mentioned in issue llvm/llvm-bugzilla-archive#41623

f8e85f34-88d0-4ec5-bbf1-7aa1a2a92a2e commented 2 years ago

mentioned in issue llvm/llvm-bugzilla-archive#41623

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 2 years ago

mentioned in issue llvm/llvm-bugzilla-archive#35669

JVApen commented 5 years ago

Bug llvm/llvm-bugzilla-archive#41623 has been marked as a duplicate of this bug.

f8e85f34-88d0-4ec5-bbf1-7aa1a2a92a2e commented 6 years ago

In my (brief) testing, clang provided the counterintuitive behaviour.

On my machine the program in comment #​7 returns 0, which means the assignment does not take effect on var, I think. https://wandbox.org/permlink/tvmcwvTsIL2yAC5h seems to agree with me.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

In my (brief) testing, clang provided the counterintuitive behaviour.

f8e85f34-88d0-4ec5-bbf1-7aa1a2a92a2e commented 6 years ago

If varRef is captured by copy, the lambda will access the copy instead of the original variable. In this case the capture actually has an effect despite the -Wunused-lambda-capture warning. Is this also intentional?


int var = 0; int main() { int& varRef = var; [varRef]() mutable {varRef = 1;}(); // warning: lambda capture 'varRef' is not required to be captured for this use [-Wunused-lambda-capture] return var; // 0 }

[expr.prim.lambda.capture]/11 says:

"Every id-expression within the compound-statement of a lambda-expression that is an odr-use (6.2) of an entity captured by copy is transformed into an access to the corresponding unnamed data member of the closure type. [ Note: An id-expression that is not an odr-use refers to the original entity, never to a member of the closure type. However, such an id-expression can still cause the implicit capture of the entity. — end note ]"

So this assigns to 'var', not to a copy thereof. The warning seems especially relevant in this case, because the result of the rules is highly counterintuitive.

So the warning is correct and the behavior is not, and it seems that no compiler actually implements the "correct" but highly counterintuitive behavior :\

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

Bug llvm/llvm-bugzilla-archive#35669 has been marked as a duplicate of this bug.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

If varRef is captured by copy, the lambda will access the copy instead of the original variable. In this case the capture actually has an effect despite the -Wunused-lambda-capture warning. Is this also intentional?

int var = 0; int main() { int& varRef = var; [varRef]() mutable {varRef = 1;}(); // warning: lambda capture 'varRef' is not required to be captured for this use [-Wunused-lambda-capture] return var; // 0 }

[expr.prim.lambda.capture]/11 says:

Every id-expression within the compound-statement of a lambda-expression that is an odr-use (6.2) of an entity captured by copy is transformed into an access to the corresponding unnamed data member of the closure type. [ Note: An id-expression that is not an odr-use refers to the original entity, never to a member of the closure type. However, such an id-expression can still cause the implicit capture of the entity. — end note ]

So this assigns to var, not to a copy thereof. The warning seems especially relevant in this case, because the result of the rules is highly counterintuitive.

f8e85f34-88d0-4ec5-bbf1-7aa1a2a92a2e commented 6 years ago

If varRef is captured by copy, the lambda will access the copy instead of the original variable. In this case the capture actually has an effect despite the -Wunused-lambda-capture warning. Is this also intentional?

int var = 0;
int main() {
    int& varRef = var;
    [varRef]() mutable {varRef = 1;}(); // warning: lambda capture 'varRef' is not required to be captured for this use [-Wunused-lambda-capture]
    return var; // 0
}
ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

@​Richard If there is more than one use of varRef, to which use should the warning point to?

Pointing to just the first one would seem OK. (But we should also indicate in the diagnostic text that there are other uses in the same situation.)

llvmbot commented 6 years ago

@Richard If there is more than one use of varRef, to which use should the warning point to?

    return [&varRef]{
        int copy = varRef;
        return varRef + copy;
    }();
ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

Reopening: while the warning may be correct, the location if the diagnostic is not quite right. We should instead be pointing at the point of use:

test2.cpp:4:29: warning: lambda capture 'varRef' is not required to be captured for this use [-Wunused-lambda-capture]
    return [&varRef]{return varRef;}();
                            ^

... and it'd be a good idea to add a note, something like:

test2.cpp:3:29: note: 'varRef' is a reference bound to a constant expression evaluating to 'var'; that lvalue will be used in place of 'varRef' in the lambda body
    int& varRef = var;
         ^
ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

Clang is correct. Only odr-used variables need to be captured. Naming a reference initialized by a constant expression does not constitute an odr-use.

See http://wiki.edg.com/pub/Wg21albuquerque/CoreWorkingGroup/cwg_defects.html#1472 which introduced the "references initialized by constant expressions don't need to be captured" rule.

llvmbot commented 7 years ago

Found a related place this happens in Mongo's codebase: https://github.com/mongodb/mongo/blob/6dcdd23dd37ef12c87e71cf59ef01cd82432efe0/src/mongo/util/net/hostname_canonicalization.cpp#L95

In this case, the thing being captured is a function reference, but the underlying cause it likely the same.

llvmbot commented 7 years ago

I think the issue is somewhere related to https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/Sema/SemaExpr.cpp#L14580

markUsed() expects a bool toggling whether it's an ODR use, but "BuildAndDiagnose doesn't seem to relate to that. Kinda shooting in the dark though, as I've only just started digging into the codebase.

felker commented 1 year ago

This behavior changed at some point? https://github.com/llvm/llvm-project/issues/34213#issuecomment-980987301 mentions https://wandbox.org/permlink/tvmcwvTsIL2yAC5h which uses clang 6.0.1 and returns 0.

I got the same result with clang 7.1.0 and 8.1.0, but starting with 9.0.1, the snippet returns 1