+[GRMustache version] improperly overrides NSObject with different return type

[snej]

I enabled -Woverriding-method-mismatch in my target (highly recommended: without this the compiler will not verify that an overridden method has the same return type as its superclass method) and got a warning from GRMustache.h:

/vendor/GRMustache/include/GRMustache.h:59:1: error: conflicting return type in declaration of 'version': 'NSInteger' (aka 'long') vs 'GRMustacheVersion' [-Werror,-Woverriding-method-mismatch]
+ (GRMustacheVersion)version AVAILABLE_GRMUSTACHE_VERSION_6_0_AND_LATER;

There's already a +version method in NSObject (something to do with NSCoding), and it returns an NSInteger not a struct. It'd be a good idea to rename the GRMustache method to avoid this conflict.

(I'm using Xcode 5.0.2 in a Mac OS target, FYI. Building with the latest revision of GRMustache.)

[groue]

Hi Jens.

Well spotted: I did not know NSObject had a version method at all. However, there is no issue, and I won't fix it.

Let me explain why:

Activating -Woverriding-method-mismatch is not a good idea. Maybe it is for your own code, but it is not for code that does not belong to you. In our specific case, it reports a false positive:

As you said, +[NSObject version] is related to NSCoding. Since the GRMustache class is not suited for archiving, +[GRMustache version] and +[NSObject version] will never be called out of their proper context.

I recommend the reading of this stackexchange answer: http://programmers.stackexchange.com/questions/122608/clang-warning-flags-for-objective-c-development#124574

Written by a Clang developer, it explains the different groups of warnings. There are four of them: the warnings that are enabled by default, the warnings triggered by -Wall, the ones triggered by -Wextra, and, finally, the ones triggered by -Weverything.

-Woverriding-method-mismatch belongs to the "everything" group. Quoting him:

-Weverything: This is an insane group that literally enables every warning in Clang. Don't use this on your code. It is intended strictly for Clang developers or for exploring what warnings exist.

Thank you for your dedication, though. Happy Mustache!

[snej]

I didn't turn on -Weverything, which I agree would be nuts. I turned on one specific warning, one which should IMHO be on by default — it's a very bad idea to override a method with a different return type (for example Java or C++ will produce a hard compile error in this case.) For some reason GCC's Obj-C compiler didn't have a check for this; I remember filing a Radar on it years ago. I guess Clang finally added it, but because it's new it hasn't been folded into -Wall yet. (FYI, there are a lot of other very useful warnings that aren't in -Wall.)

I agree that in your specific case the inherited +initialize method won't be called because GRMustache doesn't support NSCoding. However, that doesn't make it any less ugly that it's shadowing a superclass method with the wrong return type. +version is still part of the public API of NSObject.

[snej]

FYI, there's a thread, that I started, about this flag on Apple's objc-language list from last year. In that thread John McCall (who works on Apple's compiler team) said:

I’m not sure why this one isn’t enabled by default. There are probably situations where programmers would like to override in a non-contravariant way; for example, with a delegate which will only be used with a specific kind of object. Still, we could have a reduced version enabled by default that requires roughly equivalent types to at least catch double-vs.-pointer differences. Please file a bug if you’d find this useful.

[groue]

I stick to my recommendation: only activate -Woverriding-method-mismatch for your own code.

Yes, the Liskov substitution principle is violated. NSObject defines a method, and GRMustache overrides it in an incompatible manner.

Yes, in class-oriented languages like Java or C++, this would have generated a compiler error.

However, in the message-oriented Objective-C language, this code compiles just fine. Our two version messages will not clash. There is no issue, no bug, no hidden horror waiting to blow up your application. Unless you archive GRMustache, which is not a thing to do. Please send John McCall here, he'll have another argument against enabling this warning by default.

Finally, please consider that fixing this non-issue would require a bump of the major version of the library, because this would be an incompatible change. For a non-existing issue. I don't want to do that.

If you want to keep -Woverriding-method-mismatch enabled for compiling GRMustache, while keeping your project pristine and warning-free, you can fork it, and wrap the declaration of the offending method:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Woverriding-method-mismatch"

+ (GRMustacheVersion)version;

#pragma clang diagnostic pop

[snej]

It has nothing to do with Obj-C being more dynamic than Java or C++. It's still static enough (unlike Ruby, say) that the compiler needs to know the approximate type of a return value at compile-time. Calling a method typed as integer and having a struct returned violates the ABI — I'm not sure what the exact result is, but it's likely to be stack corruption. I wouldn't call that "compiles just fine".

Your argument rests on the inherited +initialize method being for a feature of NSObject that GRMustache doesn't use. While this is true, it's really that you've dodged a bullet by luck. It's as though I had an NSView subclass where I implemented a -tag method that returns an NSString, not being aware that NSView already has an int-valued -tag method, and saying that "there's nothing wrong with my code because I never use view tags anyway".

Anyway, I can see that it would be awkward to have to fix this if it means bumping the major version, so I'll just work around it by wrapping my #import with those pragmas.

[groue]

It has nothing to do with Obj-C being more dynamic than Java or C++. It's still static enough (unlike Ruby, say) that the compiler needs to know the approximate type of a return value at compile-time. Calling a method typed as integer and having a struct returned violates the ABI — I'm not sure what the exact result is, but it's likely to be stack corruption. I wouldn't call that "compiles just fine".

How is the ABI violated?

Compiler knows just well the exact type of a return value: you would not invoke [GRMustache version] without importing GRMustache.h first, would you?

Even the runtime is just happy with it:

NSMethodSignature *sigNSObject = [NSObject methodSignatureForSelector:@selector(version)];
NSLog(@"%s", [sigNSObject methodReturnType]);
// q

NSMethodSignature *sigGRMustache = [GRMustache methodSignatureForSelector:@selector(version)];
NSLog(@"%s", [sigGRMustache methodReturnType]);
// {GRMustacheVersion=iii}

It's as though I had an NSView subclass where I implemented a -tag method that returns an NSString, not being aware that NSView already has an int-valued -tag method, and saying that "there's nothing wrong with my code because I never use view tags anyway".

And you will indeed never archive GRMustache. Yes, of course, I'm lucky, and this leads us to this:

Anyway, I can see that it would be awkward to have to fix this if it means bumping the major version, so I'll just work around it by wrapping my #import with those pragmas.

Thank you for your understanding. Maintaining a library involves responsabilities, and distinguishing real issues from harmless warnings is one of them.

[snej]

The ABI is violated if I have a Class object and invoke the regular +version on it, as in this artificial example:

    Class c = [GRMustache class];
    NSInteger vers = [c version];
    NSLog(@"version = %ld", vers);

It's not a very clean example because classes aren't strongly typed the way instances are; but the compiler will use the message as declared by NSObject and set up a call site expecting an integer return value. I don't know the x86 ABI so I'm not sure exactly what happens when the method tries to store a struct back. I actually ran this code (on OS X 64-bit) and it logged a huge random number. Nothing seemed to be immediately broken I'm not sure whether or not something in the stack got messed up.

[groue]

GRMustache is globally sane, you know, design-wise. I generally do care about this matter. Should I have known about the +[NSObject version] in the 1st place, I would have named my method +[GRMustache libraryVersion] or something like that.

Yes, the compiler can be fooled. If I'm lucky enough, the library will never fall in such a trap. And if eventually it does, not in a theoretical way, but in a way that actually bothers a user, I'll apologize and try to find a solution.

[snej]

FYI, here's the workaround I'm using, as a reference for other people who run into this (viz. #72). It's basically the same as what @groue suggested above, but I wrap it around the #import in my own source code so I don't have to fork GRMustache.

_Pragma("clang diagnostic push")
_Pragma("clang diagnostic ignored \"-Woverriding-method-mismatch\"")
#import "GRMustache.h"
_Pragma("clang diagnostic pop")

[groue]

Thanks, Jens :-)

I may well have a good idea for bumpimp the major version of the lib. This warning will then disappear.

[snej]

Cool! And sorry for being argumentative — it's easy to fall into language-lawyer mode. Aside from this one nit I'm really enjoying GRMustache :)

[groue]

I love arguments :-) And I'm glad the library is useful to you!

[groue]

The released GRMustache v7.0.0 fixes this issue. Thanks @snej for pointing this out!

[snej]

👍