Page MenuHomePhabricator

[clangd] Detect rename conflicits within enclosing scope
ClosedPublic

Authored by kbobyrev on Feb 2 2021, 10:08 PM.

Details

Summary

This patch allows detecting conflicts with variables defined in the current
CompoundStmt or If/While/For variable init statements.

Diff Detail

Event Timeline

kbobyrev created this revision.Feb 2 2021, 10:08 PM
kbobyrev requested review of this revision.Feb 2 2021, 10:08 PM

Thanks, this looks right to me.

clang-tools-extra/clangd/refactor/Rename.cpp
340

I found that the current structure (we have two places of this If cases, and they are separated) is a bit hard to follow, I'd suggest union them, just like the code below, it seems straight-forward to me, and easy to extend.

if (const auto* CompoundStmtEnclosing = Parents.begin()->get<CompoundStmt>()) {
   CheckCompoundStmt(CompoundStmtEnclosing);
   // below we detect conflicts in conditions or others (if the CompoundStmt is a function body, we could detect the parameter decls as well)
   getParents(CompoundStmtEnclosing);
   if ( ...<IfStmt>()) {
      CheckConditionVariable();
   } else if (...) {
   }
} else if (const auto* IfEnclosing = ... <IfStmt>()) {
   CheckCompoundStmt(If->getThen());
  // possibly CheckCompoundStmt(If->getElse)
} else {
  ...
}
341

thinking more about the if case, I think else should be included as well? no need to address in this patch.

like

if (int a = 0) {
} else {
  int s; // rename s=>a will cause a compiling error.
}
354

nit: VarDecl => NamedDecl

kbobyrev updated this revision to Diff 321050.Feb 3 2021, 4:22 AM
kbobyrev marked 3 inline comments as done.

Resolve review comments.

clang-tools-extra/clangd/refactor/Rename.cpp
341

This case is already supported: else's CompoundStmt is attached to the same IfStmt with variable declaration and is checked just like the "main" branch.

The other one wasn't (renaming a into s) but I've added this and also added tests for this.

kbobyrev updated this revision to Diff 321052.Feb 3 2021, 4:26 AM

Save few LOCs by checking for nullptr in CheckDeclStmt.

kbobyrev updated this revision to Diff 321053.Feb 3 2021, 4:28 AM

Don't spell out DynTypedNodeList and don't include ParentMapContext.h

kbobyrev updated this revision to Diff 321055.Feb 3 2021, 4:29 AM

Revert last change: leads to incomplete types.

hokein added a comment.Feb 3 2021, 4:54 AM

thanks, looks better now, just some nits to improve the code readability.

clang-tools-extra/clangd/refactor/Rename.cpp
332

since we repeat this code multiple times, I think we can use wrap it with a lambda (e.g. getSingleParent etc).

338

maybe name it LookupInDeclStmt and pass the Name as a parameter, the same below.

345

the same lookupInCompoundStmt.
nit: I would pass Stmt directly, and do the "compoundStmt" cast internally, to save some boilerplate cast in call sides :)

350

nit: can be just return CheckDeclStmt(DS);

355

nit: explicit spell out the auto type?

359

nit: the same here, return CheckDeclStmt (Scope->getConditionVariableDeclStmt()), just adding the nullptr handling in CheckDeclStmt.

364

I understand all cases of these if branches at the moment, but I think we'd better to add some comments, or examples here -- help us understand this code in case that we forget the context in the future :)

382

I think we need a return null here (or use if-else pattern), otherwise, the following if-stmt will be examined as well, which seems no necessary.

kbobyrev updated this revision to Diff 321086.Feb 3 2021, 6:35 AM
kbobyrev marked 6 inline comments as done.

Resolve most comments but one: some comments & examples in the code incoming in
the next diff.

kbobyrev added inline comments.Feb 3 2021, 6:45 AM
clang-tools-extra/clangd/refactor/Rename.cpp
355

That's the idea behind this helper: it's used both on IfStmt and WhileStmt, so the point here is that getConditionVariableDeclStmt() is available in both but not via interface/inheritance.

kbobyrev updated this revision to Diff 321095.Feb 3 2021, 7:00 AM
kbobyrev marked an inline comment as done.

Add comments.

hokein added inline comments.Feb 3 2021, 11:55 PM
clang-tools-extra/clangd/refactor/Rename.cpp
333

If the intention is for storage, maybe call it Storage.

334

const DynTypedNode &

362

I think we need to iterate *all* children, rather than the first one. looks like our testcase doesn't cover this, maybe add one.

372

CompoundStmt is the most common enclosing scope for function-local symbols.

378

maybe just GetSingleParent(Parent), rather than creating a new DynTypeNode.

390

be careful about nullptr here, the init-statement could be a nullptr, which will trigger a crash in dyn_cast, thinking about the case below:

for (; ; ;) {
  int var; 
}

use dyn_cast_or_null.

kbobyrev updated this revision to Diff 321334.Feb 4 2021, 12:30 AM
kbobyrev marked 5 inline comments as done and an inline comment as not done.

Fix a couple problems pointed out during the review.

clang-tools-extra/clangd/refactor/Rename.cpp
334

Sorry, I'm a bit confused: this can return nullptr when there are _no parents_. Is there any way I can return the reference safely?

kbobyrev updated this revision to Diff 321336.Feb 4 2021, 12:36 AM
kbobyrev marked an inline comment as done.

Pass const ref to DynTypedNode in helper.

hokein accepted this revision.Feb 4 2021, 12:40 AM

Looks good, thanks!

This revision is now accepted and ready to land.Feb 4 2021, 12:40 AM
hokein added inline comments.Feb 4 2021, 12:42 AM
clang-tools-extra/clangd/refactor/Rename.cpp
411

another case we could add is FunctionDecl, if we rename a parameter decl, we could check collisions in the function body. this can be addressed in a follow-up.

This revision was landed with ongoing or failed builds.Feb 4 2021, 12:46 AM
This revision was automatically updated to reflect the committed changes.