1307170 - Reject clang static analysis attributes in invalid locations

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

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

We have a bunch of annotations which we have added for the clang static analysis, and it would be good to make sure that people are using them correctly, and not placing them in places where they would not be recognized by the analysis. We should error out if they are placed in the wrong places.

Comment 1

[Andi [:andi]]

Attachment: 316155.patch

This is to restrict the usage of MOZ_REQUIRED_BASE_METHOD only for CXXMethodDecl that is also virtual. I'm not sure that we should also enforce it that the parent CXXRecordDecl should be base.

Comment 2

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

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

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

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

I feel like we should at least add some comment somewhere here which explains that this is a companion checker, and is just used to ensure that we don't include the annotation in places it would be ignored.

@@ +186,4 @@
>    RefCountedCopyConstructorChecker RefCountedCopyConstructor;
>    AssertAssignmentChecker AssertAttribution;
>    KungFuDeathGripChecker KungFuDeathGrip;
> +  OverrideBaseCallAnnotationChecker OverrideBaseCallAnnotation;

I'm not sure that I like this name. Perhaps OverrideBaseCallUsageChecker to signify that we are validating the usage of this attribute?

@@ +875,5 @@
>  AST_MATCHER(QualType, isRefPtr) {
>    return typeIsRefPtr(Node);
>  }
> +
> +AST_MATCHER(CXXMethodDecl, hasBaseCallAnnotationForNonVirtual) {

This checker feels like it's doing too much, can we have a isNotVirtual and the isRequiredBaseMethod checker separately?

Comment 3

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

Another note, I feel like this bug should be a metabug, can we move this patch onto another bug which blocks this one?