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

gh-125600: Only show stale code warning on source code display commands #125601

Merged
merged 8 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions Lib/pdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,15 @@ def _cmdloop(self):
except KeyboardInterrupt:
self.message('--KeyboardInterrupt--')

def _update_file_mtime(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name seems wrong - it's not "updating" the mtime if it's already there. It only sets it if it's not there.

Also, the docstring of _validate_file_mtime is no longer correct - it's not "since last time we saw it".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to init, do you think it's better? Also changed the docstring a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "save"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "save" better than "init"? Because I think save is similar to update. In this specific case, we only care for the entries that are not intialized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init sounds to me like you are initialising the actual file_mtime, rather than saving a copy of it. How about save_initial_file_mtime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that makes sense. I thought about it and I don't think we need to put it in the command path now. We used to do that because we want the error message on every command. Now we can do it once in setup for all the frames and leave onecmd alone.

"""update the file mtime table with the current frame's file"""
try:
filename = self.curframe.f_code.co_filename
mtime = os.path.getmtime(filename)
self._file_mtime_table.setdefault(filename, mtime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to overwrite a previous value if the file changed on disk?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, then let's avoid going to the disk when the dict already has a value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we separated the update and check. We do not generate warnings on every update. We want to avoid file change -> update(updated the new info) -> check(fail to find the change).

But yes, we should avoid updating it if not necessary.

except Exception:
return

def _validate_file_mtime(self):
"""Check if the source file of the current frame has been modified since
the last time we saw it. If so, give a warning."""
Expand All @@ -505,7 +514,7 @@ def _validate_file_mtime(self):
mtime != self._file_mtime_table[filename]):
self.message(f"*** WARNING: file '{filename}' was edited, "
"running stale code until the program is rerun")
self._file_mtime_table[filename] = mtime
self._file_mtime_table[filename] = mtime

# Called before loop, handles display expressions
# Set up convenience variable containers
Expand Down Expand Up @@ -835,7 +844,7 @@ def onecmd(self, line):
a breakpoint command list definition.
"""
if not self.commands_defining:
self._validate_file_mtime()
self._update_file_mtime()
if line.startswith('_pdbcmd'):
command, arg, line = self.parseline(line)
if hasattr(self, command):
Expand Down Expand Up @@ -979,6 +988,7 @@ def completedefault(self, text, line, begidx, endidx):

def _pdbcmd_print_frame_status(self, arg):
self.print_stack_trace(0)
self._validate_file_mtime()
self._show_display()

def _pdbcmd_silence_frame_status(self, arg):
Expand Down Expand Up @@ -1860,6 +1870,7 @@ def do_list(self, arg):
self.message('[EOF]')
except KeyboardInterrupt:
pass
self._validate_file_mtime()
do_l = do_list

def do_longlist(self, arg):
Expand All @@ -1878,6 +1889,7 @@ def do_longlist(self, arg):
self.error(err)
return
self._print_lines(lines, lineno, breaklist, self.curframe)
self._validate_file_mtime()
do_ll = do_longlist

def do_source(self, arg):
Expand Down
19 changes: 19 additions & 0 deletions Lib/test/test_pdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -3662,6 +3662,25 @@ def test_file_modified_after_execution(self):
self.assertIn("WARNING:", stdout)
self.assertIn("was edited", stdout)

def test_file_modified_and_immediately_restarted(self):
script = """
print("hello")
"""

# the time.sleep is needed for low-resolution filesystems like HFS+
commands = """
filename = $_frame.f_code.co_filename
f = open(filename, "w")
f.write("print('goodbye')")
import time; time.sleep(1)
f.close()
restart
"""

stdout, stderr = self.run_pdb_script(script, commands)
self.assertNotIn("WARNING:", stdout)
self.assertNotIn("was edited", stdout)

def test_file_modified_after_execution_with_multiple_instances(self):
# the time.sleep is needed for low-resolution filesystems like HFS+
script = """
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Only show stale code warning in :mod:`pdb` when we display source code.
Loading