1230311 - Add some annotation and static analysis to enforce that if a method is overridden also base method is called

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

This would have prevented bug 1230110 as an example.


Would be nice to have some MOZ_REQUIRED_BASE_METHOD or some such annotation, and static
analysis would then check that if method is overridden, the method in the base class is also called.

Comment 1

[Andrew McCreight [:mccr8]]

I think this also needs to handle multiple inheritance, where A is a subclass of B and C, which both are a subclass of D, and D defines a REQUIRED_BASE_METHOD.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

I don't see myself having time to work on this soon. Andi, are you interested?

Comment 3

[Andi [:andi]]

Sure i'll look other it after i'll be back from PTO, on monday 15th august.

Comment 4

[Andi [:andi]]

Can we have a more detailed example on this?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

bug 1230110 isn't clear enough?

Other example is SetAttr. Many HTML*Element implementations override the method from Element, yet they need to remember to call Element::SetAttr too. (well, I think they may call nsGenericHTMLElement::SetAttr or something which calls Element::SetAttr)

Comment 6

[Andi [:andi]]

Attachment: 316155.patch

Comment 7

[Andi [:andi]]

Attachment: add annotation to enforce that if a method is overridden also base method is called

MozReview-Commit-ID: AQ3Kx2qidU0

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8797134 [details] [diff] [review]
316155.patch

Review of attachment 8797134 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +163,4 @@
>      virtual void run(const MatchFinder::MatchResult &Result);
>    };
>  
> +  class OverrideBaseCallCheck : public MatchFinder::MatchCallback {

Usually these are called something more like OverrideBaseCallChecker, rather than Check.

@@ +879,5 @@
>  AST_MATCHER(QualType, isRefPtr) {
>    return typeIsRefPtr(Node);
>  }
> +
> +AST_MATCHER(CXXRecordDecl, doesClassInherits) {

I'm not the biggest fan of this function name, perhaps hasBaseClasses()? That way the matcher declaration would read like cxxRecordDecl(hasBaseClasses()).bind("class"), which I think is a bit more clear.

@@ +1855,4 @@
>    Diag.Report(E->getLocStart(), NoteID) << NoteThing << getNameChecked(D);
>  }
>  
> +bool DiagnosticsMatcher::OverrideBaseCallCheck::isSuitable(

I don't like this method name. Can we call it something more clear, like perhaps `isRequiredBaseMethod`?

@@ +1861,5 @@
> +}
> +
> +void DiagnosticsMatcher::OverrideBaseCallCheck::evaluateExpression(
> +    Stmt *StmtExpr, const CXXRecordDecl *ChildClass,
> +    std::list<const CXXMethodDecl*> &MethodList) {

I think you could make this method a lot simpler, I feel like something like this might work:

void checkStmt(Stmt *S, CXXRecordDecl *Class
         std::vector<const CXXMethodDecl*> &MethodList) {
  if (!MethodList.size()) {
    return;
  }

  if (CXXMethodCallExpr *Call = dyn_cast<CXXMethodCallExpr>(S)) {
    if (CXXMethodDecl *Method = dyn_cast_or_null<CXXMethodDecl>(Call->getDirectCallee())) {
      // We can only remove it if it is contained, so let's just try to remove it.
      MethodList.Remove(Method);
    }
  }

  for (Stmt *SS : S->children()) {
    checkStmt(SS, Class, MethodList);
  }
}

@@ +1922,5 @@
> +    CXXMethodDecl *Method = dyn_cast_or_null<CXXMethodDecl>(
> +        MemberFuncCall->getDirectCallee());
> +    if (!Method) {
> +      return;
> +    }

Do we not want to also check the arguments to this function?

@@ +1939,5 @@
> +    const MatchFinder::MatchResult &Result) {
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned OverrideBaseCallCheckID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Error,
> +      "Base method %0 is not called in overridden method from %1");

I feel like `Method BASE_CLASS::METHOD_NAME must be called in all overrides, but is not called in this override defined for class CHILD_CLASS` might be more clear (with the source location for the error message being located at the override.

@@ +1944,5 @@
> +  const CXXRecordDecl *Decl = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
> +
> +  if (!Decl) {
> +    return;
> +  }

When do you expect this to happen? We usually don't null-check the values which are guaranteed by the matcher.

@@ +1977,5 @@
> +
> +    // If list is not empty pop up errors
> +    for (auto BaseMethod : MethodsList) {
> +      Diag.Report(Method->getLocation(), OverrideBaseCallCheckID)
> +      << BaseMethod->getName() << Decl;

The indentation here is a bit weird.

::: build/clang-plugin/tests/TestOverrideBaseCall.cpp
@@ +1,5 @@
> +#define MOZ_REQUIRED_BASE_METHOD __attribute__((annotate("moz_required_base_method")))
> +
> +class Base {
> +public:
> +  virtual void fo() MOZ_REQUIRED_BASE_METHOD {

Please add tests for shadowing non-virtual methods as well. I believe we want those to work with this analysis as well, and IIRC they aren't listed in the overridden_methods() iterator. It will be a bit more tricky but I think it's important.

@@ +96,5 @@
> +      int a;
> +      a = Base::foRet();
> +      return 0;
> +  }
> +};
\ No newline at end of file

How about something like:

class A : public Base { int fo() { Base::fo(); } };
class B : public A { int fo() { A::fo(); } };

I think that should be allowed. We should assume that any method which overrides the must override method counts as a call to the method.

Can we also just have some tests for, say, overriding operators? I think it should work fine with your current implementation, but I want to be sure that it doesn't break when we have non-identifier named methods.

Comment 9

[Andi [:andi]]

After talking with Oli we could push in a later patch the functionality for non-virtual base functions since this scenario is very limited in our code and in practice this is somehow useless.
Also I'll push later today the patch for review and push another patch here in order to avoid the miss usage of this annotation on non-virtual base methods.

Comment 10

[Andi [:andi]]

Attachment: 316155.patch

Besides what we've discussed hope you don't mind but i still want to keep this portion of the core where we decide what functions are candidate for the list search:

>>      // Underlying object parent must be different than the current one
>>      if (Method->getParent() != ChildClass) {
>>        MethodList.remove(Method);
>>      }

I want to do this because list::remove has a complexity of O(N) where N is the number of pushed functions thus limiting the number of calls to remove has a better impact on our build time. Sure we don't expect our list to be long but still without that criteria each call to member methods will consequentially call list::remove, by having that if we limit the call to only member functions that are inherit.

Comment 11

[Andi [:andi]]

Attachment: 316155.patch

Needed to re-update this patch since a spell error was in the previous one.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8797145 [details] [diff] [review]
add annotation to enforce that if a method is overridden also base method is called

Review of attachment 8797145 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the wordsmithing changes below.

::: mfbt/Attributes.h
@@ +468,4 @@
>   *   use `auto` in place of this type in variable declarations.  This is intended to
>   *   be used with types which are intended to be implicitly constructed into other
>   *   other types before being assigned to variables.
> + * MOZ_REQUIRED_BASE_METHOD: Applies to class method declaration. Sometimes

Nit: "class method declarations".

@@ +468,5 @@
>   *   use `auto` in place of this type in variable declarations.  This is intended to
>   *   be used with types which are intended to be implicitly constructed into other
>   *   other types before being assigned to variables.
> + * MOZ_REQUIRED_BASE_METHOD: Applies to class method declaration. Sometimes
> + *  derivative classes override methods that need to be called by by their

Nit: we usually say "derived classes" instead of "derivative classes".

@@ +469,5 @@
>   *   be used with types which are intended to be implicitly constructed into other
>   *   other types before being assigned to variables.
> + * MOZ_REQUIRED_BASE_METHOD: Applies to class method declaration. Sometimes
> + *  derivative classes override methods that need to be called by by their
> + *  overridden counterparts. The marker is used to make the static analysis

How about "This marker indicates that the marked function..."?

@@ +470,5 @@
>   *   other types before being assigned to variables.
> + * MOZ_REQUIRED_BASE_METHOD: Applies to class method declaration. Sometimes
> + *  derivative classes override methods that need to be called by by their
> + *  overridden counterparts. The marker is used to make the static analysis
> + *  tool aware that the marked function must be called by the function that it

Nit: "method" instead of "function" in this sentence.

Comment 13

[Andi [:andi]]

Attachment: add annotation to enforce that if a method is overridden also base method is called

MozReview-Commit-ID: AQ3Kx2qidU0

Comment 14

[Andi [:andi]]

Comment on attachment 8797610 [details] [diff] [review]
add annotation to enforce that if a method is overridden also base method is called

Modified a bit the comment in order to be clear that this annotation should be used only for virtual methods.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8797610 [details] [diff] [review]
add annotation to enforce that if a method is overridden also base method is called

Review of attachment 8797610 [details] [diff] [review]:
-----------------------------------------------------------------

Good call on including the virtual function bit.  r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e34aad70c15
add annotation to enforce that if a method is overridden also base method is called. r=nfroyd

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8797586 [details] [diff] [review]
316155.patch

Review of attachment 8797586 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +168,5 @@
> +    virtual void run(const MatchFinder::MatchResult &Result);
> +  private:
> +    void evaluateExpression(Stmt *StmtExpr,
> +        const CXXRecordDecl *ChildClass,
> +        std::list<const CXXMethodDecl*> &MethodList);

I feel like a SmallVector would be a better type to use here, especially because most of the time there will be only one class here.

@@ +1867,5 @@
> +  if (!MethodList.size()) {
> +    return;
> +  }
> +
> +  if (auto MemberFuncCall = dyn_cast_or_null<CXXMemberCallExpr>(StmtExpr)) {

When is StmtExpr null?

@@ +1868,5 @@
> +    return;
> +  }
> +
> +  if (auto MemberFuncCall = dyn_cast_or_null<CXXMemberCallExpr>(StmtExpr)) {
> +    if(auto Method = dyn_cast_or_null<CXXMethodDecl>(

Space between the if and (

::: build/clang-plugin/tests/TestOverrideBaseCall.cpp
@@ +87,5 @@
> +public:
> +  void fo() {
> +    Base::fo();
> +  }
> +};

What is the difference between DerivIf and the other Deriv's here? They all seem identical?

@@ +133,5 @@
> +    BaseOperator::operator++();
> +    value++;
> +    return *this;
> +  }
> +};

How about:

class Deriv : public Base {
public:
  void fo() { Base::fo(); }
};

class DerivPrime : public Deriv {
public:
  void fo() { Deriv::fo(); }
};

Which shouldn't produce an error, while:

class Deriv : public Base {
public:
  void fo() { Base::fo(); }
};

class DerivPrime : public Deriv {
public:
  void fo() {}
};

should.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e34aad70c15

Comment 19

[Andi [:andi]]

(In reply to Michael Layzell [:mystor] from comment #17)
> How about:
> 
> class Deriv : public Base {
> public:
>   void fo() { Base::fo(); }
> };
> 
> class DerivPrime : public Deriv {
> public:
>   void fo() { Deriv::fo(); }
> };
> 
> Which shouldn't produce an error, while:
> 
> class Deriv : public Base {
> public:
>   void fo() { Base::fo(); }
> };
> 
> class DerivPrime : public Deriv {
> public:
>   void fo() {}
> };
> 
> should.

After the discussion on IRC from last night i agree we must have support for overridden functions that are not in the overridden_methods. My approach here is when adding to the list methods that meet our criteria to recursively crawl to all overriden_method from the overridden node and to push only nodes that have MOZ_REQUIRED_BASE_METHOD annotation.
The matching part will also be done recursively but without actually recursing through the bodies of the functions that are called from the main method, but more we recurse to the nodes that the caller nodes inherits and check them regarding our list. For this example:

>>class Base {
>>public:
>>  virtual void fo() MOZ_REQUIRED_BASE_METHOD {
>>  }
>>};
>>
>>class BaseTwo {
>public:
>>  virtual void fo() MOZ_REQUIRED_BASE_METHOD {
>>  }
>>};
>>
>>class A : public Base, public BaseTwo
>>{
>>public:
>>  virtual void fo() MOZ_REQUIRED_BASE_METHOD {
>>    Base::fo();
>>    BaseTwo::fo();
>>  }
>>};
>>
>>class Deriv : public A {
>>  void fo() {
>>    A::fo();
>>  }
>>};

For Deriv::fo the methods list will be: 
>>A::fo
>>Base::fo
>>BaseTwo::fo
And thus crawling through all methods called from Deriv::fo with their overriden_method will eliminate all of the items from the list.
What we don't do we don't actually check the validity of the inherited methods that override others since we are not inspect their bodies if they call their also inherited methods, but we base on the compiler that applies the same checker recursively on all class derivates.

Comment 20

[Andi [:andi]]

Attachment: Bug 1230311 - clang-plugin - static analysis to enforce that if a method is overridden also base method is called.

Review commit: https://reviewboard.mozilla.org/r/83662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83662/

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8798109 [details]
Bug 1230311 - clang-plugin - static analysis to enforce that if a method is overridden also base method is called.

https://reviewboard.mozilla.org/r/83662/#review82246

Looks good to me, thanks!

Comment 22

[Andi [:andi]]

Comment on attachment 8798109 [details]
Bug 1230311 - clang-plugin - static analysis to enforce that if a method is overridden also base method is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83662/diff/1-2/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2df8f3b2b014
clang-plugin - static analysis to enforce that if a method is overridden also base method is called. r=mystor

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4418ea65d1ce
Backed out changeset 2df8f3b2b014 for bustage on a CLOSED TREE

Comment 25

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4588587&repo=autoland

Comment 26

[Andi [:andi]]

this is (In reply to Carsten Book [:Tomcat] from comment #25)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=4588587&repo=autoland

This is something that's missing from clang 3.8.0 that is used on try to build, it lacks the implementation of range that was later added in clang 3.9.0

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdb34758652
clang-plugin - static analysis to enforce that if a method is overridden also base method is called. r=mystor

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dcdb34758652