Add MOZ_NON_AUTOABLE to restrict using auto in place of specific types

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: Ehsan, Assigned: Nika)

Tracking

unspecified
mozilla43

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
(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.
Assignee: nobody → michael
Oops - I didn't include code for `auto &` and `const auto &` among others :S.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3a3a8fe7eb
Attachment #8646950 - Attachment is obsolete: true
Attachment #8646950 - Flags: review?(ehsan)
Attachment #8646960 - Flags: review?(ehsan)
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*?
Attachment #8646960 - Flags: review?(ehsan) → review+
Attachment #8646951 - Flags: review?(ehsan) → review+
(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.
(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?
https://hg.mozilla.org/mozilla-central/rev/4a4eba03ae3e
https://hg.mozilla.org/mozilla-central/rev/0f595614207f
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.