Page MenuHomePhabricator

[clang-tidy] Detect and fix calls to grand-...parent virtual methods instead of calls to parent's virtual methods
ClosedPublic

Authored by zinovy.nis on Mar 9 2018, 2:15 AM.

Details

Summary

Warns if one calls grand-..parent's virtual method in child's virtual method instead of overridden parent's. Can automatically fix such cases by retargeting calls to correct class.

class A {
...
int virtual foo() {...}
}

class B: public A {
int foo() override {...}
}

class C: public B {
...
int foo() override {... A::foo()...} // warning: qualified name A::foo refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
}

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zinovy.nis marked 10 inline comments as done.Mar 15 2018, 12:16 PM
zinovy.nis added inline comments.
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
129

"is not defined by a direct base class" is not a correct proposal. Issues that this check finds are all about calling a grand-parent's method instead of parent's whether base class defines this method or not.

How about

Qualified function name %0 refers to a function not from a direct base class; did you mean %1?

?

zinovy.nis marked 6 inline comments as done.
zinovy.nis added inline comments.Mar 15 2018, 12:28 PM
test/clang-tidy/bugprone-parent-virtual-call.cpp
128

Please see // Test virtual method is diagnosted although not overridden in parent. section in the test file.

aaron.ballman added inline comments.Mar 16 2018, 5:41 AM
test/clang-tidy/bugprone-parent-virtual-call.cpp
106

Typo: diagnosted -> diagnosed

114

This seems like a false positive to me. Yes, the virtual function is technically exposed in BI, but why is the programmer obligated to call that one rather than the one from A, which is written in the source?

zinovy.nis added inline comments.Mar 16 2018, 5:55 AM
test/clang-tidy/bugprone-parent-virtual-call.cpp
106

oops, thanks.

114

IMHO it's a matter of safety. Today virt_1() is not overridden in BI, but tomorrow someone will implement BI::virt_1() and it will silently lead to bugs or whatever.

aaron.ballman added inline comments.Mar 16 2018, 6:00 AM
test/clang-tidy/bugprone-parent-virtual-call.cpp
114

If tomorrow someone implements BI::virt_1(), then the check will start diagnosing at that point.

zinovy.nis added inline comments.Mar 16 2018, 6:07 AM
test/clang-tidy/bugprone-parent-virtual-call.cpp
114

Correct, but anyway I don't think it's a problem.

aaron.ballman added inline comments.Mar 16 2018, 6:18 AM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
108–109

You can get rid of this and just pass in the Decl * directly while using %q in the format string.

129

Not keen on "from", but we could use "qualified function name %q0 refers to a function that is not explicitly declared by a direct base class; did you mean %1?"

test/clang-tidy/bugprone-parent-virtual-call.cpp
114

I'd prefer to see this changed to not diagnose. I don't relish the idea of explaining why that code diagnoses as it stands.

zinovy.nis edited the summary of this revision. (Show Details)
zinovy.nis marked 5 inline comments as done.Mar 18 2018, 10:20 AM
zinovy.nis added inline comments.
test/clang-tidy/bugprone-parent-virtual-call.cpp
114

Done. Now the check looks for a most recent class with the overridden method. Please review this change.

zinovy.nis marked an inline comment as done.Mar 18 2018, 10:32 AM
zinovy.nis marked 6 inline comments as done.Mar 18 2018, 10:35 AM
zinovy.nis added inline comments.
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
129

Changed to

qualified name %0 refers to a member which was overridden in subclass%1"

zinovy.nis marked 2 inline comments as done.Mar 19 2018, 12:26 AM

Hope I satisfied all your requests now.

alexfh added inline comments.Mar 20 2018, 7:59 AM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
25

http://llvm.org/docs/CodingStandards.html says: "Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())."

29

Is Parent the only capture? I'd prefer it to be explicit instead of automatic.

aaron.ballman added inline comments.Mar 20 2018, 10:37 AM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
40

Local variables should be capitalized.

41–42

Please add the const qualifier to both of these.

43–44

Do not use auto here.

50–54

Do not use auto here as the type is not explicitly spelled out in the initialization. Also, please do not name the local variable Type as 1) it seem to be a declaration, and 2) Type is a type name.

55

Please add a message to your asserts so that if they trigger, the user gets something more helpful as the diagnostic. Same elsewhere.

90

Please put appropriate explicit qualifiers on all of the auto uses (here and elsewhere).

94

Instead of dyn_cast followed by assert(), you should just use cast<>, but only if you're certain you cannot get anything other than a CXXMethodDecl out of the expression.

99

Do not use auto here.

129

Please use %q0 rather than calling your custom qualified name function. The diagnostics engine will handle printing the qualified name appropriately for you.

docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
7

This formulation is a bit odd because of the grand-...parent part. I would reword to be more along the lines of: detects calls to qualified function names that refer to a function that is not explicitly declared by a direct base class.

Aaron, I applied the changes you suggest.

I also found and fixed a new case when grandparent method is called via 'typedef'ed or 'using' type. There's also a new test (class C2) for it.

zinovy.nis edited the summary of this revision. (Show Details)Mar 22 2018, 12:53 PM
zinovy.nis marked an inline comment as done.

camelCase last minute fix.

zinovy.nis marked 9 inline comments as done.Mar 22 2018, 1:02 PM
zinovy.nis added inline comments.
docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
7
that is not explicitly declared by a direct base class.

In fact, it detects calls to qualified function names

that explicitly overridden in subclasses.

Eugene.Zelenko added inline comments.Mar 22 2018, 1:05 PM
docs/ReleaseNotes.rst
91

Please rebase from trunk and use :doc: for link.

Please add a test where the parent class is in a differently named namespace.

alexfh added inline comments.Mar 23 2018, 7:43 AM
docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
27

The formatting looks weird. Could you move warning: .* to the start of the next line and put the whole message on the same line?

zinovy.nis marked 5 inline comments as done.Mar 23 2018, 12:59 PM
zinovy.nis marked 4 inline comments as done.Mar 23 2018, 1:07 PM
  • Rebased.
  • Added 1 more test for namespaced base clases
  • Fixed minor issues.
alexfh requested changes to this revision.Mar 26 2018, 8:54 AM
alexfh added inline comments.
docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
27

Could you address the "and put the whole message on the same line" part as well?

Please also put // before warning: and ...s to make the example look more like syntactically valid code.

This revision now requires changes to proceed.Mar 26 2018, 8:54 AM
zinovy.nis retitled this revision from [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods to [clang-tidy] Detect and fix calls to grand-...parent virtual methods instead of calls to parent's virtual methods.
zinovy.nis retitled this revision from [clang-tidy] Detect and fix calls to grand-...parent virtual methods instead of calls to parent's virtual methods to [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods.
  • Beautify bugprone-parent-virtual-call.rst
zinovy.nis marked an inline comment as done.Mar 26 2018, 9:16 AM
  • Updated warning message to

warning: qualified name 'A::foo' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]

zinovy.nis edited the summary of this revision. (Show Details)Mar 28 2018, 1:33 AM
alexfh requested changes to this revision.Mar 28 2018, 5:36 PM

A few more nits.

clang-tidy/bugprone/ParentVirtualCallCheck.cpp
115–118

I suggest using 1. range for loop; 2. std::string::append instead of operator+/operator+=. Not that performance is extremely important here, but it's also cleaner, IMO:

std::string ParentsStr = "'";
for (const CXXRecordDecl *Parent : Parents) {
  if (ParentsStr.size() > 1)
    ParentsStr.append("' or '");
  ParentsStr.append(getNameAsString(Parent));
}
ParentsStr.append("'");
127

Please remove the parentheses.

test/clang-tidy/bugprone-parent-virtual-call.cpp
33

Specifying each unique message completely once should be enough to detect typos in it (e.g. missing or extra spaces or quotes around placeholders). In other messages let's omit repeated static parts to make the test a bit more easier to read, e.g.:

// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean 'B'?
This revision now requires changes to proceed.Mar 28 2018, 5:36 PM
zinovy.nis marked 3 inline comments as done.
  • Cosmetic fixes.

BTW, I've recently found few dozens of issues in the Chromium code with my check.
For ex.:

browser/src/cc/layers/nine_patch_layer_impl.cc:89:51: warning: qualified name 'cc::LayerImpl::LayerAsJson' refers to a member overridden in subclass; did you mean 'cc::UIResourceLayerImpl'? [bugprone-parent-virtual-call]
  std::unique_ptr<base::DictionaryValue> result = LayerImpl::LayerAsJson();
                                                  ^~~~~~~~~~~~~~~~~~~~~~
                                                  cc::UIResourceLayerImpl::
zinovy.nis updated this revision to Diff 140579.Apr 1 2018, 7:35 AM
  • Minor cosmetic fix: use lexical representation instead of semanic for printing callee string. It helps to print class names hidden behind 'typedef' and 'using':
using A1 = A;
typedef A A2;

class C2 : public B {
public:
  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
  // BEFORE: warning: qualified name 'A::virt_1'...
  // BEFORE: warning: qualified name 'A::virt_1'...

  //  AFTER: warning: qualified name 'A1::virt_1'...
  //  AFTER: warning: qualified name 'A2::virt_1'...
};
alexfh requested changes to this revision.Apr 4 2018, 9:28 AM
alexfh added inline comments.
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
74

That thread doesn't seem to be particularly useful. Lexer::getSourceText(CharSourceRange::getTokenRange(E.getSourceRange()), SM, CorrectLangOpts, 0); (or clang::tooling::fixit::getText) should work well in macro-free cases. When there are macros, Lexer::makeFileCharRange may help to find a contiguous range that corresponds to a SourceRange (and when it doesn't, this usually means that there's no such text range).

Both of these don't remove whitespace, but why would you do that anyway? If really needed, this can be done outside.

78

Any reason to not use the actual LangOptions? Substituting it with the default may result in incorrect handling of certain tokens.

80

Consider using llvm::remove_if.

81

Text.at(Text.size() - 1) seems to be the same as Text.back(). And what does this condition do, btw?

82–84

That seems to be a bad thing to fall back to - it uses getCharRange which will drop the last token.

145–147

Please remove the commented out code.

test/clang-tidy/bugprone-parent-virtual-call.cpp
33

I'd also remove the [bugprone-parent-virtual-call] part from repeated CHECK-patterns.

This revision now requires changes to proceed.Apr 4 2018, 9:28 AM
  • Switched to use 'tooling::fixit::getText' for qualified names.
  • Shortened test code with {{.*}}.
zinovy.nis marked 6 inline comments as done.Apr 5 2018, 12:43 PM
zinovy.nis marked an inline comment as done.
zinovy.nis retitled this revision from [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods to [clang-tidy] Detect and fix calls to grand-...parent virtual methods instead of calls to parent's virtual methods.Apr 6 2018, 4:44 AM
This revision is now accepted and ready to land.Apr 6 2018, 8:14 AM

Alexander, thanks for your patience and comments :-)

This revision was automatically updated to reflect the committed changes.