Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

New [must_use] property in XPIDL

132 views
Skip to first unread message

Nicholas Nethercote

unread,
Aug 22, 2016, 12:15:08 AM8/22/16
to dev-platform
Greetings,

I just added [1] a new property to XPIDL called [must_use].

When used with an IDL method, it will add MOZ_MUST_USE to the generated C++
declarations and macros relating to that method. Here is an example:

/* [must_use] void init (in nsIFile file); */
MOZ_MUST_USE NS_IMETHOD Init(nsIFile *file) = 0;

When used with an IDL attribute, it will add MOZ_MUST_USE to the generated C++
getter and setter declarations and macros for that attribute. Here is a
getter-only example:

/* [must_use] readonly attribute nsIAsyncInputStream inputStream; */
MOZ_MUST_USE NS_IMETHOD GetInputStream(nsIAsyncInputStream * *aInputStream) =
0;

Any method or attribute that is fallible and should usually/always have its
result checked should use this property. For example, I am working on
adding [must_use] to nsIFile.idl [2], and most of the methods and attributes in
that file should use this property. (But not all of them; the remove() method
is an example where not checking is common and reasonable, because it's usually
the last thing done on a file.)

I'm only part way through nsIFile.idl and I have already found dozens and
dozens of missing checks. Many of these missing checks constitute bugs, and the
effects depend on the call site. I am certain that the story will be similar
for other IDL files. Therefore, I strongly encourage people to do likewise on
any IDL files with which they are familiar. Adding appropriate checks isn't
always easy, so it's nice if it can be done by people familiar with the code in
question.

More generally, we stand to benefit from wider use of MOZ_MUST_USE in normal
C++ code, too. Any function that returns an error indicator (usually nsresult
or bool) is a good candidate. Past experience has shown that the use of
MOZ_MUST_USE frequently turns up missing checks.

If you file a bug to add [must_use] or MOZ_MUST_USE somewhere, please make the
bug block the tracking bug [3].

I'm happy to answer any questions people might have about this topic.

Nick

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1295825
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1296164
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=MOZ_MUST_USE

R Kent James

unread,
Aug 22, 2016, 7:39:19 PM8/22/16
to
On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
> I strongly encourage people to do likewise on
> any IDL files with which they are familiar. Adding appropriate checks isn't
> always easy

Exactly, and I hope that you and others restrain your exuberance a
little bit for this reason. A warning would be one thing, but a hard
failure that forces developers to drop what they are doing and think
hard about an appropriate check is just having you set YOUR priorities
for people rather than letting people do what might be much more
important work.

There's lots of legacy code around that may or may not be worth the
effort to think hard about such failures. This is really better suited
for a static checker that can make a list of problems, which list can be
managed appropriately for priority, rather than a hard error that forces
us to drop everything.

:rkent

Bobby Holley

unread,
Aug 22, 2016, 8:04:12 PM8/22/16
to R Kent James, dev-pl...@lists.mozilla.org
I don't quite follow the objection here.

Anybody who adds such an annotation needs to get the tree green before they
land the annotation. Developers writing new code that ignores the nsresult
will get instant feedback (by way of try failure) that the developer of the
API thinks the nsresult needs to be checked. This doesn't seem like an
undue burden, and enforced-by-default assertions are critical to code
hygiene and quality.

If your concern is the way this API change may break Thunderbird-only
consumers of shared XPCOM APIs, that's related to Thunderbird being a
non-Tier-1 platform, and pretty orthogonal to the specific change that Nick
made.

bholley



> :rkent
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

Nick Fitzgerald

unread,
Aug 22, 2016, 8:07:34 PM8/22/16
to R Kent James, dev-platform
On Mon, Aug 22, 2016 at 4:39 PM, R Kent James <ke...@caspia.com> wrote:

> On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
> > I strongly encourage people to do likewise on
> > any IDL files with which they are familiar. Adding appropriate checks
> isn't
> > always easy
>
> Exactly, and I hope that you and others restrain your exuberance a
> little bit for this reason. A warning would be one thing, but a hard
> failure that forces developers to drop what they are doing and think
> hard about an appropriate check
>

​Developers absolutely must already be thinking hard about appropriate
checks when calling fallible functions.​ These kinds of annotations make
our lives easier, not harder, in the long run.

Nathan Froyd

unread,
Aug 22, 2016, 8:07:58 PM8/22/16
to R Kent James, dev-platform
On Mon, Aug 22, 2016 at 7:39 PM, R Kent James <ke...@caspia.com> wrote:
> On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
>> I strongly encourage people to do likewise on
>> any IDL files with which they are familiar. Adding appropriate checks isn't
>> always easy
>
> Exactly, and I hope that you and others restrain your exuberance a
> little bit for this reason. A warning would be one thing, but a hard
> failure that forces developers to drop what they are doing and think
> hard about an appropriate check is just having you set YOUR priorities
> for people rather than letting people do what might be much more
> important work.

It's worth noting that "an appropriate check" may be as simple as:

mozilla::Unused << MustUseMethod(...);

which effectively retains the status quo of not checking, while
quieting the compiler warning.

-Nathan

Nicholas Nethercote

unread,
Aug 23, 2016, 12:28:45 AM8/23/16
to R Kent James, dev-platform
On Tue, Aug 23, 2016 at 9:39 AM, R Kent James <ke...@caspia.com> wrote:
> On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
>> I strongly encourage people to do likewise on
>> any IDL files with which they are familiar. Adding appropriate checks isn't
>> always easy
>
> Exactly, and I hope that you and others restrain your exuberance a
> little bit for this reason. A warning would be one thing, but a hard
> failure that forces developers to drop what they are doing and think
> hard about an appropriate check is just having you set YOUR priorities
> for people rather than letting people do what might be much more
> important work.

I see that I've caused several Thunderbird breakages with the changes
I've been making. I apologize for not giving you advance warning about
this.

I have several desires here.

- I want to greatly increase the use of MOZ_MUST_USE/[must_use] in Gecko.

- I don't want to make Thunderbird developers' lives more difficult.

- I don't want to have to modify comm-central when I add new uses of
MOZ_MUST_USE/[must_use] to mozilla-central.

The best solution I can see is to insert -Wno-unused-result or
-Wno-error=unused-result in the appropriate place(s) in comm-central.

Nick

Nicholas Nethercote

unread,
Aug 23, 2016, 12:42:44 AM8/23/16
to dev-platform
On Mon, Aug 22, 2016 at 2:14 PM, Nicholas Nethercote
<n.neth...@gmail.com> wrote:
>
> If you file a bug to add [must_use] or MOZ_MUST_USE somewhere, please make the
> bug block the tracking bug

Some recent work on this front:

- Jan Varga used [must_use] in nsIQuota{ManagerService,Requests}.idl:
https://bugzilla.mozilla.org/show_bug.cgi?id=1297056

- Wei-Cheng Pan is using MOZ_MUST_USE in uriloader/:
https://bugzilla.mozilla.org/show_bug.cgi?id=1293212

It's a good start, but barely scratches the surface! Let's keep them coming.

More motivation:
https://bugzilla.mozilla.org/show_bug.cgi?id=1290350#c6 explains how
an OOM crash that I thought I had straightforwardly fixed turns out to
be not so straightforward after all, due to missing checks.

Nick

R Kent James

unread,
Aug 23, 2016, 1:23:32 AM8/23/16
to Nicholas Nethercote, dev-platform
On 8/22/2016 9:28 PM, Nicholas Nethercote wrote:
> On Tue, Aug 23, 2016 at 9:39 AM, R Kent James <ke...@caspia.com> wrote:
>> On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
>>> I strongly encourage people to do likewise on
>>> any IDL files with which they are familiar. Adding appropriate checks isn't
>>> always easy
>> Exactly, and I hope that you and others restrain your exuberance a
>> little bit for this reason. A warning would be one thing, but a hard
>> failure that forces developers to drop what they are doing and think
>> hard about an appropriate check is just having you set YOUR priorities
>> for people rather than letting people do what might be much more
>> important work.
> I see that I've caused several Thunderbird breakages with the changes
> I've been making. I apologize for not giving you advance warning about
> this.
>
> I have several desires here.
>
> - I want to greatly increase the use of MOZ_MUST_USE/[must_use] in Gecko.
>
> - I don't want to make Thunderbird developers' lives more difficult.
>
> - I don't want to have to modify comm-central when I add new uses of
> MOZ_MUST_USE/[must_use] to mozilla-central.
>
> The best solution I can see is to insert -Wno-unused-result or
> -Wno-error=unused-result in the appropriate place(s) in comm-central.
>
> Nick

Thanks for the suggestions. Generally though Thunderbird accepts that we
need to fix these breakages, so I am not discouraging you from moving
forward for our sake.

I would still encourage you though to be a little careful about forcing
checks when they are not needed. Just as an example, in nsIPipe we now have:

|[must_use] readonly attribute nsIAsyncInputStream inputStream;|

A common programming pattern that I would use, when I don't really care
about why something failed, is:

nsCOMPtr<nsIAsyncInputStream> inputStream;

pipe.GetInputStream(getter_AddRefs(inputStream));

if (!inputStream) {

... take action

}

If I understand the must_use correctly, you are specifically not
allowing that pattern. Why? If you have good reasons, OK fine do it. Hey
I'm a hack, maybe my example sucks and you really do want to discourage
it. But I would like to understand the reasoning for this, as from my
perspective this is just code churn where your over-exuberance is
creating unneeded work (with associated risk of regressions).

Please consider this a general question and not Thunderbird-specific.

:rkent


Eric Rescorla

unread,
Aug 23, 2016, 9:20:30 AM8/23/16
to Bobby Holley, dev-pl...@lists.mozilla.org, R Kent James
I'm indifferent to this particular case, but I think that rkent's point
about static
checking is a good one. Given that C++ has existing annotations that say:

- This does not produce a useful return value (return void)
- I am explicitly ignoring the return value (cast to void)

And that we have (albeit imperfect) static checking tools that can detect
non-use of
return values, it seems like we would ultimately be better-off by using
those tools
to treat use of the return value by the default flagging a compiler error
when that
doesn't happen yet a third annotation rather than treating "use the return
value
somehow" as the default and flagging a compiler error when that doesn't
happen.
I appreciate that we have a lot of code that violates this rule now, so
actually cleaning
that up is a long process gradually converting the code base, but it has
the advantage
that once that's done then it just stays clean (just like any other -Werror
conversion).

-Ekr


On Mon, Aug 22, 2016 at 5:03 PM, Bobby Holley <bobby...@gmail.com> wrote:

Benjamin Smedberg

unread,
Aug 23, 2016, 9:40:51 AM8/23/16
to Eric Rescorla, dev-pl...@lists.mozilla.org, Bobby Holley, R Kent James
cast-to-void is commonly suggested as an alternative to an explicit unused
marking, and it is something that I wanted to use originally.
Unfortunately, we have not been able to make that work: this is primarily
because compilers often remove the cast-to-void as part of the parsing
phase, so it's not visible in the parse tree for static checkers.

--BDS

Eric Rescorla

unread,
Aug 23, 2016, 9:48:01 AM8/23/16
to Benjamin Smedberg, dev-pl...@lists.mozilla.org, Bobby Holley, R Kent James
Fair enough. I wouldn't be against introducing a separate unused marker for
this purpose.

-Ekr


On Tue, Aug 23, 2016 at 6:40 AM, Benjamin Smedberg <benj...@smedbergs.us>
wrote:

Michael Layzell

unread,
Aug 23, 2016, 11:08:30 AM8/23/16
to Eric Rescorla, Benjamin Smedberg, dev-pl...@lists.mozilla.org, Bobby Holley, R Kent James
We already have mozilla::Unused in mfbt/Unused.h. You can use it as
`mozilla::unused << SomethingReturningAValue();`. I believe that the
existing static analyses which look at unused variables respect this.

Gerald Squelart

unread,
Aug 23, 2016, 6:30:12 PM8/23/16
to
On the build-time vs static analysis point:

I'd much prefer to have errors pointed out right from './mach build', which I can fix on the spot; rather than wait hours until I notice static analysis errors on a treeherder build.
(e.g., I always forget explicit/MOZ_IMPLICIT!)

Unless we can get cross-platform & consistent static analysis from our build environments?

g.

Nicholas Nethercote

unread,
Aug 23, 2016, 8:16:12 PM8/23/16
to Benjamin Smedberg, Eric Rescorla, dev-pl...@lists.mozilla.org, Bobby Holley, R Kent James
On Tue, Aug 23, 2016 at 11:40 PM, Benjamin Smedberg
<benj...@smedbergs.us> wrote:
> cast-to-void is commonly suggested as an alternative to an explicit unused
> marking, and it is something that I wanted to use originally.
> Unfortunately, we have not been able to make that work: this is primarily
> because compilers often remove the cast-to-void as part of the parsing
> phase, so it's not visible in the parse tree for static checkers.

Also, while casting to void is enough to satisfy clang when calling a
MOZ_MUST_USE function, it doesn't satisfy GCC. Using mozilla::Unused,
as Michael mentioned elsewhere in the thread, is enough to satisfy
both compilers.

Nick

Nicholas Nethercote

unread,
Aug 23, 2016, 8:30:08 PM8/23/16
to R Kent James, dev-platform
On Tue, Aug 23, 2016 at 3:22 PM, R Kent James <ke...@caspia.com> wrote:
>
> A common programming pattern that I would use, when I don't really care
> about why something failed, is:
>
> nsCOMPtr<nsIAsyncInputStream> inputStream;
>
> pipe.GetInputStream(getter_AddRefs(inputStream));
>
> if (!inputStream) {
> ... take action
> }
>
> If I understand the must_use correctly, you are specifically not allowing
> that pattern. Why?

It is a common idiom. With a function like that (a getter for a
pointer) you have two ways of detecting failure -- looking at the
return value, or checking if the pointer is null. I prefer looking at
the return value, because:

- Consistency: that's what you do with other nsresult-returning
functions that don't match this idiom.

- Checkability: you can make "check the return value" unforgettable
with MOZ_MUST_USE, as I have done. It's harder to make "check if the
pointer is null" unforgettable. Maybe with a separate static analysis
tool, but that's much less convenient than the compiler telling you
immediately.

Unfortunately, it is sometimes unclear whether "return NS_OK and set
the pointer to null" is a possibility, and also whether "return a
failure code and set the pointer to non-null", which does complicate
things.

In general, some functions should obviously have [must_use], some
obviously should not, and then some are in a grey area and people
could reasonably disagree. I tend to err on the side of safety -- my
rule of thumb of late has been "what would Rust do?" -- because an
unnecessary check is less harmful than a missing check. But if you
feel strongly about that case, please file a bug and a patch, get r+,
and change it.

Nick

Jörg Knobloch

unread,
Aug 24, 2016, 3:17:13 AM8/24/16
to dev-pl...@lists.mozilla.org
On 23/08/2016 06:28, Nicholas Nethercote wrote:
> I see that I've caused several Thunderbird breakages with the changes
> I've been making. I apologize for not giving you advance warning about
> this.

Since I'm the guy who was, as Kent said, "forced [...] to drop
everything", I think some warning is all we need. Just NI us on the bug
with an approximate landing date and perhaps a hint what to do to
proactively fix the issues before they arise.

What's next? Bug 1297300? There are a few more bugs searching for
"must_use".

Jörg (Thunderbird Developer).

Gijs Kruitbosch

unread,
Aug 24, 2016, 7:38:09 AM8/24/16
to Nicholas Nethercote
Can this problem be solved with more of these types of
annotations/"compiler-aids", that is, could we teach the compiler to
accept either nsresult or out checks at least for simple xpidl property
fetches? Feels like that would avoid some "false" positives and
therefore some rewriting.

I'm mostly in frontend Firefox land so excuse the potentially dumb
question and probably incorrect terminology... My impression was that we
already have things like NonNull, and if we are now forcing nsresult
codes to be actually used, presumably we could just as well indicate
whether particular IDL properties could/couldn't be null, and accept a
check on that rather than the rv if the idiom is very common?


Separately, the "return failure but also set pointer to non-null"
concept sounds very strange to me. Do we have code that actually does
this? For a JS consumer, the return code ends up as an exception, so you
*can't* actually have a js-compatible implementation that both returns a
property value and sets a non-NS_OK return code - the property value
would be unfetchable from JS. With methods (rather than properties) and
outparams, in theory you could, but the resulting consumer code would
look something like:

let baz = {};
try {
foo.maybeGetBaz(baz);
} catch (ex) {
if (baz.value) {
...
}
}

which looks... odd, to say the least.

~ Gijs

Boris Zbarsky

unread,
Aug 24, 2016, 9:42:51 AM8/24/16
to
On 8/24/16 7:38 AM, Gijs Kruitbosch wrote:
> Separately, the "return failure but also set pointer to non-null"
> concept sounds very strange to me.

There are two possible ways of thinking about outparams in XPCOM:

1) The callee must not touch the outparam values when returning an error
result.

2) The caller must not examine the outparam values when an error result
was returned.

We haven't been very consistent about which convention we use,
unfortunately. I _think_ at one point #1 was the intended convention,
but we certainly have callees that violate it by, for example,
0-initializing all outparams up front to make it simpler to have
multiple success return points...

In practice, there are various code patterns that _can_ result in
"return failure but set to non-null". Here's a simple example:

nsresult
Foo::GetBar(nsIBar** aBar)
{
nsresult rv = SomeFactorMethod(aBar);
NS_ENSURE_SUCCESS(rv);

return (*aBar)->Init();
}

Now I agree that in this case it would be better to not write to *aBar
until we know Init() succeeds. That doesn't mean all of our code does
that. :(

> Do we have code that actually does this?

Hopefully not, but I can't guarantee it.

> For a JS consumer, the return code ends up as an exception, so you
> *can't* actually have a js-compatible implementation that both returns a
> property value and sets a non-NS_OK return code

Oh, I don't think anyone does this on _purpose_.

-Boris

Jim Blandy

unread,
Aug 24, 2016, 2:32:10 PM8/24/16
to R Kent James, dev-platform
On Mon, Aug 22, 2016 at 4:39 PM, R Kent James <ke...@caspia.com> wrote:

> Exactly, and I hope that you and others restrain your exuberance a
> little bit for this reason. A warning would be one thing, but a hard
> failure that forces developers to drop what they are doing and think
> hard about an appropriate check is just having you set YOUR priorities
> for people rather than letting people do what might be much more
> important work.
>

it seems to me that propagating errors correctly is a just fundamental part
of writing production code. It's more like a coding standards issue, not
something that's practical to treat as a question of personal preference
and priorities.

R Kent James

unread,
Aug 24, 2016, 3:18:23 PM8/24/16
to Jim Blandy, dev-platform
On 8/24/2016 11:31 AM, Jim Blandy wrote:
> On Mon, Aug 22, 2016 at 4:39 PM, R Kent James <ke...@caspia.com
> <mailto:ke...@caspia.com>> wrote:
>
> Exactly, and I hope that you and others restrain your exuberance a
> little bit for this reason. A warning would be one thing, but a hard
> failure that forces developers to drop what they are doing and think
> hard about an appropriate check is just having you set YOUR priorities
> for people rather than letting people do what might be much more
> important work.
>
>
> it seems to me that propagating errors correctly is a just fundamental
> part of writing production code. It's more like a coding standards
> issue, not something that's practical to treat as a question of
> personal preference and priorities.

It's clear that my opinion here is a tiny minority, so I'm not really
trying to change this anymore.

But for the record, there are LOTS of issues in Mozilla code that are
missing a "part of writing production code" My own pet peeve is JS code
that gives no hint about the types of inputs, when there are complex
assumptions about the types. Or C++ files that give you no hint about
why they exist, and their fundamental purpose. How would you like it if
I pushed a patch that shutdown all Firefox code development until all of
these documentation issues were cleaned up? That might not be the
highest priority today, right, even though it is a "fundamental part of
writing production code" (as well as my pet peeve and a high priority to
me)?

That is the point I am trying to make. But as I said I am a minority, so
not really worth continuing this discussion. So I guess I'll have to
drop what I am doing to review patches to fix this in really old,
unimportant code (like a SeaMonkey to Thunderbird profile migrator).

:rkent

Jim Blandy

unread,
Aug 24, 2016, 4:58:06 PM8/24/16
to R Kent James, dev-platform
On Wed, Aug 24, 2016 at 12:17 PM, R Kent James <ke...@caspia.com> wrote:

> But for the record, there are LOTS of issues in Mozilla code that are
> missing a "part of writing production code" My own pet peeve is JS code
> that gives no hint about the types of inputs, when there are complex
> assumptions about the types. Or C++ files that give you no hint about why
> they exist, and their fundamental purpose. How would you like it if I
> pushed a patch that shutdown all Firefox code development until all of
> these documentation issues were cleaned up? That might not be the highest
> priority today, right, even though it is a "fundamental part of writing
> production code" (as well as my pet peeve and a high priority to me)?
>

Fair enough. If someone simply broke all of Mozilla Central and said,
"Well, it's good engineering to do it this way", that wouldn't fly: within
Mozilla Central, the burden is on the annotator to keep the code working.
And indeed, the way the change was done, marking up interfaces is a
follow-up process that can proceed incrementally. Markup is rate-limited by
the rule that patches mustn't break the build.

So I guess what you're you're saying is that, out-of-tree, it's the
reverse: the burden rests on people who have no interest in the interface
being annotated, and who possibly aren't even familiar with the affected
code in their application. There's no rate limit on the impact on
out-of-tree code.

Mike Hommey

unread,
Aug 24, 2016, 5:39:04 PM8/24/16
to R Kent James, dev-pl...@lists.mozilla.org
On Mon, Aug 22, 2016 at 04:39:15PM -0700, R Kent James wrote:
> On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
> > I strongly encourage people to do likewise on
> > any IDL files with which they are familiar. Adding appropriate checks isn't
> > always easy
>
> Exactly, and I hope that you and others restrain your exuberance a
> little bit for this reason. A warning would be one thing, but a hard
> failure that forces developers to drop what they are doing and think
> hard about an appropriate check is just having you set YOUR priorities
> for people rather than letting people do what might be much more
> important work.

If you feel so strongly that you don't want to spend your time chasing
those new errors as they pop up, you're also free to drop what you are
doing and add -Wno-error=unused-result to your build flags.

Mike

Nicholas Nethercote

unread,
Aug 25, 2016, 1:14:41 AM8/25/16
to Gijs Kruitbosch, dev-platform
On Wed, Aug 24, 2016 at 9:38 PM, Gijs Kruitbosch
<gijskru...@gmail.com> wrote:
>
> Can this problem be solved with more of these types of
> annotations/"compiler-aids", that is, could we teach the compiler to accept
> either nsresult or out checks at least for simple xpidl property fetches?
> Feels like that would avoid some "false" positives and therefore some
> rewriting.

XPIDL has an [infallible] annotation for attributes. For those, an
infallible getter is generated, which returns the value directly
instead of via an outparam. It solves a lot of problems, but
unfortunately it's only useable within [builtinclass] interfaces --
because there's no way to guarantee that a JS implementation of an
interface really is infallible, whereas we can audit all C++
implementations -- and it only works with some builtin types.

> I'm mostly in frontend Firefox land so excuse the potentially dumb question
> and probably incorrect terminology... My impression was that we already have
> things like NonNull, and if we are now forcing nsresult codes to be actually
> used, presumably we could just as well indicate whether particular IDL
> properties could/couldn't be null, and accept a check on that rather than
> the rv if the idiom is very common?

I'm not sure if this addresses your comment directly, but: IDL methods
have to return nsresult to satisfy interface requirements. Because
xptcall, which is the code that handles these calls from C++ land and
JS land, expects an nsresult return value. Changing this would be
difficult.

Also, not all methods and attributes will/should get the [must_use] annotation.

Nick

Gijs Kruitbosch

unread,
Aug 25, 2016, 8:33:27 AM8/25/16
to Nicholas Nethercote
Sorry, I'm not being very clear, I'm afraid. I'm not asking to ditch
nsresult.

I'm wondering if we can make a "smarter" annotation than [must_use] that
requires *either* a nullcheck on the outparam or an nsresult check
(rather than requiring the latter and not being satisfied with a
nullcheck on the outparam). I don't know if that's feasible with today's
compilers and/or static checking, but it seemed worth asking.

If we could do something like that, in theory it might also be possible
to use it to annotate some of the wonky behaviour bz and you referenced
where nullchecking the outparam isn't enough (ie [must_use] vs. whatever
we call the other thing would indicate whether a non-null outparam was
enough to guarantee that the nsresult was NS_OK and everybody was happy.).

~ Gijs

ISHIKAWA,chiaki

unread,
Aug 25, 2016, 1:31:58 PM8/25/16
to dev-pl...@lists.mozilla.org
On 2016/08/25 6:38, Mike Hommey wrote:
> On Mon, Aug 22, 2016 at 04:39:15PM -0700, R Kent James wrote:
>> On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
>>> I strongly encourage people to do likewise on
>>> any IDL files with which they are familiar. Adding appropriate checks isn't
>>> always easy
>>
>> Exactly, and I hope that you and others restrain your exuberance a
>> little bit for this reason. A warning would be one thing, but a hard
>> failure that forces developers to drop what they are doing and think
>> hard about an appropriate check is just having you set YOUR priorities
>> for people rather than letting people do what might be much more
>> important work.
>
> If you feel so strongly that you don't want to spend your time chasing
> those new errors as they pop up, you're also free to drop what you are
> doing and add -Wno-error=unused-result to your build flags.
>
> Mike

But that means we have to add -Wno-error=unused-result to compiler flag
that produces the released binary in the server farm. Correct?
(At least this would be the case of C-C TB a couple of days ago...)

In the long run, I would DFINITELY like to see this type of compile-time
warning (maybe not the compiler failure) so that we can fix the
non-checking of should-be-checked return value of low primitives.
There are simply TOO MANY such omission of checks in C-C and M-C code to
my taste. I was disgusted to read the code (and still am) to read C-C
code to fix a minor bug (and that is not limited to C++ code, but to JS
code either), and lo and behold, when I trace the return value
processing, I often end up seeing that M-C code also fails to do the
proper checking of return value. (It is a problem of legacy code without
such built-in checks at the early stage.)

I have tried to add such checks in many places when I produced the patch
to enable buffering for file I/O (mostly output) in C-C TB.
Without such checks, the supposed transaction processing of e-mail
messages in the face of download/saving failure of the message does not
work, and I have found out there are places where proper error checking
that should have been there in the face of network file system failures
due to networking issues.

I have said this several times before, but if the code in mozilla source
tree is handed in as a course project to a systems programming 401 or
something like that, I would have no qualm to give D to the submitter as
the TA or lecturer.

There ought to be a long-term plan for C-C and M-C to introduce such
missing checks gradually.

Just two my cents worth.

CI

Nicholas Nethercote

unread,
Aug 25, 2016, 7:17:56 PM8/25/16
to ISHIKAWA,chiaki, dev-platform
On Fri, Aug 26, 2016 at 3:31 AM, ISHIKAWA,chiaki <ishi...@yk.rim.or.jp> wrote:
>
> There ought to be a long-term plan for C-C and M-C to introduce such missing
> checks gradually.

That's exactly what I'm trying to do with MOZ_MUST_USE/[must_use]. Bug
1268766 is the tracking bug. There's a lot of work to be done.

Much of this thread is about what is essentially yet another
manifestation of the "comm-central development sucks" problem, alas :(

Nick

Nicholas Nethercote

unread,
Aug 25, 2016, 7:20:40 PM8/25/16
to Gijs Kruitbosch, dev-platform
On Thu, Aug 25, 2016 at 10:33 PM, Gijs Kruitbosch
<gijskru...@gmail.com> wrote:
>
> Sorry, I'm not being very clear, I'm afraid. I'm not asking to ditch
> nsresult.
>
> I'm wondering if we can make a "smarter" annotation than [must_use] that
> requires *either* a nullcheck on the outparam or an nsresult check (rather
> than requiring the latter and not being satisfied with a nullcheck on the
> outparam). I don't know if that's feasible with today's compilers and/or
> static checking, but it seemed worth asking.

Oh, I see. I certainly don't know how to do that with standard
compiler annotations. Writing a custom static analysis check for that
might be feasible. But I also don't want to focus too much on this
nsresult+outparam case, because it's just one case of many, and I feel
like we're already in the weeds :)

Nick

Ehsan Akhgari

unread,
Aug 26, 2016, 10:52:17 AM8/26/16
to Gerald Squelart, dev-pl...@lists.mozilla.org
On 2016-08-23 6:30 PM, Gerald Squelart wrote:
> On the build-time vs static analysis point:
>
> I'd much prefer to have errors pointed out right from './mach build', which I can fix on the spot; rather than wait hours until I notice static analysis errors on a treeherder build.
> (e.g., I always forget explicit/MOZ_IMPLICIT!)
>
> Unless we can get cross-platform & consistent static analysis from our build environments?

For the errors discussed in this thread, you can add |ac_add_options
--enable-warnings-as-errors| to your mozconfig to catch them locally.
For our clang based static analysis checks, you can add |ac_add_options
--enable-clang-plugin| to your mozconfig. Make sure to use a clang
binary from llvm.org for best results (clang from Linux distros may or
may not work depending on how the distro packages it.)

0 new messages