1187486 - clang-plugin's implicit constructor analysis misses templated constructors

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

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

If we have a class with a templated single argument constructor, like

  template <typename I>
  nsRefPtr(already_AddRefed<I>& aSmartPtr)
    : mRawPtr(aSmartPtr.take())
    // construct from |already_AddRefed|
  {
  }

the clang-plugin currently doesn't complain about the lack of an `explicit` or `MOZ_IMPLICIT` annotation, in spite of the constructor being an implicit constructor. 

I think that this is because these items are listed in the definition of the struct as FunctionTemplateDecl rather than CXXConstructorDecl, which means that our analysis misses them. We should be able to get around that by also checking for FunctionTemplateDecls where the template declaration is a CXXConstructorDecl.

In addition, I believe that this could be avoided by using Matchers rather than the current strategy of using the RecursiveASTVisitor, as the RecursiveASTVisitor, by default, doesn't enter template instantiations, and if we just directly match CXXConstructorDecls, we will match those produced by template instantiations as well.

Comment 1

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

Attachment: Update the clang plugin to detect templated implicit constructors

We can't land this yet because there are loads of violating code in the codebase right now which need to be fixed first, but this is the change to the plugin to get started.

We should open dependent issues for each of the constructors we discover this way, but I don't have time to do that right now.

Comment 2

[(no longer active)]

Comment on attachment 8638797 [details] [diff] [review]
Update the clang plugin to detect templated implicit constructors

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

::: build/clang-plugin/clang-plugin.cpp
@@ +746,5 @@
> +    // We don't want deleted constructors.
> +    !Node.isDeleted();
> +}
> +
> +AST_MATCHER(CXXConstructorDecl, isNotExplicitlyImplicit) {

Please create a positive predicate here (isImplicit) and in the matcher code, use unless(isImplicit()).

Comment 3

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

Attachment: Annotate implicit templated constructors in MFBT

Comment 4

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

Attachment: Annotate implicit templated constructors in gfx

Comment 5

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

Attachment: Annotate implicit templated constructors in js

Comment 6

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

Attachment: Annotate implicit templated constructors in xpcom

Comment 7

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

Attachment: Annotate implicit templated constructors on ParentObject

Comment 8

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

Attachment: Annotate implicit templated constructor on ByteReader in libstagefright

Comment 9

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

Attachment: Annotate implicit templated constructor on Buffer

Comment 10

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

Attachment: Annotate implicit templated constructor on StackAllocator

Comment 11

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

Attachment: Annotate implicit templated constructor in Replay.cpp

Comment 12

[(no longer active)]

Comment on attachment 8638797 [details] [diff] [review]
Update the clang plugin to detect templated implicit constructors

Actually, as you can see from the change you had to make to JSCompartment, this analysis is not 100% correct, since it only needs to look at declarations in the body of the class, not definitions of constructors.

Comment 13

[(no longer active)]

Attachment: Check the canonical declaration of the constructor

This patch will help fix the issue I mentioned above.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e78fb4512a6

Comment 15

[(no longer active)]

I fixed up the patch to address my own comments and landed it for you.

Comment 16

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

(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #15)
> I fixed up the patch to address my own comments and landed it for you.

Thanks :)

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe5e8dc9bab

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d88da1f3c511

Comment 19

[Carsten Book [:Tomcat]]

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