Skip to content
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.

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

bpo-45191: Fix tb_lineno for multi-line expressions #28753

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

nahco314
Copy link

@nahco314 nahco314 commented Oct 6, 2021

Merge remote-tracking branch 'upstream/main' into fix-issue-45191
bpo-45191: Fix tb_lineno for multi-line expressions
Merge remote-tracking branch 'upstream/main' into fix-issue-45191
aaa
Merge remote-tracking branch 'upstream/main' into fix-issue-45191
aaa
@nahco314
Copy link
Author

I think I made a mistake in the commit message, should I fix it?
Also, I have a commit history that I don't need, can it be merged together?

@pablogsal
Copy link
Member

I think I made a mistake in the commit message, should I fix it?
Also, I have a commit history that I don't need, can it be merged together?

Both can be changed when/if we merge, so don't worry too much about it :)

@pablogsal
Copy link
Member

You need to rebase though, as you have conflicts in test_dis

@nahco314
Copy link
Author

I think I've probably solved it.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 20, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@iritkatriel
Copy link
Member

I'm not sure about this approach. If we have more then one arg, and they are on different lines, it will report the last line, but that's probably not what we want.

>>> 1 \
 .bit_length(*["aaa"], \
             2,
             3)

I would go for the 3.8, 3.9 behaviour from #89354 (comment)

which means to fix the location for CALL.

@nahco314 nahco314 requested a review from iritkatriel as a code owner June 17, 2024 08:20
@nahco314
Copy link
Author

After thinking about it for a while, it certainly seems correct that the error location is aligned on the first line.
I have made a commit to correct this.

By the way, should I delete the old NEWS.d and create a new one...?

@nahco314
Copy link
Author

I believe that some of the tests that this code is failing should change the expected result, what do you think?
a.b() is interpreted as CALL(ATTR(a, "b"), []). It seems odd that the execution location of the CALL does not include the execution locations of all child nodes.

@iritkatriel
Copy link
Member

I believe that some of the tests that this code is failing should change the expected result, what do you think? a.b() is interpreted as CALL(ATTR(a, "b"), []). It seems odd that the execution location of the CALL does not include the execution locations of all child nodes.

If the range is too wide then coverage can show code that never executed as if it did execute. As in:

>>> f( 
...     (a + b) 
...     if x
...     else
...    (c + d)
... )

@nahco314
Copy link
Author

Sorry for the delay in replying.
I thought about it, but was not sure of the impact of this PR in terms of coverage, etc.
My understanding is that it would behave at least the same as a method call involving starg. If the behavior of this method call is undesirable, I think the behavior of the method call containing stararg needs to be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants