Nicholas Nethercote [inactive] Assignee
	Description • 8 years ago

I tried adding MOZ_MUST_USE to NS_IMETHOD in bug 1292440. I have concluded that is too blunt an instrument to be practical.

The obvious alternative is to add a new annotation to XPIDL that adds MOZ_MUST_USE to the generated declarations. The obvious name for this is [must_use]. It can apply equally well to methods and attributes.

	Nicholas Nethercote [inactive] Assignee
	Comment 1 • 8 years ago

To write this patch I just looked for all the places where |nostdcall| occurred
and did something appropriate for |must_use|.

I'm pretty sure this doesn't have any bad interactions with existing
attributes. The only one of interest is [infallible]. If you use [infallible,
must_use] on an XPIDL attribute, you end up with this generated code:

> /* [infallible,must_use] attribute boolean allowMedia; */
> MOZ_MUST_USE NS_IMETHOD GetAllowMedia(bool *aAllowMedia) = 0;
> inline bool GetAllowMedia()
> {
>   bool result;
>   mozilla::DebugOnly<nsresult> rv = GetAllowMedia(&result);
>   MOZ_ASSERT(NS_SUCCEEDED(rv));
>   return result;
> }
> MOZ_MUST_USE NS_IMETHOD SetAllowMedia(bool aAllowMedia) = 0;

It's not a useful combination -- you're forced to check the
guaranteed-to-be-NS_OK return value if you use the first overloading of
GetAllowMedia() -- but it's a coherent one. So I don't think it needs to be
disallowed or otherwise treated specially.

	Nicholas Nethercote [inactive] Assignee
	Comment 2 • 8 years ago

I'm partway through a patch that adds [must_use] throughout nsIFile.idl, but
fixing all the missing checks is turning out to be a large amount of work.
Here's a patch that is less compelling but much smaller, to demonstrate how
[must_use] works.

	Nicholas Nethercote [inactive] Assignee
	Comment 3 • 8 years ago

Another not especially compelling example :)

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 8 years ago

Here's the generated code for nsIPipe.h, in case it's helpful.

	Nicholas Nethercote [inactive] Assignee
	Comment 5 • 8 years ago

A piece of advance bikeshedding. In a declaration like this:

> MOZ_MUST_USE nsresult Foo(Blah aBlah);

the MOZ_MUST_USE clearly applies to the |nsresult|, which is good.

In a declaration like this:

> [must_use] void init(in nsIFile file);

the [must_use] looks like it applies to the |void|, which doesn't make sense. You have to understand that it's applying to the implicit |nsresult| return value.

With that in mind, maybe [must_use] isn't the best name. Perhaps [must_check] would be better? Or perhaps sticking with a name that mirrors MOZ_MUST_USE would be better. I'm not sure. Suggestions welcome.

	Nicholas Nethercote [inactive] Assignee
	Comment 6 • 8 years ago

Another thought: it would be useful to have an annotation that represents the opposite of [must_use]. It might be called [need_not_use]. The rationale being that without it, if you see a method/attribute without a [must_use] annotation, it's unclear if not checking the result is reasonable, or is this just a method/attribute that nobody has looked at yet. The annotation would have no effect, it would be essentially a comment. (I've already written some comments in some patches saying "this doesn't need a [must_use] annotation because ignoring the result is common and reasonable". This annotation would be a more concise version of that.)

	Nathan Froyd [:froydnj]
	Comment 7 • 8 years ago

(In reply to Nicholas Nethercote [:njn] from comment #5)
> A piece of advance bikeshedding. In a declaration like this:
> 
> > MOZ_MUST_USE nsresult Foo(Blah aBlah);
> 
> the MOZ_MUST_USE clearly applies to the |nsresult|, which is good.
> 
> In a declaration like this:
> 
> > [must_use] void init(in nsIFile file);
> 
> the [must_use] looks like it applies to the |void|, which doesn't make
> sense. You have to understand that it's applying to the implicit |nsresult|
> return value.
> 
> With that in mind, maybe [must_use] isn't the best name. Perhaps
> [must_check] would be better? Or perhaps sticking with a name that mirrors
> MOZ_MUST_USE would be better. I'm not sure. Suggestions welcome.

Yeah, I have this concern too.  The only problem is that making it more clear requires something like:

  [cxx_must_use_nsresult] void init(...);

which is a bit of a mouthful, though at least makes it clear to JS people looking at the IDL that you don't have to worry about what the attribute means for your JS implementation.  Or maybe:

  [cxx_must_use] void init(...);

would be OK?

	Nathan Froyd [:froydnj]
	Comment 8 • 8 years ago

Comment on attachment 8781794 [details] [diff] [review]
(part 1) - Add a [must_use] attribute to XPIDL

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

I think what we get for [infallible] things are OK.

For broader context: I talked to Nick offline about the possibility of making this a class-level attribute, and his response was that it's too big of a hammer; it's similar to declaring MOZ_MUST_USE on NS_IMETHOD.  I'm not too keen on adding a bunch of clutter to the IDL to add this attribute on Every. Single. Method., but I'm not sure there's a better way to do things yet.  This attribute at least has the virtue of letting us convert things incrementally, and maybe there will be some obvious conventions that come out of it once we have experience converting things ([check_all_nsresult] class attribute + [need_not_check] on particular methods?  who knows?).

comment 7 has my contribution to the bikeshed.  I vote for [cxx_must_use].

	Nicholas Nethercote [inactive] Assignee
	Comment 9 • 8 years ago

> > In a declaration like this:
> > 
> > > [must_use] void init(in nsIFile file);
> > 
> > the [must_use] looks like it applies to the |void|, which doesn't make
> > sense. You have to understand that it's applying to the implicit |nsresult|
> > return value.

After writing this, I realized that all the properties (binaryname, deprecated, noscript, etc.) could be interpreted this way. Which indicates it's the wrong way to think about it; anything appearing in the square brackets really applies to the entire method declaration.

Whether or not that changes your desire for the cxx_ prefix, I don't know.

	Nicholas Nethercote [inactive] Assignee
	Comment 10 • 8 years ago

> Whether or not that changes your desire for the cxx_ prefix, I don't know.

Also, most of the other attributes are C++ specific, too: notxpcom, infallible, nostdcom, binaryname, etc. So for consistency they should also have cxx_ prefixes, but of course they don't. So I'm now going to argue that must_use shouldn't have a cxx_ prefix either. Thoughts?

	Nathan Froyd [:froydnj]
	Comment 11 • 8 years ago

(In reply to Nicholas Nethercote [:njn] from comment #10)
> > Whether or not that changes your desire for the cxx_ prefix, I don't know.
> 
> Also, most of the other attributes are C++ specific, too: notxpcom,
> infallible, nostdcom, binaryname, etc. So for consistency they should also
> have cxx_ prefixes, but of course they don't. So I'm now going to argue that
> must_use shouldn't have a cxx_ prefix either. Thoughts?

That's a good point.  Let's go with [must_use].

	Nicholas Nethercote [inactive] Assignee
	Comment 12 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e85eb383995ef7b01cf57b51cd0ac0f5792380d2
Bug 1295825 (part 1) - Add a [must_use] attribute to XPIDL. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/44f472638f79eac0f0ad3ccb6011bbdf6ce32717
Bug 1295825 (part 2) - Use [must_use] and MOZ_MUST_USE in nsIPipe.idl. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2071b63a3d7a5fa478569eeb8d87ad644066989f
Bug 1295825 (part 3) - Use [must_use] and MOZ_MUST_USE in nsIGZFileWriter.idl. r=froydnj.

	Nicholas Nethercote [inactive] Assignee
	Comment 13 • 8 years ago

I updated https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL accordingly. I also added documentation for [infallible], which was missing.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e85eb383995e
https://hg.mozilla.org/mozilla-central/rev/44f472638f79
https://hg.mozilla.org/mozilla-central/rev/2071b63a3d7a