RFA: Objective-c prototype search cleanups

[Alexander Malmberg]

Ziemowit Laski wrote:
> Hey Alex,
> 
> I've started looking at your patch, and generally it looks great, but I
> do
> have two "issues" with it.
> 
> First, the documentation states:
[snip documentation]
> My question is, if a class declares a method, why even look in the
> superclass (or
> any of its categories)? It is very clear which one the runtime will
> use.

No, it isn't. If there are several categories with conflicting
prototypes, or conflicts between categories and main interfaces, we
don't know what the runtime will do. Thus, I think a warning is highly
appropriate.

In the superclass/subclass, we can be reasonably sure what the runtime
will do. However, conflicting prototypes would break the principle that
you should be able to use a subclass anywhere where the superclass is
expected (the "Liskov substitution principle"). Thus, if you want to
adhere to this principle (which arguably, you should ;-), you want this
warning. If not, you can turn it off.

> Furthermore,
> the current approach (i.e., allowing superclass method prototypes to
> differ) allows for
> at least an "appearance" of covariant return type support. :-)

This is just a warning. It doesn't affect behavior, so covariant return
types will still work.

That said, the checks for prototype compatibility are stricter than they
need to be. My patch uses comp_proto_with_proto(), already used by the
global hash list checks, to do these checks. Improving the checking to
handle covariant/contravariant types (where applicable) is on my TODO
list, but since it's a rare case and just a warning, I don't consider it
a high priority (eg. fixing comp_proto_with_proto() to handle typedef:s
is more important :).

> Secondly, if no matching method prototype is found, you put in a
> last-ditch attempt
> to find it in the hash tables.  This sometimes succeeds and sometimes
> fails, leading
> to an inconsistent failure mode.

The change is intended to make the behavior here more sane and
consistent. From my quick testing, in 3.2 and earlier versions, the hash
tables were searched for receivers if the type was 'id<SomeProtocol>',
but not in other cases. Current mainline checks the hash tables if the
receiver is a class object of a forward declared class, but not in other
cases. (I'm not sure where this changed; I'll try to get some testing
done on 3.3.)

The current behavior is not good. :) Always, never, or only for
'id<SomeProtocol>' (and 'Class<SomeProtocol>') are all acceptable. This
choice is based on the optimistic assumption (that we've used elsewhere)
that we have seen the prototype for all messages, so if we can't find it
based on the type, it should be in the hash tables.

> Furthermore, whenever you do find a
> selector in a
> hash table, you do not appear to check for duplicates.  If one is to
> consult the hash
> tables at all, I think it should be done in the same way as when
> messaging 'id' or 'Class'
> (and preferrably in the same place in the code).

It is done in the same place, and it does check for duplicates. It's in
the final else block in the num_found_prototypes checks, lines
5855-5893.

> I'm sure we're all going to have a meaningful discussion on this, as
> always. :-)  In the meantime,
> is it OK if I just commit the 'instance methods of root class
> considered class methods' part
> of your patch?

Sure.

> I'd like to label the patch with your name, but I'm not
> sure what your copyright
> assignment status is.  Please let me know,

Pending. That particular change is fairly small, but you're the
maintainer, so it's your call. :)

- Alexander Malmberg