[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.
sammccall added a comment. All these things are good ideas - I'm not addressing comments yet because I'm waiting for feedback from the team that's trying this out - maybe this needs patches, or maybe it'll never work. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.
ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:700 + unsigned DiagLoc = Loc.second; + if (DiagLoc < StartOfLine || DiagLoc > Offset) + return; kadircet wrote: > There are also a lot of cases where we can't find an include file(usually due > to confused compile commands) and fail miserably. Maybe in addition to > checking current line, we could also check for lines that are starting with > `#include` or diags of type `diag::err_pp_file_not_found` ? Good point! It's even more complicated than that: some of those missing files might be in preamble. This will lead to preamble being incomplete. The problem with that is that DiagnosticsConsumer we create for completion will not see those errors (we have a separate consumer instance for preamble). So we also need to a way to keep the reason why preamble is broken in the preamble itself. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.
kadircet added inline comments. Comment at: clangd/CodeComplete.cpp:700 + unsigned DiagLoc = Loc.second; + if (DiagLoc < StartOfLine || DiagLoc > Offset) + return; There are also a lot of cases where we can't find an include file(usually due to confused compile commands) and fail miserably. Maybe in addition to checking current line, we could also check for lines that are starting with `#include` or diags of type `diag::err_pp_file_not_found` ? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.
ilya-biryukov added a comment.
Since we're showing the diagnostics in the editors anyway, how crucial do we
think it is to actually add that to the procotol?
Having more concrete reasons for misbehaving completions sounds more useful,
though, e.g. "cannot complete members of the incomplete type", etc.
Totally fine to have this in the C++ API for experiments, of course.
> - LSP provides no protocol mechanims, and editors don't have UI for this
Have we tried returning an error in addition to the results? How do the clients
behave in that case?
Comment at: clangd/CodeComplete.cpp:674
+// Tries to come up with a convincing excuse for our bad completion results,
+// based on the diagnostics near the completion point.
It took some time to figure out what "excuse" means here.
Maybe be more straightforward in naming/comments about what this class does?
I.e. finds a last diagnostic on the same line.
It's private to this file, so it's easy to rename if we'll want to expand the
heuristics.
Comment at: clangd/CodeComplete.cpp:686
+ for (StartOfLine = Offset;
+ StartOfLine > 0 && Code[StartOfLine - 1] != '\n'; --StartOfLine)
+ ;
Maybe use a getLineNumber helper from the SourceManager instead?
It needs a FileID, but it shouldn't be hard to get one.
Comment at: clangd/CodeComplete.cpp:1040
+ IncludeStructure *Includes = nullptr,
+ std::string *Excuse = nullptr) {
trace::Span Tracer("Sema completion");
Maybe replace an out parameter with returning a struct of two members?
Is there any use for calling code completion without providing an "excuse"?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53406
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53406: [clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.
sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ioeric.
In some circumstances we provide bad completions or no completions, because of
problems in the code. This can be puzzling and opaque to users.
If we can tell the user that this is the case and why, they'll be happy.
Experiments with go language servers have shown this to be a big win.
Two major problems:
- Sema doesn't provide the information we need
- LSP provides no protocol mechanims, and editors don't have UI for this
This patch attempts to guess the information, looking at diagnostics on the
line.
Other heuristics are possible (e.g. using completion context). It's unclear how
useful or successful they will be. This is mostly a quick hack to test
viability.
This is exposed as an extension of the C++ API only (not bound to LSP).
The idea is to test viability with a non-public editor that happens to have the
right UI already (and doesn't use LSP), and experiment with approaches.
If something fairly simple works well, we'll keep this and expose an LSP
extension.
If it's not useful, we should drop this idea.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53406
Files:
clangd/ClangdLSPServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2176,6 +2176,50 @@
AllOf(Qualifier("nx::"), Named("Clangd2";
}
+TEST(CompletionTest, Excuses) {
+ struct {
+const char *Code;
+const char *Excuse;
+ } Tests[] = {
+ {
+ R"cpp(
+class X;
+X* x;
+int main() {
+ x->^
+}
+ )cpp",
+ "member access into incomplete type 'X'",
+ },
+ {
+ R"cpp(
+// Error is on same line.
+template struct X;
+X x; ^
+ )cpp",
+ "implicit instantiation of undefined template 'X'",
+ },
+ {
+ R"cpp(
+// Error is on previous line.
+template struct X;
+X x;
+^
+ )cpp",
+ "",
+ },
+ {
+ R"cpp(
+// Just a warning (declaration does not declare anything).
+int; x^
+ )cpp",
+ "",
+ },
+ };
+ for (const auto : Tests)
+EXPECT_EQ(completions(Test.Code).Excuse, Test.Excuse) << Test.Code;
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -196,6 +196,10 @@
std::vector Completions;
bool HasMore = false;
CodeCompletionContext::Kind Context = CodeCompletionContext::CCC_Other;
+
+ // Our best guess at why completions might be poor (human-readable string).
+ // Only set if we suspect completions are poor *and* we know why.
+ std::string Excuse;
};
raw_ostream <<(raw_ostream &, const CodeCompleteResult &);
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -671,6 +671,42 @@
return false;
}
+// Tries to come up with a convincing excuse for our bad completion results,
+// based on the diagnostics near the completion point.
+class ExcuseMaker : public DiagnosticConsumer {
+ unsigned StartOfLine, Offset;
+ std::string
+ SmallString<256> ExcuseBuf;
+
+ public:
+ ExcuseMaker(StringRef Code, unsigned Offset, std::string )
+ : Offset(Offset), Excuse(Excuse) {
+ assert(Offset <= Code.size());
+ for (StartOfLine = Offset;
+ StartOfLine > 0 && Code[StartOfLine - 1] != '\n'; --StartOfLine)
+ ;
+ }
+
+ void HandleDiagnostic(DiagnosticsEngine::Level Level,
+ const clang::Diagnostic ) override {
+ // We accept >= error diagnostics, before the cursor but on the same line.
+ if (Level < DiagnosticsEngine::Error || !Diag.hasSourceManager())
+ return;
+ auto& SM = Diag.getSourceManager();
+ auto Loc = SM.getDecomposedLoc(Diag.getLocation());
+ if (Loc.first != SM.getMainFileID())
+ return;
+ unsigned DiagLoc = Loc.second;
+ if (DiagLoc < StartOfLine || DiagLoc > Offset)
+ return;
+ ExcuseBuf.clear();
+ Diag.FormatDiagnostic(ExcuseBuf);
+ }
+
+ ~ExcuseMaker() { Excuse = ExcuseBuf.str(); }
+};
+
+
// The CompletionRecorder captures Sema code-complete output, including context.
// It filters out ignored results (but doesn't apply fuzzy-filtering yet).
// It doesn't do scoring or conversion to CompletionItem yet, as we
