py/runtime: Do generic lookup if type->attr fails. - #6253

Thanks for this alternative, it's definitely much simpler.

One thing to check is if there are existing types (eg user defined classes and instances thereof) which do a full lookup in type->attr(...) and if it fails then doing an extra lookup in locals_dict would be incorrect/inefficient. Eg doing int.xyz will call type_attr(...) which will search locals_dict, fail, and then it'll be searched again by this PR's change.

For your use-case would you prefer #4969 or this here? Ultimately both of the patches could be useful for different things, they can both coexist.

Thanks. What about the following?

    // dest[0] = MP_OBJ_NULL means load
    //  return: for fail, do nothing
    //          for continue lookup in locals_dict, dest[0] = MP_OBJ_SENTINEL    <---- **NEW**
    //          for attr, dest[0] = value
    //          for method, dest[0] = method, dest[1] = self

This way, the attr function can explicitly decide what is done next and, if I understand correctly, existing code would not be affected. If this makes sense, I'll update the PR. The change to runtime.c is almost the same.

For your use-case would you prefer #4969 or this here? Ultimately both of the patches could be useful for different things, they can both coexist.

Probably still #4969 (or something like it) for most of our use cases, which is returning attributes without running code on them.

This new PR originated from a different use case where we did want to run some code on reading a property (return transpose on my_matrix.T). It was after experimenting with it that I realized it could theoretically be an alternative if #4969 is too big to merge, but I agree that having them coexist would be nice.

This way, the attr function can explicitly decide what is done next and, if I understand correctly, existing code would not be affected. If this makes sense, I'll update the PR.

That does sound like a good idea.

Another thing to keep in mind with this and #4969 is that if/when they get merged they become part of the public C API, which makes it harder to change/refactor things later on, eg to make lookups for native types more efficient, see #4360 .

This way, the attr function can explicitly decide what is done next and, if I understand correctly, existing code would not be affected. If this makes sense, I'll update the PR.

That does sound like a good idea.

This turns out to be not entirely trivial.

• Setting dest[0] = MP_OBJ_SENTINEL alone as suggested above is not really possible because a lot of code just checks for dest[0] == MP_OBJ_NULL. Also, dest[0] = MP_OBJ_SENTINEL looks to be used already for a slightly unrelated use case.
• A similar issue arises with using dest[1] == MP_OBJ_SENTINEL. When it is not NULL after the initial lookup, some code (e.g. mp_load_attr) then tries to treat a normal attribute as a bound method and crashes. Both do work if you immediately set it back to MP_OBJ_NULL after checking.

So again quite a few options (and variations of each) and I'm not sure which is the best.

In the update that just pushed, I've opted to use dest[1] == MP_OBJ_SENTINEL as the criterion as I think it was not used anywhere yet. Instead of resetting it to NULL right after the check, I've set dest[1] = MP_OBJ_NULL in mp_convert_member_lookup in a separate commit, which seems to be consistent with most lookup/loading functions.

Implemented in e2ca8ab