1192130 - Add MOZ_NON_AUTOABLE to restrict using auto in place of specific types

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

The following pattern is a memory leak since already_AddRef'ed destructor doesn't drop the reference:

already_AddRefed<T> Foo();

auto obj = Foo();

We should add a MOZ_NON_AUTOABLE attribute which would apply to already_AddRefed and similar, so that we can detect these cases and convert them into compile time errors.

Comment 1

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

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #0)
> The following pattern is a memory leak since already_AddRef'ed destructor
> doesn't drop the reference:
> 
> already_AddRefed<T> Foo();
> 
> auto obj = Foo();
> 
> We should add a MOZ_NON_AUTOABLE attribute which would apply to
> already_AddRefed and similar, so that we can detect these cases and convert
> them into compile time errors.

This should be pretty trivial to implement, I'll get something working tomorrow hopefully.

Comment 2

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

Attachment: Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

Comment 3

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

Attachment: Part 2: Use MOZ_NON_AUTOABLE to validate the usage of already_AddRefed

Comment 4

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

Attachment: Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

Oops - I didn't include code for `auto &` and `const auto &` among others :S.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3a3a8fe7eb

Comment 5

[(no longer active)]

Comment on attachment 8646960 [details] [diff] [review]
Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

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

r=me with the below fixed.

::: build/clang-plugin/clang-plugin.cpp
@@ +1367,5 @@
> +  const VarDecl *D = Result.Nodes.getNodeAs<VarDecl>("node");
> +
> +  if (const AutoType *T = D->getType()->getContainedAutoType()) {
> +    if (const CXXRecordDecl *Rec = T->getAsCXXRecordDecl()) {
> +      if (MozChecker::hasCustomAnnotation(Rec, "moz_non_autoable")) {

Couldn't you use autoType() and hasDeducedType() AST matchers instead of doing the checks programatically?  If not, this is fine too.

::: build/clang-plugin/tests/TestNoAutoType.cpp
@@ +1,3 @@
> +#define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable")))
> +
> +struct MOZ_NON_AUTOABLE ExplicitType {};

Please also add a test where this annotation is applied to a template class.

@@ +6,5 @@
> +void f() {
> +  ExplicitType a;
> +  auto b = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}}
> +  auto &br = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}}
> +  const auto &brc = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}}

What about auto* and const auto*?

Comment 6

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

(In reply to Ehsan Akhgari (don't ask for review please) from comment #5)
> Couldn't you use autoType() and hasDeducedType() AST matchers instead of
> doing the checks programatically?  If not, this is fine too.

I could do the match
autoType(hasDeducedType(nonAutoableType())), but that would unfortunately not give me enough information to be able to give good line numbers (as I won't have the VarDecl).

If I decided to include the vardecl, unfortunately varDecl(hasType(autoType(..))) isn't the same as getContainedAutoType, as it doesn't strip away the & and const qualifiers, so I would have to separately match against each of those :-/. 

I think it makes the most sense to just do these checks programmatically. I think that this problem is a bit awkward to shoehorn into the matcher system, even though it is possible. If you think it's worth it, I can move it into the matcher system though.

Comment 7

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

(In reply to Michael Layzell [:mystor] from comment #6)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #5)
> > Couldn't you use autoType() and hasDeducedType() AST matchers instead of
> > doing the checks programatically?  If not, this is fine too.
> 
> I could do the match
> autoType(hasDeducedType(nonAutoableType())), but that would unfortunately
> not give me enough information to be able to give good line numbers (as I
> won't have the VarDecl).
> 
> If I decided to include the vardecl, unfortunately
> varDecl(hasType(autoType(..))) isn't the same as getContainedAutoType, as it
> doesn't strip away the & and const qualifiers, so I would have to separately
> match against each of those :-/. 
> 
> I think it makes the most sense to just do these checks programmatically. I
> think that this problem is a bit awkward to shoehorn into the matcher
> system, even though it is possible. If you think it's worth it, I can move
> it into the matcher system though.

Actually, I think I'll just add an `autoNonAutoableType()` matcher which uses getContainedAutoType internally, and performs the check there, that way I can move the logic into the matchers. Is that OK with you Ehsan?

Comment 8

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

Attachment: Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types

Updated patch

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4eba03ae3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f595614207f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a4eba03ae3e
https://hg.mozilla.org/mozilla-central/rev/0f595614207f