[warning] Lift the restriction that first frame must be where warning is issued
We'll dynamically escape such frame from call site attribution instead
Design Doc: http://go/recipe-warning-design
R=iannucci
Change-Id: I8e803c92fd39a81a1ea8133de7be81eefdf0a635
Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/recipes-py/+/2293501
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Auto-Submit: Yiwei Zhang <yiwzhang@google.com>
diff --git a/recipe_engine/internal/warn/escape.py b/recipe_engine/internal/warn/escape.py
index 71abe96..76c7300 100644
--- a/recipe_engine/internal/warn/escape.py
+++ b/recipe_engine/internal/warn/escape.py
@@ -42,7 +42,7 @@
# Dict[_FuncLoc, Tuple[regular expression pattern]]
WARNING_ESCAPE_REGISTRY = {}
-def escape_warning_predicate(name, _, frame):
+def escape_warning_predicate(name, frame):
"""A predicate used in warning recorder that returns True when the function
that the given frame is currently executing is escaped from the given warning
name via decorators provided in this module.
diff --git a/recipe_engine/internal/warn/record.py b/recipe_engine/internal/warn/record.py
index 1476428..0d6acdb 100644
--- a/recipe_engine/internal/warn/record.py
+++ b/recipe_engine/internal/warn/record.py
@@ -20,6 +20,7 @@
from ..class_util import cached_property
from ..recipe_deps import Recipe, RecipeDeps, RecipeModule
+
@attr.s(frozen=True, slots=True)
class _AnnotatedFrame(object):
"""A wrapper class over built-in frame which associates additional attributes
@@ -40,9 +41,9 @@
causes for a given warning.
There're two types of warnings; Execution warnings and Import warnings:
- * Execution Warning: issued within the execution of recipe code
+ * Execution Warning: issued within the execution of recipe code.
* Import Warning: issued during dependency resolution (DEPS), when a recipe
- or recipe module depends on a module with warning declared.
+ or recipe module depends on a module with warning declared.
"""
# The RecipeDeps object for current recipe execution.
recipe_deps = attr.ib(validator=attr_type(RecipeDeps))
@@ -86,19 +87,17 @@
def record_execution_warning(self, name, frames):
"""Record the warning issued during recipe execution and its cause (
warning_pb.CallSite). A frame will be attributed as call site frame if it
- is the first frame in the supplied frames matching all of following
+ is the first frame in the supplied frames matching the following
conditions:
- * Not the frame where warning is issued
- * The source of code object frame is executing should be located in the
- main recipe repo or one of its descendant recipe repos
+ * The source code of the frame is 'recipe code' (i.e. in the current
+ recipe repo or one of its dependencies).
* The function that the frame executes is not escaped from the issued
- warning
+ warning.
Args:
- * name (str): Fully qualified warning name (e.g. repo_name/WARNING_NAME)
+ * name (str): Fully qualified warning name (e.g. repo_name/WARNING_NAME).
* frames (List[Frame]): List of frames captured at the time the given
- warning is issued (assuming the first frame is at where the warning is
- issued)
+ warning is issued.
"""
# TODO(yiwzhang): update proto to include skip reason and populate
call_site_frame, _ = self._attribute_call_site(name, frames)
@@ -122,9 +121,9 @@
warning_pb.ImportSite).
Args:
- * name (str): Fully qualified warning name (e.g. repo_name/WARNING_NAME)
+ * name (str): Fully qualified warning name (e.g. repo_name/WARNING_NAME).
* importer (Recipe|RecipeModule): The recipe or recipe module which
- depends on a recipe module with given warning name declared
+ depends on a recipe module with given warning name declared.
Raise ValueError if the importer is not instance of Recipe or RecipeModule
"""
@@ -149,18 +148,14 @@
skipped. A predicate function will have signature as follows.
Args:
- * name (str) - Fully qualified warning name e.g. 'repo/WARNING_NAME'
- * index (int) - The index of the provided frame in call stack. Outer
- frame has larger index.
+ * name (str) - Fully qualified warning name e.g. 'repo/WARNING_NAME'.
* frame (types.FrameType) - A frame in call stack that the predicate
- function is currently evaluating against
+ function is currently evaluating against.
Returns a human-readable reason (str) why the given frame should be skipped.
Returns None if the warning can be attributed to the given frame.
"""
return (
- # Skip the first frame as it is where the warning is being issued
- lambda name, index, frame: 'warning issued frame' if index == 0 else None,
self._non_recipe_code_predicate,
escape_warning_predicate
)
@@ -176,9 +171,8 @@
if all of the frames are skipped.
"""
skipped_frames = []
- for index, frame in enumerate(frames):
- lazy_skip_reasons = (
- p(name, index, frame) for p in self._skip_frame_predicates)
+ for frame in frames:
+ lazy_skip_reasons = (p(name, frame) for p in self._skip_frame_predicates)
reason = next((r for r in lazy_skip_reasons if r is not None), None)
if reason is None:
return frame, skipped_frames # culprit found
@@ -193,7 +187,7 @@
return tuple(repo.recipes_root_path for repo in (
list(itervalues(self.recipe_deps.repos))))
- def _non_recipe_code_predicate(self, name, index, frame):
+ def _non_recipe_code_predicate(self, _name, frame):
"""A predicate that skips a frame when it is executing a code object whose
source is not in any of the recipe repos in the currently executing
recipe_deps.
diff --git a/unittests/warn_test.py b/unittests/warn_test.py
index ebb5b30..0ad0fbd 100755
--- a/unittests/warn_test.py
+++ b/unittests/warn_test.py
@@ -157,22 +157,22 @@
self.assertTrue(cause.call_site.call_stack)
def test_record_execution_warning_skip_frame(self):
- def not_first_and_second_predicate(name, index, frame):
- return 'not the first and second' if index in (0, 1) else None
- self._override_skip_frame_predicates((not_first_and_second_predicate,))
+ def line_number_less_than_4(_name, frame):
+ return 'line number is less then 4' if frame.f_lineno < 4 else None
+ self._override_skip_frame_predicates((line_number_less_than_4,))
with create_test_frames(self.test_file_path) as test_frames:
self.recorder.record_execution_warning(
'recipe_engine/SOME_WARNING', test_frames)
- # attribute to the third frame
+ # attribute to frame on line 4
expected_cause = warning_pb.Cause()
expected_cause.call_site.site.file = self.test_file_path
- expected_cause.call_site.site.line = 5
+ expected_cause.call_site.site.line = 4
self.assert_has_warning('recipe_engine/SOME_WARNING', expected_cause)
def test_record_empty_site_for_execution_warning(self):
self._override_skip_frame_predicates((
- lambda name, index, frame: 'skip all frames', ))
+ lambda _name, _frame: 'skip all frames', ))
with create_test_frames(self.test_file_path) as test_frames:
self.recorder.record_execution_warning(
'recipe_engine/SOME_WARNING', test_frames)
@@ -339,7 +339,7 @@
@staticmethod
def apply_predicate(warning_name, frame):
- return escape.escape_warning_predicate(warning_name, -1, frame)
+ return escape.escape_warning_predicate(warning_name, frame)
if __name__ == '__main__':
