Closed Bug 1493093 Opened Last year Closed Last year

Maybe<non-temporary>() should be allowed

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

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.
> 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.
(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.
Summary: Maybe<temporary>() should be allowed → Maybe<non-temporary>() should be allowed
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
Summary: Maybe<non-temporary>() should be allowed → Maybe<temporary>() should be allowed
Summary: Maybe<temporary>() should be allowed → Maybe<non-temporary>() should be allowed
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.
Attachment #9011178 - Flags: review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/5c8f8fe4ef3e
Allow to relax MOZ_NON_TEMPORARY_CLASS for some specific constructors r=andi
https://hg.mozilla.org/mozilla-central/rev/5c8f8fe4ef3e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → mh+mozilla
You need to log in before you can comment on or make changes to this bug.