This is an archive of the discontinued LLVM Phabricator instance.

[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

zinovy.nis created this revision.Mar 9 2018, 2:15 AM
zinovy.nis edited the summary of this revision. (Show Details)Mar 9 2018, 2:25 AM
malcolm.parsons added inline comments.
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
153

What does this do for templated parent classes?

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

FIXME.

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

typo: choice

lebedev.ri retitled this revision from Detects and fixes 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.Mar 9 2018, 9:26 AM
zinovy.nis updated this revision to Diff 137783.
zinovy.nis added inline comments.Mar 9 2018, 10:02 AM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
153

Please see my last test case in updated revision.

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

Oops. Thanks!

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

Thanks!

Eugene.Zelenko added inline comments.
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
27

Please make this function static and remove anonymous namespace.

41

Please make this function static and remove anonymous namespace.

49

Please make this function static and remove anonymous namespace.

67

Please make this function static and remove anonymous namespace.

92

Please remove this line.

docs/ReleaseNotes.rst
88

Please make description here and first statement in documentation same.

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

Please add .. code-block:: c++

clang-tidy/bugprone/ParentVirtualCallCheck.cpp
153

Also, a templated parent class of a templated class.

template <class T>
class DF : public BF<T> {
public:
  int virt_1() override { return A::virt_1(); }
  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'BF<T>'?
  // CHECK-FIXES:  int virt_1() override { return BF<T>::virt_1(); }
};
Eugene.Zelenko added inline comments.Mar 9 2018, 1:53 PM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
13

Please run Clang-format and remove empty line between headers.

zinovy.nis marked 10 inline comments as done.
test/clang-tidy/bugprone-parent-virtual-call.cpp
126

Does this fixit compile?

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.
test/clang-tidy/bugprone-parent-virtual-call.cpp
126

Thanks for pointing. Hope I fixed this case in the latest patch. Please have a look at it.

zinovy.nis updated this revision to Diff 137953.
zinovy.nis marked an inline comment as done.
Eugene.Zelenko added inline comments.Mar 11 2018, 4:46 PM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
48

Please remove empty line.

117

No need for brackets.

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

There isn't an F in scope.
The base class isn't a dependent type here, so no template params are needed.

zinovy.nis marked an inline comment as done and an inline comment as not done.
zinovy.nis added inline comments.
test/clang-tidy/bugprone-parent-virtual-call.cpp
116

Finally I decided not to propose a fix for templated parent.

zinovy.nis marked an inline comment as done.Mar 13 2018, 1:45 PM
zinovy.nis marked 4 inline comments as done.
aaron.ballman added inline comments.Mar 13 2018, 5:38 PM
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
25–26

You can drop these asserts.

29–30

You can use llvm::find_if() for the range-based API instead.

31–33

Factor out Base.getType()->getAs() and use the pointer in both the assert and the comparison.

40–41

And these asserts as well.

I don't think this function adds much value given that it's a one-liner; I'd hoist its functionality out to the caller.

57–61

This should be getNameAsString()

102–104

Convert this to an else if and elide the braces.

113

Do not use auto here as the type is not spelled out in the intialization.

129

The diagnostic should not contain punctuation aside from the question mark.

Also, the diagnostic uses some novel terminology in "grandparent's method". How about: "qualified function name %0 refers to a function that is not defined by a direct base class; did you mean %1?"

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

What should happen in this case?

struct Base {
  virtual void f() {}
};

struct Intermediary : Base {
};

struct Derived : Intermediary {
  void f() override {
    Base::f(); // I would expect this not to diagnose
  }
};

This would be a reasonable test case to add.

alexfh requested changes to this revision.Mar 14 2018, 9:29 AM
alexfh added inline comments.
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
25–26

... and make arguments const references ;)

45

std::list is almost never a good choice due to large overhead, poor locality of the data, etc. Use std::vector instead.

54

Pull this to a variable to avoid repetition.

This revision now requires changes to proceed.Mar 14 2018, 9:29 AM
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
85

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
114–117

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("'");
126

Please remove the parentheses.

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

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
32

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.