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

Jump to bottom

bpo-27578: Fix inspect.getsource() on empty file #20809

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

bpo-27578: Fix inspect.getsource() on empty file #20809

wants to merge 15 commits into from
+23 −1

Conversation

Reviewers

@remilapeyre remilapeyre

Assignees
No one assigned
Labels
awaiting change review CLA signed
Projects
None yet
Milestone
No milestone
4 participants
@kernc @remilapeyre @bedevere-bot @the-knights-who-say-ni
@kernc
Copy link
Contributor

@kernc kernc commented Jun 11, 2020 •
edited
Loading

For modules from empty files, inspect.getsource() instead of raising inaccurate OSError now returns an empty string, and inspect.getsourcelines() returns a list of one empty string, fixing the expected invariant.

As indicated by exec(''), empty strings are valid Python source code.

https://bugs.python.org/issue27578
@kernc
bpo-27578: Fix inspect.getsource() on empty file
83b3529 
For modules from empty files, `inspect.getsource()` now
returns an empty string, and `inspect.getsourcelines()` returns
a list of one empty string, fixing the expected invariant.

As indicated by `exec('')`, empty strings are valid Python
source code.
@kernc kernc mentioned this pull request Jun 11, 2020
Open
remilapeyre
remilapeyre suggested changes Jun 15, 2020
View changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment

Thanks for looking into this. I think it would make more sense to change https://github.com/python/cpython/blob/3.9/Lib/linecache.py#L140-L141 to:

    if not lines:
        lines = ['\n']
    elif not lines[-1].endswith('\n'):
        lines[-1] += '\n'

and then inspect would not need to be changed.

Loading

kernc added 4 commits Jun 15, 2020
@kernc
revert inspect
f7ad70d
@kernc
Instead linecache.getlines() on empty file returns '\n'
43431c4
@kernc
fixup test
ee60bd8
@kernc
fix unrelated PDB test now that even an empty module has '\n' source
95ebd0e
@kernc
Copy link
Contributor Author

@kernc kernc commented Jun 15, 2020

Thanks @remilapeyre! Indeed, using the whole existing pipeline makes plenty more sense. 😅

With trailing '\n' forced even on the first and only line of a blank module, this makes its inspect.getsource() return a truthy value, which required changing a seemingly unrelated test.

Is the discrepancy between linecache.getlines() and file.readlines() documented anywhere in prose, or is it just at one time someone's misplaced convenience for splicing tracebacks? I understand that's likely set in stone now either way?

Loading

@remilapeyre
Copy link
Contributor

@remilapeyre remilapeyre commented Jun 16, 2020

Thanks @remilapeyre! Indeed, using the whole existing pipeline makes plenty more sense. 😅

With trailing '\n' forced even on the first and only line of a blank module, this makes its inspect.getsource() return a truthy value, which required changing a seemingly unrelated test.

The change in the pdb test is related to this issue, it had nowhere to put the current line marker, but I think having the current line marker on an empty line when the file is empty is not an issue.

Is the discrepancy between linecache.getlines() and file.readlines() documented anywhere in prose, or is it just at one time someone's misplaced convenience for splicing tracebacks? I understand that's likely set in stone now either way?

linecache.getlines is specialised so I think it's OK to have it differs from readlines(). The documentation of linecache.getline says:

This function will never raise an exception — it will return '' on errors (the terminating newline character will be included for lines that are found).

It is useful to have a sentinel value that is a string as you don't have to worry to handle None when you call it and I think it makes sense to have linecache.getlines in similar fashion.

For the change to be accepted, you will have to add a test for the change in linecache and one for inspect as it's the initial issue on bpo.

Loading

@kernc
re-add inspect test
e42ccf6
@kernc
Copy link
Contributor Author

@kernc kernc commented Jun 16, 2020

Thanks. Test for inspect re-added. linecache on empty file is already changed/covered:

cpython/Lib/test/test_linecache.py

Lines 84 to 86 in 95ebd0e

class EmptyFile(GetLineTestsGoodData, unittest.TestCase):
file_list = []
test_list = ['\n']

Loading

@kernc
fixup NEWS
95f1f6b
remilapeyre
remilapeyre suggested changes Jul 20, 2020
View changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment

Thanks for making the change, I just have a last remark for the tests.

Loading

Lib/test/test_linecache.py Outdated Show resolved Hide resolved
Loading
kernc added 8 commits Jul 21, 2020
@kernc
touch fewer tests
031682c
@kernc
noop
aacd1d4
@kernc
Merge remote-tracking branch 'upstream/master' into inspect.getsource…
7590191 
…-empty-file
@kernc
Relocate test.support.os_helper.temp_cwd()
8c33efb
@kernc
noop
0012f8f
@kernc
Merge remote-tracking branch 'upstream/master' into inspect.getsource…
4e1626e 
…-empty-file
@kernc
noop
e24dd23
@kernc
Merge remote-tracking branch 'upstream/master' into inspect.getsource…
648160f 
…-empty-file
@kernc
Copy link
Contributor Author

@kernc kernc commented Nov 14, 2020

I didn't expect the Spanish Inquisition

Loading

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Nov 14, 2020

Nobody expects the Spanish Inquisition!

: please review the changes made to this pull request.

Loading

@kernc kernc mentioned this pull request Nov 14, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment