Removing unncessary class states in bdb.Breakpoint

[bombs-kim]

Feature or enhancement

Proposal:

According to the comment in the Breakpoint class

Keeping state in the class is a mistake -- this means you cannot have more than one active Bdb instance.

I can support this idea further. Currently, Breakpoint has three class states, next, bplist, and bpbynumber.

class Breakpoint:
    ...
    next = 1
    bplist = {}
    bpbynumber = [None]

These states are used to enable the reuse of previously set breakpoint instances across multiple interactive sessions or other use cases. However, relying on class states is not the only way to achieve this goal.

Using class states in this case has several clear limitations. As noted in the comment, it makes it difficult to maintain more than one active Bdb instance. Additionally, logic dependent on class state can make the behavior of a new Bdb instance unpredictable in many scenarios unless the exact states of Breakpoint is fully known. Another drawback of maintaining states in the Breakpoint class is that it tightly couples the behavior of all Bdb instances. For example, the deleteMe method must be called periodically by a Bdb instance to ensure the Breakpoint class remains in a valid state.

# example deleteMe calls in Bdb methods

class Bdb:
    ...
    def clear_break(self, filename, lineno):
        ...
        for bp in Breakpoint.bplist[filename, lineno][:]:
            bp.deleteMe()  # here
        self._prune_breaks(filename, lineno)
        return None
        
    def clear_bpbynumber(self, arg):
        ...
        try:
            bp = self.get_bpbynumber(arg)
        except ValueError as err:
            return str(err)
        bp.deleteMe()  # here
        self._prune_breaks(bp.file, bp.line)
        return None

As an alternative, instance-level state can be maintained within Bdb or derived classes such as Pdb, allowing each instance to manage its own breakpoint data independently. This approach can still support the reuse of previously set breakpoint instances across multiple interactive sessions.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

• gh-127392: Removing unncessary class states in bdb.Breakpoint #127410

[picnixz]

cc @gaogaotiantian

[gaogaotiantian]

Next time please wait for a discussion before making such a big PR.

I agree, if we start the design from the beginning, we should not keep the state in the Breakpoint class. However, as you can tell from the blame of the comment, it has been there for 26 years. Even a small bug that has been 26 years could be a feature, let alone a design.

Your change is a breaking one, meaning it would make plenty of the code in the market fail, which is something we are trying to avoid for projects like CPython. You can get the idea when you were changing the tests - each test changed could be some code out there that would break.

Historically, sharing breakpoints between different Bdb(thus Pdb) instances was intentional, because pdb.set_trace() would create a new instance for each call. In order to remember the breakpoints between each inline breakpoint, we need to share it.

This might not be the best design, but over the years, it worked without any issue. Yes, it does not work well when we have two active Bdb instances, but what's a real life scenario for us to have two active debuggers in one process? That is an issue, but more like a hypothetical one at this point.

So my thought about this is - the change is a big risk, without enough justification. We don't have a real life use case to support the breaking change.

[bombs-kim]

@gaogaotiantian

Thank you for the feedback!

The PR I submitted was an attempt to address the design concern mentioned in the comment, as I thought it would be more direct to share the idea through code rather than just words. I realize now that it would have been better to include the code in the issue itself. I’ll make sure to do that next time.

I created this issue because of the comment in the codebase. I’m also okay with removing that comment, given it has been there for 26 years and hasn’t caused significant problems in practice. However, if we decide to keep the comment and treat it as a backlog item, it might be better to address it sooner rather than later, regardless of whether the solution is mine or another approach.

Regarding my proposed change, I understand it doesn’t solve an immediate, practical issue. However, I view it as an improvement in flexibility for potential future use cases, such as managing multiple Bdb instances. Its potential is unknown because the current design has restrained such use cases for decades, leaving no room for exploration. It’s similar to how the Global Interpreter Lock (GIL) has been a constraint: it has worked well enough for decades, but removing it could pave the way for new possibilities, albeit at some risk.

If the consensus is to leave the current design unchanged, I completely understand. My goal was to explore the comment’s implication and propose an alternative while respecting the project’s priorities. If this is the case, I think we should consider deleting the comment and committing to the current design for good.

Looking forward to any additional thoughts!

[ericvsmith]

I agree we shouldn't change this. My only suggestion would be to add a sentence or two from @gaogaotiantian's message to the comment.

[bombs-kim]

Hi @ericvsmith,

Thanks for your input! While I appreciate the idea of adding a sentence or two to clarify the comment, I’m concerned it might not significantly contribute to resolving the underlying issue. The comment has been in the codebase for 26 years and hasn’t caused practical issues, but it still points out a design limitation. Adding more detail to the comment might only defer the discussion further.

Instead, I think we should either:
1. Make a final decision to stick with the current design and delete the comment to avoid future confusion.
2. Keep the issue open as a backlog item, allowing the community to weigh in until a clear consensus emerges.

I feel this approach would better serve the long-term maintainability of the code. Let me know your thoughts!

[ericvsmith]

I think changing the comment to "Because state is stored in this class there cannot be multiple instances of Bdb. This was an unfortunate design choice, but it's too late to change it now.".