laike9m / Cyberbrain

Python debugging, redefined.
http://bit.ly/cyberbrain-features
MIT License
2.51k stars 159 forks source link

Fix multiline linenos for Python3.7 #132

Closed victorjzsun closed 2 years ago

victorjzsun commented 3 years ago

This PR tries to address the problem found in #110. The problem with multiline linenos was fixed in Python3.8 as described in https://github.com/python/cpython/pull/8774.

Instead of keeping random types of items on Cyberbrain's simulated value stack, the PR uses new StackItem classes.

In addition, added return_list option to _pop method to enforce returning a list of StackItems even when we are only popping one item.


Previous comment before solution was finalized:

For Python3.7, we can add the linenos to the value stack as we trace the bytecode. Then for any event, we take the smallest lineno from all of the bytecode operations for the event to estimate where the real line starts. Since some lines do not have any bytecode operation, this solution is not perfect. For the following example, currently, the corresponding event will have lineno=4. The proposed solution will change it to have lineno=2. The perfect solution, aka what has been implemented in Python3.8 has lineno=1.

s = [ # line 1
  a,  # line 2
  b,  # line 3
  c   # line 4
]     # line 5
laike9m commented 3 years ago

As a general feedback, I think the complexity the current implementation brings outweighs the benefits. I don't like the idea of adding another Value class, and I would hope to see metadata (like lineno) added to the Symbol class. Could you try moving towards that direction and see if it works?

victorjzsun commented 3 years ago

However, we quite often use placeholders with no Symbols in value stack when instructions are executed which throws away lineno info. Take the following example:

LOAD_CONST lineno=2
STORE_FAST lineno=None

If we push an empty placeholder for the LOAD_CONST instruction, we wouldn't have any information on the lineno for the STORE_FAST instruction. We could wrap every piece in value stack as a Symbol class but that's the same as my implementation

laike9m commented 3 years ago

(outdated) You're right. How about this, we can create a class called like StackItem, and put lineno there. Let Symbol inherit from StackItem. Then, for placeholders and errors, we wrap it with StackItem to make sure that everything on the stack is a StackItem object or a List[StackItem].

To be honest I also find having a list on the stack not ideal (for a long time), so perhaps we can even make StackItem be able to act as a container and hold multiple symbols, so we don't need to have a list sometimes, and it's always a StackItem object that's on the stack.

This is just a thought and we don't have to go this way. Let me know what you think.

laike9m commented 2 years ago

Suggest to move the removal of pkg_resources to another PR. It's not relevant to the lineno issue.

laike9m commented 2 years ago

LGTM'ed, only a nit left

This is a long journey, but I'm glad we finally completed it. Thanks for the dedication and patience!