Skip to content

sys.monitoring branches for match cases showing incorrectly #123044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
jaltmayerpizzorno opened this issue Aug 15, 2024 · 5 comments
Open

sys.monitoring branches for match cases showing incorrectly #123044

jaltmayerpizzorno opened this issue Aug 15, 2024 · 5 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@jaltmayerpizzorno
Copy link

jaltmayerpizzorno commented Aug 15, 2024 •
edited by bedevere-app bot
Loading

Bug report

Bug description:

With the code from PR 122564, the destinations of branches into the blocks of certain cases are mapping to the pattern as well, making them look like they go out of scope:

v = 1
match v:
    case 1:
        x = 1
    case 2:
        x = 2
    case 3:
        x = 3
x += 2

This code yields (branches listing below extracted from co_branches()):

  0          0       RESUME                   0

  1          2       LOAD_CONST               0 (1)
             4       STORE_NAME               0 (v)

  2          6       LOAD_NAME                0 (v)

  3          8       COPY                     1
            10       LOAD_CONST               0 (1)
            12       COMPARE_OP              88 (bool(==))
            16       POP_JUMP_IF_FALSE        5 (to L1)
            20       NOT_TAKEN
            22       POP_TOP

  4         24       LOAD_CONST               0 (1)
            26       STORE_NAME               1 (x)
            28       JUMP_FORWARD            19 (to L3)

  5   L1:   30       COPY                     1
            32       LOAD_CONST               1 (2)
            34       COMPARE_OP              88 (bool(==))
            38       POP_JUMP_IF_FALSE        5 (to L2)
            42       NOT_TAKEN
            44       POP_TOP

  6         46       LOAD_CONST               1 (2)
            48       STORE_NAME               1 (x)
            50       JUMP_FORWARD             8 (to L3)

  7   L2:   52       LOAD_CONST               2 (3)
            54       COMPARE_OP              88 (bool(==))
            58       POP_JUMP_IF_FALSE        3 (to L3)
            62       NOT_TAKEN

  8         64       LOAD_CONST               2 (3)
            66       STORE_NAME               1 (x)

  9   L3:   68       LOAD_NAME                1 (x)
            70       LOAD_CONST               1 (2)
            72       BINARY_OP               13 (+=)
            76       STORE_NAME               1 (x)
            78       RETURN_CONST             3 (None)
'<module>' branches:
    ex8.py 3:9-3:10 "1" (<module>@16) -> 3:9-3:10 "1" (<module>@22) [case -> out]
    ex8.py 3:9-3:10 "1" (<module>@16) -> 5:9-5:10 "2" (<module>@30) [case -> next case]
    ex8.py 5:9-5:10 "2" (<module>@38) -> 5:9-5:10 "2" (<module>@44) [case -> out]
    ex8.py 5:9-5:10 "2" (<module>@38) -> 7:9-7:10 "3" (<module>@52) [case -> next case]
    ex8.py 7:9-7:10 "3" (<module>@58) -> 8:12-8:13 "3" (<module>@64) [case -> body]
    ex8.py 7:9-7:10 "3" (<module>@58) -> 9:0-9:1 "x" (<module>@68) [case -> next]

@markshannon @nedbat

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS

Linked PRs
@jaltmayerpizzorno jaltmayerpizzorno added the type-bug An unexpected behavior, bug, or error label Aug 15, 2024
This was referenced Aug 15, 2024
Merged
Closed
@gaogaotiantian
Copy link
Member

Do we craete issues for unmerged PR? I thought issues are supposed to be about code that's already merged. For unmerged PRs, shouldn't we just discuss it in the PR itself?

@picnixz picnixz added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Aug 16, 2024
@markshannon
Copy link
Member

I don't think there is any harm in creating a new issue. We often break down larger pieces of work into smaller issues.
We should make it clear that this is part of #122564, though.

@markshannon markshannon mentioned this issue Aug 16, 2024
Open
@markshannon
Copy link
Member

I think we should tackle these issues of locations within match statement related issues as follows:

  • Generally, we want to be giving POP_TOPs and other cleanup code NO_LOCATION in the front-end.
  • When assigning locations to instructions with no locations we should prefer locations within the same basic block to those locations outside it.
  • We also want the compiler to prefer locations following the basic block, to those preceding it, which assigning locations to basic blocks with no locations.

@iritkatriel does that make sense to you?

@iritkatriel
Copy link
Member

We also want the compiler to prefer locations following the basic block, to those preceding it, which assigning locations to basic blocks with no locations.

I think it's doing the opposite now. Will need to experiment with this.

@markshannon markshannon added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Aug 20, 2024
@markshannon
Copy link
Member

Given that we cannot reliably expected NOT_TAKEN to be followed by an instruction with a meaningful location (see #123050) we'll need a different approach to this.

We still need correct locations for actual expressions, but we should be fine with the compiler filling in locations as it does now for artificial instructions.

@bedevere-app bedevere-app bot mentioned this issue Feb 27, 2025
Merged
markshannon added a commit that referenced this issue Mar 10, 2025
@markshannon
GH-123044: Give the POP_TOP after a case test a location in the bod…
be046ee 
…y, not the pattern. (GH-130627)
seehwan pushed a commit to seehwan/cpython that referenced this issue Apr 16, 2025
@markshannon @seehwan80
pythonGH-123044: Give the POP_TOP after a case test a location in t…
6653bc2 
…he body, not the pattern. (pythonGH-130627)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@iritkatriel @markshannon @picnixz @gaogaotiantian @jaltmayerpizzorno