1493093 - Maybe<non-temporary>() should be allowed

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Mike Hommey [:glandium]]

Another set of static analysis errors that show up on mac when upgrading to clang 7, that should have been caught by clang 6 but haven't is:

  In file included from /builds/worker/workspace/build/src/obj-firefox/toolkit/recordreplay/Unified_cpp_toolkit_recordreplay1.cpp:29:
  /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/JSControl.cpp:441:28: error: variable of type 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is not valid in a temporary
                            dom::Optional<HandleObject>(), &obj, er);
                            ^
 /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/JSControl.cpp:441:28: note: value incorrectly allocated in a temporary
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:234:7: note: 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is a non-temporary type because it inherits from a non-temporary type 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >'
 class Optional<JS::Handle<T> > :
       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:217:23: note: 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >' is a non-temporary type because member 'mImpl' is a non-temporary type 'Maybe<JS::Rooted<JSObject *> >'
   Maybe<InternalType> mImpl;
                       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'Maybe<JS::Rooted<JSObject *> >' is a non-temporary type because it has a template argument non-temporary type 'JS::Rooted<JSObject *>'
 class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe
                                                                     ^
 In file included from /builds/worker/workspace/build/src/obj-firefox/toolkit/recordreplay/Unified_cpp_toolkit_recordreplay1.cpp:47:
 /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/ParentGraphics.cpp:105:28: error: variable of type 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is not valid in a temporary
                            dom::Optional<HandleObject>(), &obj, er);
                            ^
 /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/ParentGraphics.cpp:105:28: note: value incorrectly allocated in a temporary
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:234:7: note: 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is a non-temporary type because it inherits from a non-temporary type 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >'
 class Optional<JS::Handle<T> > :
       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:217:23: note: 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >' is a non-temporary type because member 'mImpl' is a non-temporary type 'Maybe<JS::Rooted<JSObject *> >'
   Maybe<InternalType> mImpl;
                       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'Maybe<JS::Rooted<JSObject *> >' is a non-temporary type because it has a template argument non-temporary type 'JS::Rooted<JSObject *>'
 class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe
                                                                     ^


That's not a false positive in strict terms, but it can be argued that it should be a negative. Because Maybe<Foo>() doesn't actually contain a Foo.

Now, another interesting fact is that Maybe comes with a Nothing. So instead of Maybe<Foo>(), one can actually use Nothing(). But Optional doesn't have the same thing... but it could. I'll file a separate bug for that. Whether we still want to relax the static analysis can still be discussed here.

Comment 1

[Mike Hommey [:glandium]]

> So instead of Maybe<Foo>(), one can actually use Nothing(). But Optional doesn't have the same thing... but it could. I'll file a separate bug for that.

Well, dom::ChromeUtils::Import takes a Optional<...>&, so that wouldn't help anyways. There needs to be an Optional<> temporary, and that'd be rejected by static analysis.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> > So instead of Maybe<Foo>(), one can actually use Nothing(). But Optional doesn't have the same thing... but it could. I'll file a separate bug for that.
> 
> Well, dom::ChromeUtils::Import takes a Optional<...>&, so that wouldn't help
> anyways. There needs to be an Optional<> temporary, and that'd be rejected
> by static analysis.

although... Optional<JS::Handle<JSObject*>>&... where JS::Handle<T> contains a const T*... oh my... triple indirection.

Comment 3

[Andi [:andi]]

As per our discussion on IRC, we should whitelist this case with an annotation. I see here two options:
1. expand the current matcher AstMatcher->addMatcher(materializeTemporaryExpr().bind("node"), this); and also make it match against CXXTemporaryObjectExpr. This will help us by avoiding the need to do the removal of ImplicitCast and since CXXTemporaryObjectExpr is direct decedent of CXXConstructorExpr, see [1] we will have direct access to annotation.
2. The second option will be to filter it out here [2], but doing so we will have to do a double isa<...> and I'm not very fund of that and also we need to explicitly remove the ImplicitCast.

In conclusion I will stick with option 1 since it's the easier solution, the only downside of it is that we need to bind CXXConstructorExpr to a new node and check it here [1].

[1] https://clang.llvm.org/doxygen/classclang_1_1CXXTemporaryObjectExpr.html
[2] https://dxr.mozilla.org/mozilla-central/source/build/clang-plugin/ScopeChecker.cpp#77

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1493093 - Allow to relax MOZ_NON_TEMPORARY_CLASS for some specific constructors

Comment 5

[Andi [:andi]]

Comment on attachment 9011178 [details]
Bug 1493093 - Allow to relax MOZ_NON_TEMPORARY_CLASS for some specific constructors

Andi-Bogdan Postelnicu [:andi] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c8f8fe4ef3e
Allow to relax MOZ_NON_TEMPORARY_CLASS for some specific constructors r=andi

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5c8f8fe4ef3e