gh-98390: Fix source locations of boolean sub-expressions

[iritkatriel]

Fixes #98390.

• Issue: Source locations of boolean sub-expressions are too wide #98390

[brandtbucher]

Thanks, looks good.

One comment: I don't think this function needs ploc anymore. LOC(e) is correct for everything emitted here, right?

It's actually a bug that the Compare_kind case sets the location without restoring it after. I introduced it in #96968 but haven't fixed it yet (since I didn't want to interfere with your refactoring work).

[iritkatriel]

Thanks, looks good.

One comment: I don't think this function needs ploc anymore. LOC(e) is correct for everything emitted here, right?

It's actually a bug that the Compare_kind case sets the location without restoring it after. I introduced it in #96968 but haven't fixed it yet (since I didn't want to interfere with your refactoring work).

I'm going over all the call sites to see who consumes the modified ploc (expect a PR about assert shortly). Once nobody consumes it, I will convert it to loc.

[15r10nk]

hi @iritkatriel,
I think the problem with the first fix of @brandtbucher is still there.

script:

def func():
    assert a and b<c , "msg"

import dis
for i in dis.get_instructions(func):
    if i.opname in ("COMPARE_OP","CALL"):
        print(i.positions,i.opname,i.argval)

output (Python 3.11.1):

Positions(lineno=2, end_lineno=2, col_offset=17, end_col_offset=20) COMPARE_OP <
Positions(lineno=2, end_lineno=2, col_offset=17, end_col_offset=20) CALL 0

The CALL is the creation of the assertion which has the same source range as b<c.

output (Python 3.11.0):

Positions(lineno=2, end_lineno=2, col_offset=17, end_col_offset=20) COMPARE_OP <
Positions(lineno=2, end_lineno=2, col_offset=4, end_col_offset=28) CALL 0

The source range of CALL in 3.11.0 is the whole assertion, which looks reasonable for me.

Some context:
I implemented the python 3.11 support for executing, which relies on the source positions to map bytecode instructions back to ast-nodes.

[carljm]

@15r10nk the behavior on main is correct for your example. I think what you are pointing out is that this fix hasn't been backported to 3.11.

@iritkatriel what do you think, is this a candidate for backport?

[brandtbucher]

It probably makes the most sense just to fix 3.11 separately (with a backport to 3.10), since location-related stuff has changed a lot on main.

I assigned myself to the issue a while back, but other things came up. I'll just make a PR now (thanks for the reminder @15r10nk)!

[15r10nk]

@carljm yes. I found this problem in 3.11.1.
I thought that this here is what was change between 3.11.0 and 3.11.1.
Sorry if this was the wrong pull-request.