This is an archive of the discontinued LLVM Phabricator instance.

[clangd] ExtractVariable support for C and Objective-C
ClosedPublic

Authored by dgoldman on Apr 26 2022, 4:46 PM.

Details

Summary
  • Use the expression's type for non-C++ as the variable type. This works well, but might not preserve the typedefs due to type canonicalization.
  • Improve support for Objective-C property references which are represented using ObjCPropertyRefExpr and BuiltinType::PseudoObject.

Diff Detail

Event Timeline

dgoldman created this revision.Apr 26 2022, 4:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 4:46 PM
dgoldman requested review of this revision.Apr 26 2022, 4:46 PM
dgoldman updated this revision to Diff 425354.Apr 26 2022, 5:19 PM

Minor formatting fix

sammccall added inline comments.Apr 27 2022, 8:05 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
193

(does this need to be a method?)

194

CPlusPlus11

197

if it has pseudo-object type and *isn't* handled by the ObjCPropertyRef case, we should disallow extraction right?

198

E->getObjCProperty()?

204

please don't use "" as a sentinel value, it will lead to bugs

Why are we checking this here rather than in eligibleForExtraction?

210

Type + Varname isn't correct for types in general, particularly those that use declarator syntax.
printType with varname as the placeholder should do what you want.

Can you add a testcase where the variable has function pointer type?

215

prefer printType from AST.h, which will get qualifiers, anonymous namespaces etc right

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
326

I'd drop the FIXME here: the fully-sugared type type of NSInteger * long is long, not NSInteger.

(Unless you're suggesting changing this in clang)

dgoldman updated this revision to Diff 425644.Apr 27 2022, 4:37 PM
dgoldman marked 5 inline comments as done.
  • Don't use a method for computing the type
  • Compute the type using printType
  • Support MemberExprs and add a test for function pointers
dgoldman added inline comments.Apr 27 2022, 4:38 PM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
198

Hmm I'm not sure when that would be useful looking at the logic there, can you give an example just to make sure I understand what it would handle?

204

I put it in the ExtractionContext constructor since we would otherwise have to perform the same logic twice to compute the type string. I could make it into a function to be shared by both if you'd like?

210

I went ahead and added this but I couldn't think of a way to get a result like this without enabling this for MemberExprs, so I went ahead and re-enabled for them since I think it could be useful for them. LMK if I should revert that change. I guess if we don't want to support MemberExprs I should disable the ObjC property support since it probably makes to match the C++ equivalent?

dgoldman updated this revision to Diff 425650.Apr 27 2022, 4:51 PM
  • Remove extraneous computeExprType declaration

Ok, this looks pretty good to me! Just nits

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
54

just QualType, it's internally nullable already
(some value in being explicit, but the code below handling two different null values is more confusing imo)

And this is more clearly VarType - it's the type you intend for the variable, and it's not necessarily the type of the extracted expression (see pseudo-object stuff)

120

sorry, please do pull this back out into a function

(my question was just can this be a function rather than a method, it seemed self-contained)

122

If you set ExprType to Ctx.getAutoDeductType(), I think you can avoid treating this as a special case when printing

124

nit: ExprType->getTypePtrOrNull() as a boolean is more directly !ExprType->isNull()

198

it's similar to the cast you have, but in addition to handling foo.bar it handles parens like (foo.bar), commas like (0, foo.bar) and casts. All together I think this covers more cases where a pseudo-object can be used as an lvalue and potentially modified.

210

Try:

int (*foo(char))(int);
void bar() {
  (void)foo('c');
}

extracting foo('c') to subexpr should yield int (*placeholder)(int) = foo('c');

374

The reason MemberExpr is banned is that we don't want to suggest extracting foo if it happens to be a member.
You can leave it banned, or allow it but disallow the case where the MemberExpr's base is a CXXThisExpr which is implicit.

402

I'd just change this to if (!ExprType || ExprType->isVoidType())

I don't think there's a real null-type case that does anything useful.

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
340

This comment is inaccurate: if the expression has type NSInteger, that code will be written.

The issue is that the type of the expression isn't NSInteger, it's just int. arithmetic doesn't produce typedef types in the way I think you're expecting.

If the expression was something that was an NSInteger (like a call to a function whose declared return type is NSInteger) then that type would be preserved.

dgoldman updated this revision to Diff 425914.Apr 28 2022, 3:23 PM
dgoldman marked 6 inline comments as done.
  • Extract var type handling to a function
  • Other fixes for review
dgoldman marked an inline comment as done.Apr 28 2022, 3:32 PM
dgoldman added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
198

Gotcha, thanks. That makes sense, but it doesn't look like that has any usages and I'm not sure if it's safe to call from this context (e.g. if we can have a non ObjC property PseudoObject expr). I guess I could modify getObjCProperty to return nullptr in those invalid cases, WDYT?

210

Thanks, that worked

374

Went with the latter, but LMK. It does seem odd to me that the same expression is allowed if it's explicit - maybe we should just allow always, even if it's implicit? Xcode allows it, FWIW.

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
340

Gotcha, thanks. Deleted the comment, should I delete this test case as well?

dgoldman marked an inline comment as done.May 6 2022, 7:30 AM

Friendly ping

sammccall accepted this revision.May 19 2022, 12:17 AM

Thank you!

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
418

oops, I forgot one detail: we want ME->getBase()->IgnoreImpCasts()

(in void nonConstMethod() { constMethod(); } there's an ImplicitCastExpr in there...

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
326

can you move the function pointer + NSInteger tests into a separate block with ExtraArgs = {"-xc"}?

340

Up to you, I think the test is fine.
I'd add a comment "arithmetic on typedef types yields plain integer types" or so to explain what it's about.

This revision is now accepted and ready to land.May 19 2022, 12:17 AM
dgoldman added inline comments.May 20 2022, 9:03 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
198

@sammccall WDYT about this? Leave as is?

dgoldman added inline comments.May 20 2022, 9:33 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
418

Hmm, is this right, I tested out that example and it seems like that's actually a CXXMemberCallExpr which this doesn't handle?

sammccall added inline comments.May 20 2022, 9:52 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
198

Yeah, that function is not ideal. Leaving as-is sounds fine to me.

418

The callee of the CXXMemberCallExpr is the MemberExpr i'm talking about here.

dgoldman added inline comments.May 20 2022, 12:39 PM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
418

Right but this code here doesn't handle CXXMemberCallExpr, only MemberExpr - so we don't even see the callee here. If I add the following above:

if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(E))
  E = MCE->getCallee();

and do ME->getBase()->IgnoreImpCasts() then:

class Test {
  int constMethod() const { return 1; }
  int nonConstMethod() { return [[constMethod()]]; }
};

will be unavailable.

Please go ahead and land this, the remaining open comment doesn't really matter.

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
418

Right but this code here doesn't handle CXXMemberCallExpr, only MemberExpr - so we don't even see the callee here.

I don't understand what you mean.

If you select int nonConstMethod() { return [[constMethod]](); }, then isn't N such a MemberExpr?

dgoldman added inline comments.May 31 2022, 8:09 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
418

Nope, still seems to be CXXMemberCallExpr. I'll go ahead and land this with ME->getBase()->IgnoreImpCasts() check although the example isn't working as expected.

This revision was automatically updated to reflect the committed changes.