Page MenuHomePhabricator

Argument name support for function pointer signature hints
ClosedPublic

Authored by Qwinci on May 6 2022, 12:29 PM.

Diff Detail

Event Timeline

Qwinci created this revision.May 6 2022, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 12:29 PM
Qwinci requested review of this revision.May 6 2022, 12:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 6 2022, 12:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Qwinci planned changes to this revision.May 6 2022, 12:34 PM
nridge added a subscriber: nridge.May 6 2022, 2:19 PM
Qwinci updated this revision to Diff 427948.May 8 2022, 11:00 AM

Added parameter names to signature help.

Qwinci updated this revision to Diff 427949.May 8 2022, 11:11 AM
This comment was removed by Qwinci.
Qwinci updated this revision to Diff 427950.May 8 2022, 11:20 AM

Added parameter names to signature hints.

Qwinci retitled this revision from Initial work on parameter info for function pointers to Argument name support for function pointer signature hints.May 8 2022, 11:25 AM
nridge added a comment.May 9 2022, 1:36 AM

Thanks for your work on this!

Could you add the two cases we're handling (typedef to function pointer type, and variable directly of function pointer type) as test cases to CodeCompleteTests.cpp? They could work similarly to existing signature-help tests like this one.

Qwinci updated this revision to Diff 428066.May 9 2022, 6:45 AM

Added unit tests for function pointer signature help.

Thanks again for the patch, and sorry for the long turnaround time.

The patch generally looks correct to me, I just have a few comments regarding style, commenting, etc.

clang/include/clang/Sema/CodeCompleteConsumer.h
1022

Let's add:

/// The candidate is a variable or expression of function type
/// for which we have the location of the protoype declaration.
1050
/// The location of the function prototype that describes the entity being called,
/// when Kind == CK_FunctionProtoTypeLoc.
1109

nit: FunctionProtoTypeLoc

Let's also add:

/// This can be called for any Kind, but returns null for kinds
// other than CK_FunctionProtoTypeLoc.

(Technically, we could dig out a FunctionProtoTypeLoc from a FunctionDecl, but there's no real motivation to do so here.)

clang/lib/Sema/CodeCompleteConsumer.cpp
550

This should be unnecessary, as we implemented getFunctionType() for the CK_FunctionProtoTypeLoc case.

613

else if (Kind == CK_FunctionProtoTypeLoc) would be more direct as a condition

clang/lib/Sema/SemaCodeComplete.cpp
3773

We can probably avoid a bit of repetition here:

if (Function || PrototypeLoc) {
  const ParamVarDecl *Param = Function ? Function->getParamDecl(P) : PrototypeLoc.getParam(P);
  // use Param
} else {
  // existing fallback branch
}
3774

I think, now that we understand better why this was crashing before (we were mis-using the union), this can be an assertion instead. (Otherwise, if P were ever out of range, then e.g. Prototype->getParamType(P) in the fallback case would crash as well.)

6003

Let's add a comment similar to:

// Given a callee expression `Fn`, if the call is through a function pointer,
// try to find the declaration of the corresponding function pointer type,
// so that we can recover argument names from it.
6018

Could you clang-format the patch please? (I have vscode do this as I write the code by enabling editor.formatOnSave and setting clang-format as my formatter for C++ files, but you can also do it using git clang-format or similar.)

6021

I think we can reorganize the function slightly to avoid repeating this part:

TypeLoc Target; // start off as null
if (/*function has typedef type*/) {
  // populate Target, without any unwrapping
} else if (/*expression refers to a variable*/) {
  // populate Target, without any unwrapping
}

// Now unwrap PointerType, ParenType
// check if we have a FunctionProtoTypeLoc and return
6128

TypeLoc has an operator bool, so this could also be written if (P)

nridge requested changes to this revision.Jul 10 2022, 9:07 PM

Hi @Qwinci -- do you plan to update this patch to address the review comments?

This revision now requires changes to proceed.Jul 10 2022, 9:07 PM

Hi @Qwinci -- do you plan to update this patch to address the review comments?

Yeah I can update it. I just have had other stuff to do so I haven't got time to make the changes and recompile llvm (I also forgot this patch for a while. Ill try to make the changes today/tomorrow and then recompile).

Qwinci updated this revision to Diff 445433.Jul 18 2022, 2:40 AM

Fixed formatting and made some improvements.

Qwinci marked 10 inline comments as done.Jul 18 2022, 2:44 AM
Qwinci marked an inline comment as done.
nridge accepted this revision.Tue, Jul 19, 1:17 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Tue, Jul 19, 1:17 AM
nridge edited the summary of this revision. (Show Details)Tue, Jul 19, 1:18 AM

I'm getting a test failure when running SignatureHelpTest.VariadicType:

[ RUN      ] SignatureHelpTest.VariadicType
Built preamble of size 179520 for file /clangd-test/TestTU.cpp version null in 0.02 seconds
ClangdTests: llvm/src/clang/include/clang/AST/Type.h:691: const clang::ExtQualsTypeCommonBase *clang::QualType::getCommonPtr() const: Assertion `!isNull() && "Cannot retrieve a NULL type pointer"' failed.
 #0 0x00007f7f6216966a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm/src/llvm/lib/Support/Unix/Signals.inc:565:11
 #1 0x00007f7f6216981b PrintStackTraceSignalHandler(void*) llvm/src/llvm/lib/Support/Unix/Signals.inc:632:1
 #2 0x00007f7f62167eb6 llvm::sys::RunSignalHandlers() llvm/src/llvm/lib/Support/Signals.cpp:103:5
 #3 0x00007f7f62169f45 SignalHandler(int) llvm/src/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007f7f69242730 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12730)
 #5 0x00007f7f6150e7bb raise /build/glibc-fWwxX8/glibc-2.28/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #6 0x00007f7f614f9535 abort /build/glibc-fWwxX8/glibc-2.28/stdlib/abort.c:81:7
 #7 0x00007f7f614f940f _nl_load_domain /build/glibc-fWwxX8/glibc-2.28/intl/loadmsgcat.c:1177:9
 #8 0x00007f7f61507102 (/lib/x86_64-linux-gnu/libc.so.6+0x30102)
 #9 0x00007f7f64a6dec7 clang::QualType::getCommonPtr() const llvm/src/clang/include/clang/AST/Type.h:0:5
#10 0x00007f7f64a6de55 clang::QualType::getTypePtr() const llvm/src/clang/include/clang/AST/Type.h:6501:26
#11 0x00007f7f64a86545 clang::TypeLoc::getTypePtr() const llvm/src/clang/include/clang/AST/TypeLoc.h:137:5
#12 0x00007f7f64a8fb8b clang::ConcreteTypeLoc<clang::UnqualTypeLoc, clang::PointerTypeLoc, clang::PointerType, clang::PointerLikeLocInfo>::isKind(clang::TypeLoc const&) llvm/src/clang/include/clang/AST/TypeLoc.h:370:36
#13 0x00007f7f64ef2329 clang::PointerTypeLoc clang::TypeLoc::getAs<clang::PointerTypeLoc>() const llvm/src/clang/include/clang/AST/TypeLoc.h:89:9
#14 0x00007f7f64e366f4 GetPrototypeLoc(clang::Expr*) llvm/src/clang/lib/Sema/SemaCodeComplete.cpp:6011:23
#15 0x00007f7f64e361d1 clang::Sema::ProduceCallSignatureHelp(clang::Expr*, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation) llvm/src/clang/lib/Sema/SemaCodeComplete.cpp:6108:32
#16 0x00007f7f5ca9bfbd clang::Parser::ParsePostfixExpressionSuffix(clang::ActionResult<clang::Expr*, true>)::$_2::operator()() const llvm/src/clang/lib/Parse/ParseExpr.cpp:2049:42
#17 0x00007f7f5ca9e185 clang::QualType llvm::function_ref<clang::QualType ()>::callback_fn<clang::Parser::ParsePostfixExpressionSuffix(clang::ActionResult<clang::Expr*, true>)::$_2>(long) llvm/src/llvm/include/llvm/ADT/STLFunctionalExtras.h:45:12
#18 0x00007f7f5ca9fcd9 llvm::function_ref<clang::QualType ()>::operator()() const llvm/src/llvm/include/llvm/ADT/STLFunctionalExtras.h:68:12
#19 0x00007f7f5ca9e249 clang::PreferredTypeBuilder::get(clang::SourceLocation) const llvm/src/clang/include/clang/Sema/Sema.h:338:14
#20 0x00007f7f5ca8ca27 clang::Parser::ParseAssignmentExpression(clang::Parser::TypeCastState) llvm/src/clang/lib/Parse/ParseExpr.cpp:164:50
#21 0x00007f7f5ca9bacd clang::Parser::ParseExpressionList(llvm::SmallVectorImpl<clang::Expr*>&, llvm::SmallVectorImpl<clang::SourceLocation>&, llvm::function_ref<void ()>, bool, bool) llvm/src/clang/lib/Parse/ParseExpr.cpp:3404:14
#22 0x00007f7f5ca8f9be clang::Parser::ParsePostfixExpressionSuffix(clang::ActionResult<clang::Expr*, true>) llvm/src/clang/lib/Parse/ParseExpr.cpp:2056:15
#23 0x00007f7f5ca96668 clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, bool&, clang::Parser::TypeCastState, bool, bool*) llvm/src/clang/lib/Parse/ParseExpr.cpp:1817:9
#24 0x00007f7f5ca8e5b9 clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, clang::Parser::TypeCastState, bool, bool*) llvm/src/clang/lib/Parse/ParseExpr.cpp:681:20
#25 0x00007f7f5ca8cac8 clang::Parser::ParseAssignmentExpression(clang::Parser::TypeCastState) llvm/src/clang/lib/Parse/ParseExpr.cpp:173:20
#26 0x00007f7f5ca8c97f clang::Parser::ParseExpression(clang::Parser::TypeCastState) llvm/src/clang/lib/Parse/ParseExpr.cpp:124:18
#27 0x00007f7f5cb26aa8 clang::Parser::ParseExprStatement(clang::Parser::ParsedStmtContext) llvm/src/clang/lib/Parse/ParseStmt.cpp:464:19
#28 0x00007f7f5cb256ec clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::ParsedAttributes&) llvm/src/clang/lib/Parse/ParseStmt.cpp:243:12
#29 0x00007f7f5cb25049 clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) llvm/src/clang/lib/Parse/ParseStmt.cpp:113:20
#30 0x00007f7f5cb2d24d clang::Parser::ParseCompoundStatementBody(bool) llvm/src/clang/lib/Parse/ParseStmt.cpp:1111:11
#31 0x00007f7f5cb2e97f clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) llvm/src/clang/lib/Parse/ParseStmt.cpp:2379:21
#32 0x00007f7f5cb55d81 clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) llvm/src/clang/lib/Parse/Parser.cpp:1407:3
#33 0x00007f7f5ca3d0a0 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::SourceLocation*, clang::Parser::ForRangeInit*) llvm/src/clang/lib/Parse/ParseDecl.cpp:2097:27
#34 0x00007f7f5cb54c6b clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) llvm/src/clang/lib/Parse/Parser.cpp:1158:10
#35 0x00007f7f5cb54320 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) llvm/src/clang/lib/Parse/Parser.cpp:1172:12
#36 0x00007f7f5cb53c2b clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsingDeclSpec*) llvm/src/clang/lib/Parse/Parser.cpp:998:12
#37 0x00007f7f5cb51c43 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) llvm/src/clang/lib/Parse/Parser.cpp:727:12
#38 0x00007f7f5ca199b5 clang::ParseAST(clang::Sema&, bool, bool) llvm/src/clang/lib/Parse/ParseAST.cpp:162:16
#39 0x00007f7f66dbd34c clang::ASTFrontendAction::ExecuteAction() llvm/src/clang/lib/Frontend/FrontendAction.cpp:1141:1
#40 0x00007f7f66dbcd4c clang::FrontendAction::Execute() llvm/src/clang/lib/Frontend/FrontendAction.cpp:1036:7
#41 0x00007f7f68255737 clang::clangd::(anonymous namespace)::semaCodeComplete(std::unique_ptr<clang::CodeCompleteConsumer, std::default_delete<clang::CodeCompleteConsumer>>, clang::CodeCompleteOptions const&, clang::clangd::(anonymous namespace)::SemaCompleteInput const&, clang::clangd::IncludeStructure*) llvm/src/clang-tools-extra/clangd/CodeComplete.cpp:1296:19
#42 0x00007f7f682570e6 clang::clangd::signatureHelp(llvm::StringRef, clang::clangd::Position, clang::clangd::PreambleData const&, clang::clangd::ParseInputs const&, clang::clangd::MarkupKind) llvm/src/clang-tools-extra/clangd/CodeComplete.cpp:2056:3
#43 0x0000000000950e46 clang::clangd::(anonymous namespace)::signatures(llvm::StringRef, clang::clangd::Position, std::vector<clang::clangd::Symbol, std::allocator<clang::clangd::Symbol>>, clang::clangd::MarkupKind) llvm/src/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1200:3
#44 0x00000000009506af clang::clangd::(anonymous namespace)::signatures(llvm::StringRef, std::vector<clang::clangd::Symbol, std::allocator<clang::clangd::Symbol>>, clang::clangd::MarkupKind) llvm/src/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1208:3
#45 0x0000000000977a32 clang::clangd::(anonymous namespace)::SignatureHelpTest_VariadicType_Test::TestBody() llvm/src/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2858:25
#46 0x00007f7f69141c4b void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2433:3
#47 0x00007f7f6912b587 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2488:5
#48 0x00007f7f69115133 testing::Test::Run() llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2515:3
#49 0x00007f7f6911598a testing::TestInfo::Run() llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2687:12
#50 0x00007f7f69115eeb testing::TestSuite::Run() llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2815:44
#51 0x00007f7f6911e7d9 testing::internal::UnitTestImpl::RunAllTests() llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:5337:24
#52 0x00007f7f6914535b bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2433:3
#53 0x00007f7f6912d587 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:2488:5
#54 0x00007f7f6911e3bf testing::UnitTest::Run() llvm/src/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
#55 0x00007f7f6922cd01 RUN_ALL_TESTS() llvm/src/llvm/utils/unittest/googletest/include/gtest/gtest.h:2472:3
#56 0x00007f7f6922cc74 main llvm/src/llvm/utils/unittest/UnitTestMain/TestMain.cpp:55:3
#57 0x00007f7f614fb09b __libc_start_main /build/glibc-fWwxX8/glibc-2.28/csu/../csu/libc-start.c:342:3
#58 0x00000000004e507a _start (./tools/clang/tools/extra/clangd/unittests/ClangdTests+0x4e507a)
Aborted
nridge added a comment.EditedTue, Jul 19, 1:33 AM

GetPrototypeLoc() needs an early return for Target.isNull(), you can't call getAs<...>() on a null TypeLoc.

nridge requested changes to this revision.Tue, Jul 19, 1:34 AM
This revision now requires changes to proceed.Tue, Jul 19, 1:34 AM
Qwinci updated this revision to Diff 445745.Tue, Jul 19, 2:25 AM

Added a null check in GetPrototypeLoc().

nridge accepted this revision.Tue, Jul 19, 5:01 PM
This revision is now accepted and ready to land.Tue, Jul 19, 5:01 PM
This revision was landed with ongoing or failed builds.Tue, Jul 19, 5:02 PM
This revision was automatically updated to reflect the committed changes.