Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UnusedIncludes dosen't check angle includes #1774

Open
FeignClaims opened this issue Oct 1, 2023 · 6 comments
Open

UnusedIncludes dosen't check angle includes #1774

FeignClaims opened this issue Oct 1, 2023 · 6 comments

Comments

@FeignClaims
Copy link

image image

configured using cmake -S . -B build -G 'Ninja' -DCMAKE_EXPORT_COMPILE_COMMANDS=ON

CMakeLists.txt

cmake_minimum_required(VERSION 3.27)
project(starter)

add_executable(main)
target_sources(main
  PRIVATE
  main.cpp
)
target_include_directories(main
  PRIVATE
  .
)

main.cpp

#include <hello.hpp>

hello.hpp

#ifndef HELLO_HPP
#define HELLO_HPP

inline void hello() {}

#endif

.clangd

Diagnostics:
  UnusedIncludes: Strict

Logs

for #include "hello.hpp"

V[19:46:25.318] <<< {"id":31,"jsonrpc":"2.0","method":"textDocument/documentHighlight","params":{"position":{"character":19,"line":0},"textDocument":{"uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}}

I[19:46:25.319] <-- textDocument/documentHighlight(31)
V[19:46:25.319] ASTWorker running Highlights on version 94 of /Users/feignclaims/code/cpp/temp/main.cpp
I[19:46:25.319] --> reply:textDocument/documentHighlight(31) 0 ms
V[19:46:25.319] >>> {"id":31,"jsonrpc":"2.0","result":[]}

I[19:46:25.319] --> textDocument/clangd.fileStatus
V[19:46:25.319] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}

V[19:46:25.569] <<< {"id":32,"jsonrpc":"2.0","method":"textDocument/codeAction","params":{"context":{"diagnostics":[{"code":"unused-includes","codeDescription":{"href":"https://clangd.llvm.org/guides/include-cleaner"},"message":"Included header hello.hpp is not used directly (fix available)","range":{"end":{"character":20,"line":0},"start":{"character":0,"line":0}},"relatedInformation":[],"severity":2,"source":"clangd","tags":[1]}],"triggerKind":2},"range":{"end":{"character":19,"line":0},"start":{"character":19,"line":0}},"textDocument":{"uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}}

I[19:46:25.569] <-- textDocument/codeAction(32)
V[19:46:25.569] ASTWorker running codeAction on version 94 of /Users/feignclaims/code/cpp/temp/main.cpp
I[19:46:25.569] --> reply:textDocument/codeAction(32) 0 ms
V[19:46:25.569] >>> {"id":32,"jsonrpc":"2.0","result":[{"diagnostics":[{"code":"unused-includes","message":"Included header hello.hpp is not used directly (fix available)","range":{"end":{"character":20,"line":0},"start":{"character":0,"line":0}},"severity":2,"source":"clangd"}],"edit":{"documentChanges":[{"edits":[{"newText":"","range":{"end":{"character":0,"line":1},"start":{"character":0,"line":0}}}],"textDocument":{"uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp","version":94}}]},"isPreferred":true,"kind":"quickfix","title":"remove #include directive"}]}

I[19:46:25.570] --> textDocument/clangd.fileStatus
V[19:46:25.570] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}

for #include <hello.hpp>

V[19:45:55.341] <<< {"id":8,"jsonrpc":"2.0","method":"textDocument/documentHighlight","params":{"position":{"character":19,"line":0},"textDocument":{"uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}}

I[19:45:55.341] <-- textDocument/documentHighlight(8)
V[19:45:55.341] ASTWorker running Highlights on version 92 of /Users/feignclaims/code/cpp/temp/main.cpp
I[19:45:55.341] --> reply:textDocument/documentHighlight(8) 0 ms
V[19:45:55.341] >>> {"id":8,"jsonrpc":"2.0","result":[]}

I[19:45:55.341] --> textDocument/clangd.fileStatus
V[19:45:55.341] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}

V[19:45:55.590] <<< {"id":9,"jsonrpc":"2.0","method":"textDocument/codeAction","params":{"context":{"diagnostics":[],"triggerKind":2},"range":{"end":{"character":19,"line":0},"start":{"character":19,"line":0}},"textDocument":{"uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}}

I[19:45:55.590] <-- textDocument/codeAction(9)
V[19:45:55.590] ASTWorker running codeAction on version 92 of /Users/feignclaims/code/cpp/temp/main.cpp
I[19:45:55.590] --> reply:textDocument/codeAction(9) 0 ms
V[19:45:55.590] >>> {"id":9,"jsonrpc":"2.0","result":[]}

I[19:45:55.590] --> textDocument/clangd.fileStatus
V[19:45:55.590] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///Users/feignclaims/code/cpp/temp/main.cpp"}}

System information

Output of clangd --version:

Homebrew clangd version 17.0.1
Features: mac+xpc
Platform: arm64-apple-darwin23.0.0

Editor/LSP plugin:

VSCode clangd 0.1.24

Operating system:

arm64-apple-darwin22.5.0
@HighCommander4
Copy link

This has come up in clangd/vscode-clangd#452 as well.

I suspect it may be intentional, though I don't know the motivation for it. Perhaps some folks who worked on include-cleaner such as @VitaNuo or @kadircet can provide some details.

@kiwixz
Copy link

kiwixz commented Nov 21, 2023

Clangd currently doesn't support detecting what's used in transitive includes. You can see it with such a setup:

  • super_specific_header.hpp
#pragma once

void mylib_specific_function();
  • mylib.h
#pragma once

#include "super_specific_header.hpp"
  • main.cpp
// doc says you should use this umbrella header to pull the whole thing
// but you'll get a warning if you used "" instead of <>
#include "mylib.h"

int main() { mylib_specific_function(); }

So clangd simply ignores all angle includes (except from std) to not produce false positives with this common pattern.

See here if you want to work on this.

@HighCommander4
Copy link

Assuming that an angle include is a system header doesn't seem justified, since, when resolving an angle include, the compiler does look at regular entries of the include search path after looking at system entries.

A more accurate check for "system header" would be that it was found in an entry of the include search path specified using -isystem.

@wkelton
Copy link

wkelton commented Mar 13, 2024

I stumbled upon this behavior. Was rather confusing. I would definitely like (and expected) includes using <> to be reported as unused just like includes with "".

@sam-mccall
Copy link
Member

sam-mccall commented Mar 13, 2024

@kadircet Is this check still needed?

Standalone clang-include-cleaner doesn't have it, so I suspect the new include-cleaner lib is robust enough to handle system includes (at least for the people willing to use it). If we want to conservatively avoid analyzing system headers we should at least switch to CharacteristicKind (-isystem) and make this a config option with a conservative default.

The original intent was to disable analysis of weird-layout libraries like C++ standard library, C standard library, posix etc. (I think this was using the old clangd-specific implementation which was more limited).
Angle was a heuristic to cover those, and also other suspected cases. I don't remember if there were technical issues with using CharacteristicKind or it was just oversight.

@kiwixz
Copy link

kiwixz commented Mar 13, 2024

The original intent was to disable analysis of weird-layout libraries like C++ standard library, C standard library, posix etc.

I don't think that's it. Standard library headers are handled separately. This was done to avoid errors with umbrella headers.

It'd be nice to keep it this behavior at least behind an option so I don't get spammed with warnings for third party libs, but still enjoy the check on my code that I include with ".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants