NOTMUCH_STATUS_LAST_STATUS problematic across additive library upgrades

NOTMUCH_STATUS_LAST_STATUS problematic across additive library upgrades

[Daniel Kahn Gillmor]

hi notmuch folks--

notmuch_status_t is an enum that counts up to
NOTMUCH_STATUS_LAST_STATUS.

This is fine for work within the library itself, but it seems
problematic to expose it to users of the library.

In particular, if a user builds against version X of the library, then
version X+1 is released with a new status code (but no
backward-incompatible API/ABI changes that would require an SONAME
bump), then the value of NOTMUCH_STATUS_LAST_STATUS would change, but
the application using notmuch wouldn't know about it.

Is this something we should be concerned about?  I don't know why or
when a library user might try to make use of NOTMUCH_STATUS_LAST_STATUS,
but it's also exposed in the go bindings, so it's leaking out quite a
bit.

   --dkg

Re: NOTMUCH_STATUS_LAST_STATUS problematic across additive library upgrades

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

On Mon, Dec 07 2015, Daniel Kahn Gillmor wrote:
> In particular, if a user builds against version X of the library, then
> version X+1 is released with a new status code (but no
> backward-incompatible API/ABI changes that would require an SONAME
> bump), then the value of NOTMUCH_STATUS_LAST_STATUS would change, but
> the application using notmuch wouldn't know about it.

Can you envision a scenario where this would actually lead to a bug?

It's meant to provide a convenience. It's clearly documented as "not an
actual status value", so, yes, no client code should ever be passing
that value into the library.

-Carl


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: NOTMUCH_STATUS_LAST_STATUS problematic across additive library upgrades

[Daniel Kahn Gillmor]

On Mon 2015-12-07 18:37:33 -0500, Carl Worth wrote:
> Can you envision a scenario where this would actually lead to a bug?
>
> It's meant to provide a convenience. It's clearly documented as "not an
> actual status value", so, yes, no client code should ever be passing
> that value into the library.

The only place a client can pass the value into the library is in
notmuch_status_to_string(), which is actually OK -- the library itself
will be able to tell the client what the status means, even if the
client didn't know about the status value itself.

I could see some hyper-correct client saying "whoa, we actually got a
status >= NOTMUCH_STATUS_LAST_STATUS; something must be horribly
broken!" and bailing unreasonably, but maybe we can safely say "don't do
that".

I note that it's exposed in the go bindings -- maybe it shouldn't be
exposed in any of the bindings that can avoid exposing it?

        --dkg

Re: NOTMUCH_STATUS_LAST_STATUS problematic across additive library upgrades

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

On Mon, Dec 07 2015, Daniel Kahn Gillmor wrote:
> I note that it's exposed in the go bindings -- maybe it shouldn't be
> exposed in any of the bindings that can avoid exposing it?

Quite likely. It's only there in C because there's no other way to know
the bounds for iterating over all possible values for a particular
enum. If other languages don't have this deficiency, then they shouldn't
need this silly thing.

-Carl

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: NOTMUCH_STATUS_LAST_STATUS problematic across additive library upgrades

[Daniel Kahn Gillmor]

On Tue 2015-12-08 00:22:11 -0500, Carl Worth wrote:
> On Mon, Dec 07 2015, Daniel Kahn Gillmor wrote:
>> I note that it's exposed in the go bindings -- maybe it shouldn't be
>> exposed in any of the bindings that can avoid exposing it?
>
> Quite likely. It's only there in C because there's no other way to know
> the bounds for iterating over all possible values for a particular
> enum. If other languages don't have this deficiency, then they shouldn't
> need this silly thing.

OK, so maybe the following patch should be applied.  Perhaps someone who
understands the go bindings better than i do can comment on it?

The python bindings don't seem to include LAST_STATUS, so i think
they're OK.

        --dkg

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index 0fff1ab..9f910cb 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -28,8 +28,6 @@ const (
 	STATUS_TAG_TOO_LONG
 	STATUS_UNBALANCED_FREEZE_THAW
 	STATUS_UNBALANCED_ATOMIC
-
-	STATUS_LAST_STATUS
 )
 
 func (self Status) String() string {