1434689 - Add support for a MOZ_TEMPORARY_CLASS annotation

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Eric Rahm [:erahm]]

Some types are intended to be temporary-only (eg `nsTSubstringTuple`), it would be nice to have an annotation for that, essentially the opposite of `MOZ_NON_TEMPORARY_CLASS` [1].

[1] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/mfbt/Attributes.h#470-474

Comment 1

[Eric Rahm [:erahm]]

I'm going to take a crack at this.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 1: Add moz_temporary_class annotation

This adds a `moz_non_temporary_class` annotation that can be used to indicate
a class is intended to only be used as a temporary.

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8947273 [details] [diff] [review]
Part 1: Add moz_temporary_class annotation

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

Hey Nika, this is my first draft at adding support for annotating temporary-only classes. I've got the the point where this compiles and the tests are run, but there are a two cases I think it's missing. Would you mind giving me some pointers?

On a side-note: Is there a better way to "run" the tests than doing a full |mach build|?

::: build/clang-plugin/tests/TestTemporaryClass.cpp
@@ +61,5 @@
> +
> +// Can't be member of a class, this should error.
> +struct RandomClass {
> +  // These should error.
> +  Temporary nonstaticMember; // This should fail. Why doesn't this fail?

I'd expect this to fail, but it does not. I feel like I need to update another bit of logic besides just `ScopeChecker::check`.

@@ +66,5 @@
> +  static Temporary staticMember; // expected-error {{variable of type 'Temporary' is only valid as a temporary}} expected-note {{value incorrectly allocated in a global variable}}
> +};
> +
> +// Need to declare MOZ_TEMPORARY_CLASS when inheriting, this should error.
> +struct BadInherit : Temporary {}; // Should this fail?

I also think this should fail, but maybe we don't need to be explicit about inheriting the behavior?

Comment 4

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

Comment on attachment 8947273 [details] [diff] [review]
Part 1: Add moz_temporary_class annotation

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

::: build/clang-plugin/tests/TestTemporaryClass.cpp
@@ +61,5 @@
> +
> +// Can't be member of a class, this should error.
> +struct RandomClass {
> +  // These should error.
> +  Temporary nonstaticMember; // This should fail. Why doesn't this fail?

This is allowed because RandomClass is only ever used as a temporary. It wouldn't be allowed if RandomClass was ever used in a non-temporary context.

@@ +66,5 @@
> +  static Temporary staticMember; // expected-error {{variable of type 'Temporary' is only valid as a temporary}} expected-note {{value incorrectly allocated in a global variable}}
> +};
> +
> +// Need to declare MOZ_TEMPORARY_CLASS when inheriting, this should error.
> +struct BadInherit : Temporary {}; // Should this fail?

same as above here

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 1: Add moz_temporary_class annotation

Thanks for the feedback, that makes sense now. I've update the comments but the content is the same.

This adds a `moz_non_temporary_class` annotation that can be used to indicate
a class is intended to only be used as a temporary.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 2: Add MOZ_TEMPORARY_CLASS attribute

This adds the MOZ_TEMPORARY_CLASS attribute to Attributes.h.

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 3: Annotate nsTSubstringTuple as MOZ_TEMPORARY_CLASS

The documentation indicates nsTStringTuple is intended for internal use only
and is designed to be only be used as a temporary. This makes that fact
explicit by annotating the class for static analysis.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> This adds a `moz_non_temporary_class` annotation that can be used to indicate

Fix the annotation name?

Comment 9

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> > This adds a `moz_non_temporary_class` annotation that can be used to indicate
> 
> Fix the annotation name?

Will do.

Comment 10

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

Comment on attachment 8947302 [details] [diff] [review]
Part 1: Add moz_temporary_class annotation

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

LGTM

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 8947304 [details] [diff] [review]
Part 3: Annotate nsTSubstringTuple as MOZ_TEMPORARY_CLASS

Redirecting, dbaron is out for a bit.

Comment 12

[Eric Rahm [:erahm]]

Try looks happy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=afd10420c1436fb6cda8181827bab29bc6f5ef68

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8947304 [details] [diff] [review]
Part 3: Annotate nsTSubstringTuple as MOZ_TEMPORARY_CLASS

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

+1

Comment 14

[Boris Zbarsky [:bzbarsky]]

Since IgnoreErrors has already landed, do you want to annotate that too?

Comment 15

[Eric Rahm [:erahm]]

Attachment: Part 4: Annotate IgnoreErrors as MOZ_TEMPORARY_CLASS

IgnoreErrors is intended to only be used as a temporary class, this adds an
explicit annotation.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8947995 [details] [diff] [review]
Part 4: Annotate IgnoreErrors as MOZ_TEMPORARY_CLASS

r=me

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8ec57b40f0
Part 1: Add moz_temporary_class annotation. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/0894630b8269
Part 2: Add MOZ_TEMPORARY_CLASS attribute. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f8f004395c
Part 3: Annotate nsTSubstringTuple as MOZ_TEMPORARY_CLASS. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/38fdcb96bb92
Part 4: Annotate IgnoreErrors as MOZ_TEMPORARY_CLASS. r=bz

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8a8ec57b40f0
https://hg.mozilla.org/mozilla-central/rev/0894630b8269
https://hg.mozilla.org/mozilla-central/rev/11f8f004395c
https://hg.mozilla.org/mozilla-central/rev/38fdcb96bb92