[Lldb-commits] [PATCH] D69014: [LLDB] bugfix: command script add -f doesn't work for some callables

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added a comment.

In D69014#1711400 <https://reviews.llvm.org/D69014#1711400>, @jingham wrote:

> IIUC, the only external change in this patch is that before if you implemented your Python command using a class with an `__call__` method, it would have to be the signature that took exe_ctx.   Since this is now switching off of the number of arguments (less self) you could make an `__call__` form that didn't take the extra argument.  I certainly want to discourage that, since the form without the exe_ctx will do the wrong thing in some contexts (e.g. if the command is used in breakpoint callbacks or stop-hooks).  It looks like that would be easy to prevent, you know that you've found __call__ right above where you check.  So I think it would be better to explicitly disallow __call__forms that don't take the exe_ctx.
>
> This is different from the extra_args cases in the breakpoint callbacks and thread plans and so forth.  In those cases, if you don't need extra_args, there's no good reason why you should have to have them.  But in the case of Python commands, it's actually more correct to get the state you are operating on from the exe_ctx.  Otherwise you have to fall back on the currently selected process/thread/frame/etc, and there are contexts (like when you have multiple targets all hitting breakpoints simultaneously) where it does not make sense to have the currently selected state track what process/thread/frame are pertinent to the operation of a breakpoint command or stop hook.  So I do want to gently discourage this form of command.


Right now what happens on trunk is that commands implemented with `__call__` just don't work at all on any version of python with either 4 or 5 arguments.

If you look at my latest update, you can see I fix a bug in the old implementation `GetNumArguments` where it looks for `im_self` on the original object, not the `__call__`.   I fixed it to look at the `__call__` attribute, but that is not even entirely correct either, because the python data model states that `__call__` is one of the magic methods that must be set on the //class//, and cannot be overridden for individual objects.   Oh wait, no, that was the python 2.7 data model, it looks like they changed it in 3 so `foo()` really is synonymous with `foo.__call__()`

My point here is that python callables are complicated.   They are code.   Their implementation is liable to change, both because Python itself will change, and because users will change what they put in them.    I understand the backwards-compatibility argument for why we should inspect them, but even so, we should inspect them //as little as possible//.      Ideally the only inspection we'll ever do is to ask "can this callable take  N arguments?".   And when we ask that question we should ask it in the documented way, not by peeking inside method wrappers and python code objects and trying to understand them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69014/new/

https://reviews.llvm.org/D69014