GeezerVison

The rationale for switching to last-wins instead of first-wins is reasonable: barring conditionals, last-wins at runtime.

I can also see some rationale to prefer leftmost (i.e. prefer top-level definition over something nested in e.g. a conditional.)

I'm not sure the rationale for the docstring or method based heuristics is strong enough. Simplicity of behavior has value. More heuristics make the behavior less predictable and harder to explain; we should be quite sure that they significantly increase our probability of giving the right answer.

Also, it seems like any changes to this heuristic could be backwards-incompatible and change behavior of existing code. Are we sure the benefit of changes here is sufficient to justify this potential breakage?

Using methods is almost definitive when there's a positive match - that's the only thing we have for classes that contain some compile time info. As long as the class has a method, we can find it with a super high certainty.

docstring would work as long as the class to be found has a unique docstring - which may not be super common, but not super rare either.

The bottom line is - I don't think either of the heuristics would generate a false positive in any case (unless the user manually assign a method of one class to another).

Yes this is not completely backward-compatible, but the code we potentially could break, is the code that we want to improve. (I know the sentence is a bit twisted)

I think the real way to solve this is to add a compile time info on the class, but that's a long shot.

How bad it is to add an extra dunderscore field that is not listed in dir for classes?

We have the information all along when we create the class, we just do not record it. This would solve this whole thing and may be useful in the future. An extra field won't break the existing code either. Just don't know how difficult it is to propose a new field on all classes.

How bad it is to add an extra dunderscore field that is not listed in dir for classes?

A new entire Python object attached to every class is a significant memory cost. It's not out of the question, but the value provided should be quite strong; I don't think making inspect.getsource work better in a few marginal edge cases meets the bar.

Okay I agree adding a class field only for inspect.getsource probably is not the best idea. As for the test - it's not obvious on the diff, but the existing test for cls238.cls239 is exactly testing this:

if True:
    class cls238:
        class cls239:
            '''if clause cls239'''
else:
    class cls238:
        class cls239:
            '''else clause 239'''
            pass

This is the perfect case where the old way just get it correct by luck (it would've got it wrong if it's if False), but the new way can always get it correctly.

Thank you for the review!