[clangd] Dont require confirmation for include-cleaner batch-fixes #76826
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
False negative/positive rate has decreased to the degree that these extra confirmations no longer provide any value, but only create friction in the happy case. When we have false analysis, people usually need to apply the fixes and run the builds to discover the failure. At that point they can add relevant IWYU pragmas to guide analysis, and will be more likely to create bug reports due to extra annoyance :)
|
@llvm/pr-subscribers-clangd Author: kadir çetinkaya (kadircet) ChangesFalse negative/positive rate has decreased to the degree that these When we have false analysis, people usually need to apply the fixes and Full diff: https://github.com/llvm/llvm-project/pull/76826.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 563a1f5d22f5b5..672140a6f2b4d8 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -48,7 +48,6 @@
#include <cassert>
#include <iterator>
#include <map>
-#include <memory>
#include <optional>
#include <string>
#include <utility>
@@ -237,18 +236,6 @@ removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
Diag.Fixes.front().Edits.begin(),
Diag.Fixes.front().Edits.end());
}
-
- // TODO(hokein): emit a suitable text for the label.
- ChangeAnnotation Annotation = {/*label=*/"",
- /*needsConfirmation=*/true,
- /*description=*/""};
- static const ChangeAnnotationIdentifier RemoveAllUnusedID =
- "RemoveAllUnusedIncludes";
- for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) {
- ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I);
- RemoveAll.Edits[I].annotationId = ID;
- RemoveAll.Annotations.push_back({ID, Annotation});
- }
return RemoveAll;
}
@@ -268,20 +255,8 @@ addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
Edits.try_emplace(Edit.newText, Edit);
}
}
- // FIXME(hokein): emit used symbol reference in the annotation.
- ChangeAnnotation Annotation = {/*label=*/"",
- /*needsConfirmation=*/true,
- /*description=*/""};
- static const ChangeAnnotationIdentifier AddAllMissingID =
- "AddAllMissingIncludes";
- unsigned I = 0;
- for (auto &It : Edits) {
- ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+ for (auto &It : Edits)
AddAllMissing.Edits.push_back(std::move(It.second));
- AddAllMissing.Edits.back().annotationId = ID;
-
- AddAllMissing.Annotations.push_back({ID, Annotation});
- }
return AddAllMissing;
}
Fix fixAll(const Fix &RemoveAllUnused, const Fix &AddAllMissing) {
diff --git a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
index af7cdba5b05f43..07ebe1009a78f6 100644
--- a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
+++ b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
@@ -157,21 +157,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT: "AddAllMissingIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "AddAllMissingIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: }
-# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -185,7 +174,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -213,29 +201,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT: "AddAllMissingIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "AddAllMissingIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "RemoveAllUnusedIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "RemoveAllUnusedIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: }
-# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes0",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -249,7 +218,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes1",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -263,7 +231,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -277,7 +244,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -342,21 +308,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT: "RemoveAllUnusedIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "RemoveAllUnusedIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: }
-# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes0",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -370,7 +325,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes1",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -398,29 +352,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT: "AddAllMissingIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "AddAllMissingIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "RemoveAllUnusedIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: },
-# CHECK-NEXT: "RemoveAllUnusedIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT: }
-# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes0",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -434,7 +369,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes1",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -448,7 +382,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -462,7 +395,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
|
hokein
approved these changes
Jan 4, 2024
There was a problem hiding this comment.
This change makes sense for removing all unused includes (as all these unused includes are visible in the editor). For missing includes, they are less obvious, but it is probably fine.
(I think it would be great to have a way to view all missing includes, VSCode has a feature to preview code action changes, but it seems broken with clangd.)
The rationale for missing includes was the same, when things go wrong it's usually obscure enough that problems only raise with build systems that have visibility restrictions. Hence I am not sure if this extra step provides enough value.
Agreed, making alt+enter work in these cases would be useful, and last time I checked it is actually working. I tried with this patch, and it still gives people a way to preview and selectively apply these edits. So we're actually not taking away the functionality completely, but rather putting this behind an extra shortcut :) Can you check if it's still broken for you ? |
|
oh sorry, this should ofc work with include-cleaner fixes, as we actually provide these as edits, instead of commands, as we do with our usual tweaks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
False negative/positive rate has decreased to the degree that these
extra confirmations no longer provide any value, but only create
friction in the happy case.
When we have false analysis, people usually need to apply the fixes and
run the builds to discover the failure. At that point they can add
relevant IWYU pragmas to guide analysis, and will be more likely to
create bug reports due to extra annoyance :)