llvm / llvm-project

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

Unused lambda capture source ranges are slightly off #106445

Open tbaederr opened 2 months ago

tbaederr commented 2 months ago

See: https://godbolt.org/z/MKjzsYb9b

int main() {

    int a = 0, b = 0;

    auto F = [&a, &b]() {

    };
}

The output is:

<source>:7:16: warning: lambda capture 'a' is not used [-Wunused-lambda-capture]
    7 |     auto F = [&a, &b]() {
      |               ~^~
<source>:7:20: warning: lambda capture 'b' is not used [-Wunused-lambda-capture]
    7 |     auto F = [&a, &b]() {
      |                 ~~~^
<source>:7:10: warning: unused variable 'F' [-Wunused-variable]
    7 |     auto F = [&a, &b]() {
      |          ^

1) The source range for &a includes the comma after it 2) The source range for &b includes both the comma and the whitespace between that and the capture

The second point is especially fun when inserting some useless whitespace:

    auto F = [&a,         

     &b]() {

    };
llvmbot commented 2 months ago

@llvm/issue-subscribers-clang-frontend

Author: Timm Baeder (tbaederr)

See: https://godbolt.org/z/MKjzsYb9b ```c++ int main() { int a = 0, b = 0; auto F = [&a, &b]() { }; } ``` The output is: ```console <source>:7:16: warning: lambda capture 'a' is not used [-Wunused-lambda-capture] 7 | auto F = [&a, &b]() { | ~^~ <source>:7:20: warning: lambda capture 'b' is not used [-Wunused-lambda-capture] 7 | auto F = [&a, &b]() { | ~~~^ <source>:7:10: warning: unused variable 'F' [-Wunused-variable] 7 | auto F = [&a, &b]() { | ^ ``` 1) The source range for `&a` includes the comma after it 2) The source range for `&b` includes both the comma and the whitespace between that and the capture The second point is especially fun when inserting some useless whitespace: ```c++ auto F = [&a, &b]() { }; ```
llvmbot commented 2 months ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot commented 2 months ago

@llvm/issue-subscribers-good-first-issue

Author: Timm Baeder (tbaederr)

See: https://godbolt.org/z/MKjzsYb9b ```c++ int main() { int a = 0, b = 0; auto F = [&a, &b]() { }; } ``` The output is: ```console <source>:7:16: warning: lambda capture 'a' is not used [-Wunused-lambda-capture] 7 | auto F = [&a, &b]() { | ~^~ <source>:7:20: warning: lambda capture 'b' is not used [-Wunused-lambda-capture] 7 | auto F = [&a, &b]() { | ~~~^ <source>:7:10: warning: unused variable 'F' [-Wunused-variable] 7 | auto F = [&a, &b]() { | ^ ``` 1) The source range for `&a` includes the comma after it 2) The source range for `&b` includes both the comma and the whitespace between that and the capture The second point is especially fun when inserting some useless whitespace: ```c++ auto F = [&a, &b]() { }; ```
shafik commented 2 months ago

Note the source range for a does not include the comma after it if you have whitespace: https://godbolt.org/z/sGEYPaThs

c8ef commented 2 months ago

Hi, I would like to work on this.

c8ef commented 2 months ago

I believe the issue was introduced in https://github.com/llvm/llvm-project/commit/832f49b90a4907a0bc2b34b688c32f3c9613aab0. Interpreting the source range as the fix-it range rather than the diagnostic range actually makes some kind of sense.

c8ef commented 2 months ago

So I’m wondering if this is the intended behavior. If we want to fix it, where should the changes be made? From the perspective of the fix-it suggestion, the comma still needs to be bound to a variable, either the former or the latter.

tbaederr commented 2 months ago

Right, those aren't just ranges, but delete ranges from the fixme. In that case, I'm not sure what to do. :upside_down_face:

jhendle2 commented 1 month ago

For https://godbolt.org/z/8zdKPxY6K

int main() {

    int a = 0, b = 0;

    auto F = [&a, &b]() {

    };
}

The output is:

<source>:11:16: warning: lambda capture 'a' is not used [-Wunused-lambda-capture]
   11 |     auto F = [&a, &b]() {
      |               ~^~
<source>:11:20: warning: lambda capture 'b' is not used [-Wunused-lambda-capture]
   11 |     auto F = [&a, &b]() {
      |                 ~~~^
<source>:11:10: warning: unused variable 'F' [-Wunused-variable]
   11 |     auto F = [&a, &b]() {
      |          ^

So, I assume the third warning output should be:

<source>:11:20: warning: lambda capture 'b' is not used [-Wunused-lambda-capture]
   11 |     auto F = [&a, &b]() {
      |                   ~^

Because this matches the behavior for a lambda with a single unsued capture?

<source>:11:16: warning: lambda capture 'a' is not used [-Wunused-lambda-capture]
   11 |     auto F = [&a]() {
      |               ~^
tbaederr commented 1 month ago

Kinda, yes. But the output is already correct as it is, when we consider that the source ranges aren't supposed to just highlight the unused parameter. They are fix-it hints that will remove the underlined text when applied.

jhendle2 commented 1 month ago

Hi, can this ticket be assigned to me?