From 8021196662dcadf161cbeaebb3be4b0392b51803 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 18 Oct 2021 18:04:18 -0400 Subject: [PATCH 1/2] refactor: no need for specialized pyexpat code anymore The pyexpat bug that plagued us was fixed in Python 3.4: https://bugs.python.org/issue22462 We no longer need the code that adapted to it. The test will remain, couldn't hurt. --- coverage/ctracer/stats.h | 2 - coverage/ctracer/tracer.c | 79 --------------------------------------- coverage/ctracer/tracer.h | 4 -- coverage/inorout.py | 5 --- coverage/pytracer.py | 16 -------- 5 files changed, 106 deletions(-) diff --git a/coverage/ctracer/stats.h b/coverage/ctracer/stats.h index 05173369f..75e5cc740 100644 --- a/coverage/ctracer/stats.h +++ b/coverage/ctracer/stats.h @@ -17,10 +17,8 @@ typedef struct Stats { #if COLLECT_STATS unsigned int lines; unsigned int returns; - unsigned int exceptions; unsigned int others; unsigned int files; - unsigned int missed_returns; unsigned int stack_reallocs; unsigned int errors; unsigned int pycalls; diff --git a/coverage/ctracer/tracer.c b/coverage/ctracer/tracer.c index cf5bebb15..8b8953aa1 100644 --- a/coverage/ctracer/tracer.c +++ b/coverage/ctracer/tracer.c @@ -295,48 +295,6 @@ CTracer_set_pdata_stack(CTracer *self) * Parts of the trace function. */ -static int -CTracer_check_missing_return(CTracer *self, PyFrameObject *frame) -{ - int ret = RET_ERROR; - - if (self->last_exc_back) { - if (frame == self->last_exc_back) { - /* Looks like someone forgot to send a return event. We'll clear - the exception state and do the RETURN code here. Notice that the - frame we have in hand here is not the correct frame for the RETURN, - that frame is gone. Our handling for RETURN doesn't need the - actual frame, but we do log it, so that will look a little off if - you're looking at the detailed log. - - If someday we need to examine the frame when doing RETURN, then - we'll need to keep more of the missed frame's state. - */ - STATS( self->stats.missed_returns++; ) - if (CTracer_set_pdata_stack(self) < 0) { - goto error; - } - if (self->pdata_stack->depth >= 0) { - if (self->tracing_arcs && self->pcur_entry->file_data) { - if (CTracer_record_pair(self, self->pcur_entry->last_line, -self->last_exc_firstlineno) < 0) { - goto error; - } - } - SHOWLOG(PyFrame_GetLineNumber(frame), MyFrame_GetCode(frame)->co_filename, "missedreturn"); - self->pdata_stack->depth--; - self->pcur_entry = &self->pdata_stack->stack[self->pdata_stack->depth]; - } - } - self->last_exc_back = NULL; - } - - ret = RET_OK; - -error: - - return ret; -} - static int CTracer_handle_call(CTracer *self, PyFrameObject *frame) { @@ -773,30 +731,6 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame) return ret; } -static int -CTracer_handle_exception(CTracer *self, PyFrameObject *frame) -{ - /* Some code (Python 2.3, and pyexpat anywhere) fires an exception event - without a return event. To detect that, we'll keep a copy of the - parent frame for an exception event. If the next event is in that - frame, then we must have returned without a return event. We can - synthesize the missing event then. - - Python itself fixed this problem in 2.4. Pyexpat still has the bug. - I've reported the problem with pyexpat as http://bugs.python.org/issue6359 . - If it gets fixed, this code should still work properly. Maybe some day - the bug will be fixed everywhere coverage.py is supported, and we can - remove this missing-return detection. - - More about this fix: https://nedbatchelder.com/blog/200907/a_nasty_little_bug.html - */ - STATS( self->stats.exceptions++; ) - self->last_exc_back = frame->f_back; - self->last_exc_firstlineno = MyFrame_GetCode(frame)->co_firstlineno; - - return RET_OK; -} - /* * The Trace Function */ @@ -837,11 +771,6 @@ CTracer_trace(CTracer *self, PyFrameObject *frame, int what, PyObject *arg_unuse Py_DECREF(ascii); #endif - /* See below for details on missing-return detection. */ - if (CTracer_check_missing_return(self, frame) < 0) { - goto error; - } - self->activity = TRUE; switch (what) { @@ -863,12 +792,6 @@ CTracer_trace(CTracer *self, PyFrameObject *frame, int what, PyObject *arg_unuse } break; - case PyTrace_EXCEPTION: - if (CTracer_handle_exception(self, frame) < 0) { - goto error; - } - break; - default: STATS( self->stats.others++; ) break; @@ -1050,10 +973,8 @@ CTracer_get_stats(CTracer *self, PyObject *args_unused) "calls", self->stats.calls, "lines", self->stats.lines, "returns", self->stats.returns, - "exceptions", self->stats.exceptions, "others", self->stats.others, "files", self->stats.files, - "missed_returns", self->stats.missed_returns, "stack_reallocs", self->stats.stack_reallocs, "stack_alloc", self->pdata_stack->alloc, "errors", self->stats.errors, diff --git a/coverage/ctracer/tracer.h b/coverage/ctracer/tracer.h index fbbfa202c..65d748ca5 100644 --- a/coverage/ctracer/tracer.h +++ b/coverage/ctracer/tracer.h @@ -60,10 +60,6 @@ typedef struct CTracer { /* The current file's data stack entry. */ DataStackEntry * pcur_entry; - /* The parent frame for the last exception event, to fix missing returns. */ - PyFrameObject * last_exc_back; - int last_exc_firstlineno; - Stats stats; } CTracer; diff --git a/coverage/inorout.py b/coverage/inorout.py index 2c216ea9d..6bdac06e5 100644 --- a/coverage/inorout.py +++ b/coverage/inorout.py @@ -349,11 +349,6 @@ def nope(disp, reason): # can't do anything with the data later anyway. return nope(disp, "not a real file name") - # pyexpat does a dumb thing, calling the trace function explicitly from - # C code with a C file name. - if re.search(r"[/\\]Modules[/\\]pyexpat.c", filename): - return nope(disp, "pyexpat lies about itself") - # Jython reports the .class file to the tracer, use the source file. if filename.endswith("$py.class"): filename = filename[:-9] + ".py" diff --git a/coverage/pytracer.py b/coverage/pytracer.py index d4a0b7485..0a3625915 100644 --- a/coverage/pytracer.py +++ b/coverage/pytracer.py @@ -55,8 +55,6 @@ def __init__(self): self.started_context = False self.data_stack = [] - self.last_exc_back = None - self.last_exc_firstlineno = 0 self.thread = None self.stopped = False self._activity = False @@ -118,17 +116,6 @@ def _trace(self, frame, event, arg_unused): ) return None - if self.last_exc_back: - if frame == self.last_exc_back: - # Someone forgot a return event. - if self.trace_arcs and self.cur_file_data: - pair = (self.last_line, -self.last_exc_firstlineno) - self.cur_file_data.add(pair) - self.cur_file_data, self.cur_file_name, self.last_line, self.started_context = ( - self.data_stack.pop() - ) - self.last_exc_back = None - # if event != 'call' and frame.f_code.co_filename != self.cur_file_name: # self.log("---\n*", frame.f_code.co_filename, self.cur_file_name, frame.f_lineno) @@ -204,9 +191,6 @@ def _trace(self, frame, event, arg_unused): if self.started_context: self.context = None self.switch_context(None) - elif event == 'exception': - self.last_exc_back = frame.f_back - self.last_exc_firstlineno = frame.f_code.co_firstlineno return self._trace def start(self): From afe6cf34d022e8dbbaa47826c487a98ca6832721 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 18 Oct 2021 21:54:02 -0400 Subject: [PATCH 2/2] fix: avoid measuring generated code. #1160 --- CHANGES.rst | 8 ++++++++ coverage/disposition.py | 2 ++ coverage/inorout.py | 3 +++ tests/test_oddball.py | 22 +++++++++++++++++++--- 4 files changed, 32 insertions(+), 3 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 49f5dc9a8..966a56b3d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -30,6 +30,12 @@ Unreleased - Feature: The HTML report pages for Python source files now have a sticky header so the file name and controls are always visible. +- Fix: more generated code is now excluded from measurement. Code such as + `attrs`_ boilerplate, or doctest code, was being measured though the + synthetic line numbers meant they were never reported. Once Cython was + involved though, the generated .so files were parsed as Python, raising + syntax errors, as reported in `issue 1160`_. This is now fixed. + - Fix: When sorting human-readable names, numeric components are sorted correctly: file10.py will appear after file9.py. This applies to file names, module names, environment variables, and test contexts. @@ -41,6 +47,8 @@ Unreleased .. _issue 553: https://github.com/nedbat/coveragepy/issues/553 .. _issue 840: https://github.com/nedbat/coveragepy/issues/840 +.. _issue 1160: https://github.com/nedbat/coveragepy/issues/1160 +.. _attrs: https://www.attrs.org/ .. _changes_602: diff --git a/coverage/disposition.py b/coverage/disposition.py index dfcc6def3..1c39a9c19 100644 --- a/coverage/disposition.py +++ b/coverage/disposition.py @@ -30,6 +30,8 @@ def disposition_debug_msg(disp): """Make a nice debug message of what the FileDisposition is doing.""" if disp.trace: msg = f"Tracing {disp.original_filename!r}" + if disp.original_filename != disp.source_filename: + msg += f" as {disp.source_filename!r}" if disp.file_tracer: msg += ": will be traced by %r" % disp.file_tracer else: diff --git a/coverage/inorout.py b/coverage/inorout.py index 6bdac06e5..3bc7e54e4 100644 --- a/coverage/inorout.py +++ b/coverage/inorout.py @@ -317,6 +317,9 @@ def nope(disp, reason): disp.reason = reason return disp + if original_filename.startswith('<'): + return nope(disp, "not a real original file name") + if frame is not None: # Compiled Python files have two file names: frame.f_code.co_filename is # the file name at the time the .pyc was compiled. The second name is diff --git a/tests/test_oddball.py b/tests/test_oddball.py index c3082abbc..b59e6395e 100644 --- a/tests/test_oddball.py +++ b/tests/test_oddball.py @@ -393,7 +393,13 @@ class DoctestTest(CoverageTest): """Tests invoked with doctest should measure properly.""" def test_doctest(self): - self.check_coverage('''\ + # Doctests used to be traced, with their line numbers credited to the + # file they were in. Below, one of the doctests has four lines (1-4), + # which would incorrectly claim that lines 1-4 of the file were + # executed. In this file, line 2 is not executed. + self.make_file("the_doctest.py", '''\ + if "x" in "abc": + print("hello") def return_arg_or_void(arg): """If is None, return "Void"; otherwise return @@ -403,6 +409,11 @@ def return_arg_or_void(arg): 'arg' >>> return_arg_or_void("None") 'None' + >>> if "x" in "xyz": # line 1 + ... if "a" in "aswed": # line 2 + ... if "a" in "abc": # line 3 + ... return_arg_or_void(12) # line 4 + 12 """ if arg is None: return "Void" @@ -411,8 +422,13 @@ def return_arg_or_void(arg): import doctest, sys doctest.testmod(sys.modules[__name__]) # we're not __main__ :( - ''', - [1, 11, 12, 14, 16, 17], "") + ''') + cov = coverage.Coverage() + self.start_import_stop(cov, "the_doctest") + data = cov.get_data() + assert len(data.measured_files()) == 1 + lines = data.lines(data.measured_files().pop()) + assert lines == [1, 3, 18, 19, 21, 23, 24] class GettraceTest(CoverageTest):