Page MenuHomePhabricator

[Clangd] Fixed ExtractVariable for MemberExprs and Assignment Exprs
ClosedPublic

Authored by SureYeaah on Jul 15 2019, 5:35 AM.

Diff Detail

Event Timeline

SureYeaah created this revision.Jul 15 2019, 5:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 5:35 AM
SureYeaah updated this revision to Diff 209809.Jul 15 2019, 5:42 AM

Fixed comment

kadircet added inline comments.Jul 15 2019, 5:53 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
80–81

nit: Could you reduce nesting by early returns and add braces to outer if statements if there are any nested statements left.

clang-tools-extra/clangd/unittests/TweakTests.cpp
304

left out this one ?

307

how come these changes part of that patch?

is it possible that this tweak was changed to not trigger on empty selections, but tests were not updated? If that's the case could you please send these on a different patch and rebase this patch on that one?

307

how come these changes part of that patch?

how come these changes *are* part of that patch?

SureYeaah updated this revision to Diff 209844.Jul 15 2019, 6:29 AM
SureYeaah marked 5 inline comments as done.

Removed unrelated changes

SureYeaah added inline comments.Jul 15 2019, 6:29 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
304

Since that's an IntegerLiteral, this patch doesn't apply to it?

307

This patch doesn't involve empty selection triggers. Reverted these changes.

SureYeaah edited the summary of this revision. (Show Details)Jul 15 2019, 6:35 AM

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

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

this comment repeats the implementation, but should explain why instead

kadircet added inline comments.Jul 15 2019, 6:50 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
304

nvm this one, it was for "empyt selection triggering" case.

328–329

I thought extractor didn't handle this case(missing braces).

What is the extraction in this case? Because if it is

auto dummy = a++;
while (a < 1)
   dummy++;

it is not going to be semantically same.

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

SureYeaah marked an inline comment as done.Jul 15 2019, 6:58 AM
SureYeaah added inline comments.
clang-tools-extra/clangd/unittests/TweakTests.cpp
328–329

We don't check for missing braces and continuing traversing up the AST until we find the CompoundStmt.

Yes, it's not going to be semantically the same. We only check if after extraction, the extracted expression will reference a variable that hasn't been declared till that point.

kadircet added inline comments.Jul 15 2019, 7:05 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
328–329

I believe this behavior is not expected and we should address that as well, the expressions you extract might have weird side effects, therefore if some expression is within a loop body it should stay there.
No need to address in this patch though, could you (leave a fixme || create a bug) to make sure extraction of such expressions doesn't go to outer scope, but rather stays within the loop body?

SureYeaah updated this revision to Diff 209853.Jul 15 2019, 7:15 AM
SureYeaah marked 2 inline comments as done.

Added comments

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

I agree, but this patch doesn't do that. (I think?)
Instead it bans extraction entirely in this case.

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

I agree, but this patch doesn't do that. (I think?)
Instead it bans extraction entirely in this case.

Yes it doesn't. Our current model doesn't extend the user selection and as such we only check the commonAncestor of whatever the user has selected. Should this be changed then?

Are you sure we want to disable extraction here, rather than just do the extraction at a higher level?

E.g. if bar(1,2,3, f[[o]]o(4,5)); seems like it should extract the call too foo(4,5), not fail to trigger entirely.

Selecting f[[o]]o(4,5) will just extract the foo which would be a DeclRefExpr. We want to extract the CallExpr for which we would need to select the brackets as well.

I agree, but this patch doesn't do that. (I think?)
Instead it bans extraction entirely in this case.

Yes it doesn't. Our current model doesn't extend the user selection and as such we only check the commonAncestor of whatever the user has selected. Should this be changed then?

Yes I think you want to walk up the tree like you do for finding the target stmt, giving up when you hit a non-expr.

This didn't matter prior to this patch as void subexprs are extremely rare. But declrefs are not!

Added fix for selecting the callExpr of a MemberExpr/Function DeclRefExpr

SureYeaah updated this revision to Diff 210045.Jul 16 2019, 1:49 AM

Fixed wrong function name

SureYeaah updated this revision to Diff 210047.Jul 16 2019, 2:01 AM

Fixed crash

kadircet added inline comments.Jul 18 2019, 12:54 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
198

comment seems to be repeating the code, instead lets comment about the assumptions, like not working on declrefexpr's etc.

214

maybe instead of checking internals like Target just make computeExtractionContext return an llvm::Error and check for success?

233

I don't think traversing the tree without any conditions is a good idea, for example:

struct Foo {
int bar(int);
int z;
};

void foo() {
Foo f;
f.bar([[f.z]]);
}

above selection is on a memberexpr, that has a parent membercallexpr but it is actually not related to selection at all but I believe we'll extract the whole thing in that case.
I don't think that is the intended behavior, could you also add a test case for that?

275

nit: braces

278

nit: instead of checking success check failure and bail out?

if(!TargetNode || !canBeass...)
  return;
SureYeaah marked 5 inline comments as done.Jul 18 2019, 6:25 AM
SureYeaah added inline comments.
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
214

Should it instead return a bool since there's actually no error?

SureYeaah updated this revision to Diff 210545.Jul 18 2019, 6:28 AM

Refactored code

kadircet added inline comments.Jul 18 2019, 6:47 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
214

Yes that's also plausible

296

return Target->InsertionPoint?

clang-tools-extra/clangd/unittests/TweakTests.cpp
304

another case on bar ?

[[[[t.b[[a]]r]](t.z)]]
350

also on t:

return [[t]].bar([[[[t]].z]]);
SureYeaah updated this revision to Diff 210560.Jul 18 2019, 7:15 AM
SureYeaah marked 4 inline comments as done.

Fixed tests

SureYeaah added inline comments.Jul 18 2019, 7:17 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
296

Changed to checking if target is extractable.

SureYeaah updated this revision to Diff 210590.Jul 18 2019, 9:01 AM

Merged with trigger on non empty selection only

kadircet added inline comments.Jul 19 2019, 4:21 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
214

i was trying to say:

having a function named computeExtractionContext that will require you to check some other state even when it returns true sounds like a bad idea.

296

any reasons for not returning Target->isExtractable() ?

clang-tools-extra/clangd/unittests/TweakTests.cpp
304

i suppose this is done

457

I think auto dummy = 1; should be within the label. otherwise we might break codes like(which is basically anything with labels):

void foo() {
goto label;
label:
 a = 1;
}

I don't think it is that important though, and have no idea about the effort necessary feel free to just add a FIXME if it turns out hard.

SureYeaah updated this revision to Diff 210800.Jul 19 2019, 4:52 AM
SureYeaah marked 5 inline comments as done.

Minor changes and disabled extraction from label statement

kadircet accepted this revision.Jul 19 2019, 4:57 AM

Thanks, LGTM from my side. Do you have any concerns @sammccall ?

clang-tools-extra/clangd/unittests/TweakTests.cpp
457

could you keep the previous test case within a checkNotAvailable block ?

This revision is now accepted and ready to land.Jul 19 2019, 4:57 AM
SureYeaah updated this revision to Diff 210803.Jul 19 2019, 5:14 AM
SureYeaah marked an inline comment as done.

Added test for label

SureYeaah added inline comments.Jul 19 2019, 5:25 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
457

I've disabled extraction from label statements and added a fixme because the fix for all kinds of loops as well as if statement needs another patch.

This revision was automatically updated to reflect the committed changes.