From 6927be9a814cc6061838374b438d32ee3d4b3e2b Mon Sep 17 00:00:00 2001 From: John Reese Date: Mon, 7 Feb 2022 19:49:29 -0800 Subject: [PATCH 1/6] More robust evaluation for self-referential names Tracks the `target_name` that we are recursively evaluating, and short-circuits evaluation if we attempt to evaluate that name again. Also allows the assignment evaluation to try multiple assignments until either a real value is found, or all assignments are exhausted. Also prevents combining lhs and rhs of binary addition if either side is the `"??"` sentinel value. Fixes #56 --- dowsing/setuptools/setup_py_parsing.py | 40 +++++++++++++++++++------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/dowsing/setuptools/setup_py_parsing.py b/dowsing/setuptools/setup_py_parsing.py index 2a61bba..00e2f7f 100644 --- a/dowsing/setuptools/setup_py_parsing.py +++ b/dowsing/setuptools/setup_py_parsing.py @@ -178,7 +178,9 @@ def visit_Call(self, node: cst.Call) -> Optional[bool]: BOOL_NAMES = {"True": True, "False": False, "None": None} PRETEND_ARGV = ["setup.py", "bdist_wheel"] - def evaluate_in_scope(self, item: cst.CSTNode, scope: Any) -> Any: + def evaluate_in_scope( + self, item: cst.CSTNode, scope: Any, target_name: str = "" + ) -> Any: qnames = self.get_metadata(QualifiedNameProvider, item) if isinstance(item, cst.SimpleString): @@ -224,13 +226,23 @@ def evaluate_in_scope(self, item: cst.CSTNode, scope: Any) -> Any: # module scope isn't in the dict return "??" - return self.evaluate_in_scope(gp.value, scope) + # we have recursed, likey due to `x = x + y` assignment or similar + # self-referential evaluation + if target_name and target_name == name: + return "??" + + # keep trying assignments until we get something other than ?? + result = self.evaluate_in_scope(gp.value, scope, name) + if result and result != "??": + return result + # give up + return "??" elif isinstance(item, (cst.Tuple, cst.List)): lst = [] for el in item.elements: lst.append( self.evaluate_in_scope( - el.value, self.get_metadata(ScopeProvider, el) + el.value, self.get_metadata(ScopeProvider, el), target_name ) ) if isinstance(item, cst.Tuple): @@ -248,10 +260,10 @@ def evaluate_in_scope(self, item: cst.CSTNode, scope: Any) -> Any: for arg in item.args: if isinstance(arg.keyword, cst.Name): args[names.index(arg.keyword.value)] = self.evaluate_in_scope( - arg.value, scope + arg.value, scope, target_name ) else: - args[i] = self.evaluate_in_scope(arg.value, scope) + args[i] = self.evaluate_in_scope(arg.value, scope, target_name) i += 1 # TODO clear ones that are still default @@ -264,7 +276,9 @@ def evaluate_in_scope(self, item: cst.CSTNode, scope: Any) -> Any: d = {} for arg in item.args: if isinstance(arg.keyword, cst.Name): - d[arg.keyword.value] = self.evaluate_in_scope(arg.value, scope) + d[arg.keyword.value] = self.evaluate_in_scope( + arg.value, scope, target_name + ) # TODO something with **kwargs return d elif isinstance(item, cst.Dict): @@ -272,18 +286,20 @@ def evaluate_in_scope(self, item: cst.CSTNode, scope: Any) -> Any: for el2 in item.elements: if isinstance(el2, cst.DictElement): d[self.evaluate_in_scope(el2.key, scope)] = self.evaluate_in_scope( - el2.value, scope + el2.value, scope, target_name ) return d elif isinstance(item, cst.Subscript): - lhs = self.evaluate_in_scope(item.value, scope) + lhs = self.evaluate_in_scope(item.value, scope, target_name) if isinstance(lhs, str): # A "??" entry, propagate return "??" # TODO: Figure out why this is Sequence if isinstance(item.slice[0].slice, cst.Index): - rhs = self.evaluate_in_scope(item.slice[0].slice.value, scope) + rhs = self.evaluate_in_scope( + item.slice[0].slice.value, scope, target_name + ) try: if isinstance(lhs, dict): return lhs.get(rhs, "??") @@ -296,8 +312,10 @@ def evaluate_in_scope(self, item: cst.CSTNode, scope: Any) -> Any: # LOG.warning(f"Omit2 {type(item.slice[0].slice)!r}") return "??" elif isinstance(item, cst.BinaryOperation): - lhs = self.evaluate_in_scope(item.left, scope) - rhs = self.evaluate_in_scope(item.right, scope) + lhs = self.evaluate_in_scope(item.left, scope, target_name) + rhs = self.evaluate_in_scope(item.right, scope, target_name) + if lhs == "??" or rhs == "??": + return "??" if isinstance(item.operator, cst.Add): try: return lhs + rhs From 9ae3ac377e10d325f419ba82e1f6bce2523eefc7 Mon Sep 17 00:00:00 2001 From: John Reese Date: Tue, 8 Feb 2022 23:26:48 -0800 Subject: [PATCH 2/6] Improved assignment handling with sorting --- dowsing/setuptools/setup_py_parsing.py | 98 ++++++++++++++++++-------- dowsing/tests/setuptools.py | 29 ++++++++ 2 files changed, 97 insertions(+), 30 deletions(-) diff --git a/dowsing/setuptools/setup_py_parsing.py b/dowsing/setuptools/setup_py_parsing.py index 00e2f7f..ebf7b61 100644 --- a/dowsing/setuptools/setup_py_parsing.py +++ b/dowsing/setuptools/setup_py_parsing.py @@ -8,7 +8,12 @@ from typing import Any, Dict, Optional import libcst as cst -from libcst.metadata import ParentNodeProvider, QualifiedNameProvider, ScopeProvider +from libcst.metadata import ( + ParentNodeProvider, + PositionProvider, + QualifiedNameProvider, + ScopeProvider, +) from ..types import Distribution from .setup_and_metadata import SETUP_ARGS @@ -124,7 +129,12 @@ def leave_Call( class SetupCallAnalyzer(cst.CSTVisitor): - METADATA_DEPENDENCIES = (ScopeProvider, ParentNodeProvider, QualifiedNameProvider) + METADATA_DEPENDENCIES = ( + ScopeProvider, + ParentNodeProvider, + QualifiedNameProvider, + PositionProvider, + ) # TODO names resulting from other than 'from setuptools import setup' # TODO wrapper funcs that modify args @@ -179,7 +189,7 @@ def visit_Call(self, node: cst.Call) -> Optional[bool]: PRETEND_ARGV = ["setup.py", "bdist_wheel"] def evaluate_in_scope( - self, item: cst.CSTNode, scope: Any, target_name: str = "" + self, item: cst.CSTNode, scope: Any, target_name: str = "", target_line: int = 0 ) -> Any: qnames = self.get_metadata(QualifiedNameProvider, item) @@ -192,19 +202,30 @@ def evaluate_in_scope( elif isinstance(item, cst.Name): name = item.value assignments = scope[name] - for a in assignments: - # TODO: Only assignments "before" this node matter if in the - # same scope; really if we had a call graph and walked the other - # way, we could have a better idea of what has already happened. - + assignment_nodes = sorted( + ( + (self.get_metadata(PositionProvider, a.node).start.line, a.node) + for a in assignments + if a.node + ), + reverse=True, + ) + # Walk assignments from bottom to top, evaluating them recursively. + # When recursing, only look at assignments above the "target line". + for lineno, node in assignment_nodes: # Assign( # targets=[AssignTarget(target=Name(value="v"))], # value=SimpleString(value="'x'"), # ) # TODO or an import... # TODO builtins have BuiltinAssignment + + # we have recursed, likey due to `x = x + y` assignment or similar + # self-referential evaluation, and can't + if target_name and target_name == name and lineno >= target_line: + continue + try: - node = a.node if node: parent = self.get_metadata(ParentNodeProvider, node) if parent: @@ -214,27 +235,27 @@ def evaluate_in_scope( else: raise KeyError except (KeyError, AttributeError): - return "??" - - # This presumes a single assignment - if not isinstance(gp, cst.Assign) or len(gp.targets) != 1: - return "??" # TooComplicated(repr(gp)) + continue try: scope = self.get_metadata(ScopeProvider, gp) except KeyError: # module scope isn't in the dict - return "??" + continue - # we have recursed, likey due to `x = x + y` assignment or similar - # self-referential evaluation - if target_name and target_name == name: - return "??" + # This presumes a single assignment + if isinstance(gp, cst.Assign) and len(gp.targets) == 1: + result = self.evaluate_in_scope(gp.value, scope, name, lineno) + elif isinstance(parent, cst.AugAssign): + result = self.evaluate_in_scope(parent, scope, name, lineno) + else: + # too complicated? + continue # keep trying assignments until we get something other than ?? - result = self.evaluate_in_scope(gp.value, scope, name) - if result and result != "??": + if result != "??": return result + # give up return "??" elif isinstance(item, (cst.Tuple, cst.List)): @@ -242,7 +263,10 @@ def evaluate_in_scope( for el in item.elements: lst.append( self.evaluate_in_scope( - el.value, self.get_metadata(ScopeProvider, el), target_name + el.value, + self.get_metadata(ScopeProvider, el), + target_name, + target_line, ) ) if isinstance(item, cst.Tuple): @@ -260,10 +284,12 @@ def evaluate_in_scope( for arg in item.args: if isinstance(arg.keyword, cst.Name): args[names.index(arg.keyword.value)] = self.evaluate_in_scope( - arg.value, scope, target_name + arg.value, scope, target_name, target_line ) else: - args[i] = self.evaluate_in_scope(arg.value, scope, target_name) + args[i] = self.evaluate_in_scope( + arg.value, scope, target_name, target_line + ) i += 1 # TODO clear ones that are still default @@ -277,7 +303,7 @@ def evaluate_in_scope( for arg in item.args: if isinstance(arg.keyword, cst.Name): d[arg.keyword.value] = self.evaluate_in_scope( - arg.value, scope, target_name + arg.value, scope, target_name, target_line ) # TODO something with **kwargs return d @@ -286,11 +312,11 @@ def evaluate_in_scope( for el2 in item.elements: if isinstance(el2, cst.DictElement): d[self.evaluate_in_scope(el2.key, scope)] = self.evaluate_in_scope( - el2.value, scope, target_name + el2.value, scope, target_name, target_line ) return d elif isinstance(item, cst.Subscript): - lhs = self.evaluate_in_scope(item.value, scope, target_name) + lhs = self.evaluate_in_scope(item.value, scope, target_name, target_line) if isinstance(lhs, str): # A "??" entry, propagate return "??" @@ -298,7 +324,7 @@ def evaluate_in_scope( # TODO: Figure out why this is Sequence if isinstance(item.slice[0].slice, cst.Index): rhs = self.evaluate_in_scope( - item.slice[0].slice.value, scope, target_name + item.slice[0].slice.value, scope, target_name, target_line ) try: if isinstance(lhs, dict): @@ -312,8 +338,8 @@ def evaluate_in_scope( # LOG.warning(f"Omit2 {type(item.slice[0].slice)!r}") return "??" elif isinstance(item, cst.BinaryOperation): - lhs = self.evaluate_in_scope(item.left, scope, target_name) - rhs = self.evaluate_in_scope(item.right, scope, target_name) + lhs = self.evaluate_in_scope(item.left, scope, target_name, target_line) + rhs = self.evaluate_in_scope(item.right, scope, target_name, target_line) if lhs == "??" or rhs == "??": return "??" if isinstance(item.operator, cst.Add): @@ -323,6 +349,18 @@ def evaluate_in_scope( return "??" else: return "??" + elif isinstance(item, cst.AugAssign): + lhs = self.evaluate_in_scope(item.target, scope, target_name, target_line) + rhs = self.evaluate_in_scope(item.value, scope, target_name, target_line) + if lhs == "??" or rhs == "??": + return "??" + if isinstance(item.operator, cst.AddAssign): + try: + return lhs + rhs + except Exception: + return "??" + else: + return "??" else: # LOG.warning(f"Omit1 {type(item)!r}") return "??" diff --git a/dowsing/tests/setuptools.py b/dowsing/tests/setuptools.py index 0b13207..320c5cc 100644 --- a/dowsing/tests/setuptools.py +++ b/dowsing/tests/setuptools.py @@ -344,3 +344,32 @@ def test_add_items(self) -> None: self.assertEqual(d.name, "aaaa1111") self.assertEqual(d.packages, ["a", "b", "c"]) self.assertEqual(d.classifiers, "??") + + def test_self_reference_assignments(self) -> None: + d = self._read( + """\ +from setuptools import setup + +version = "base" +name = "foo" +name += "bar" +version = version + ".suffix" + +classifiers = [ + "123", + "abc", +] + +if True: + classifiers = classifiers + ["xyz"] + +setup( + name=name, + version=version, + classifiers=classifiers, +) + """ + ) + self.assertEqual(d.name, "foobar") + self.assertEqual(d.version, "base.suffix") + self.assertListEqual(d.classifiers, ["123", "abc", "xyz"]) From 1a80f10487a3f8ffec93cb060d756ea63d1df0c0 Mon Sep 17 00:00:00 2001 From: John Reese Date: Tue, 8 Feb 2022 23:53:58 -0800 Subject: [PATCH 3/6] Don't track names, just line numbers --- dowsing/setuptools/setup_py_parsing.py | 47 ++++++++++++++------------ dowsing/tests/setuptools.py | 20 +++++++++++ 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/dowsing/setuptools/setup_py_parsing.py b/dowsing/setuptools/setup_py_parsing.py index ebf7b61..8774e6d 100644 --- a/dowsing/setuptools/setup_py_parsing.py +++ b/dowsing/setuptools/setup_py_parsing.py @@ -189,7 +189,7 @@ def visit_Call(self, node: cst.Call) -> Optional[bool]: PRETEND_ARGV = ["setup.py", "bdist_wheel"] def evaluate_in_scope( - self, item: cst.CSTNode, scope: Any, target_name: str = "", target_line: int = 0 + self, item: cst.CSTNode, scope: Any, target_line: int = 0 ) -> Any: qnames = self.get_metadata(QualifiedNameProvider, item) @@ -211,20 +211,26 @@ def evaluate_in_scope( reverse=True, ) # Walk assignments from bottom to top, evaluating them recursively. - # When recursing, only look at assignments above the "target line". for lineno, node in assignment_nodes: + + # When recursing, only look at assignments above the "target line". + if target_line and lineno >= target_line: + continue + # Assign( # targets=[AssignTarget(target=Name(value="v"))], # value=SimpleString(value="'x'"), # ) + # + # AugAssign( + # target=Name(value="v"), + # operator=AddAssign(...), + # value=SimpleString(value="'x'"), + # ) + # # TODO or an import... # TODO builtins have BuiltinAssignment - # we have recursed, likey due to `x = x + y` assignment or similar - # self-referential evaluation, and can't - if target_name and target_name == name and lineno >= target_line: - continue - try: if node: parent = self.get_metadata(ParentNodeProvider, node) @@ -245,9 +251,9 @@ def evaluate_in_scope( # This presumes a single assignment if isinstance(gp, cst.Assign) and len(gp.targets) == 1: - result = self.evaluate_in_scope(gp.value, scope, name, lineno) + result = self.evaluate_in_scope(gp.value, scope, lineno) elif isinstance(parent, cst.AugAssign): - result = self.evaluate_in_scope(parent, scope, name, lineno) + result = self.evaluate_in_scope(parent, scope, lineno) else: # too complicated? continue @@ -265,7 +271,6 @@ def evaluate_in_scope( self.evaluate_in_scope( el.value, self.get_metadata(ScopeProvider, el), - target_name, target_line, ) ) @@ -284,12 +289,10 @@ def evaluate_in_scope( for arg in item.args: if isinstance(arg.keyword, cst.Name): args[names.index(arg.keyword.value)] = self.evaluate_in_scope( - arg.value, scope, target_name, target_line + arg.value, scope, target_line ) else: - args[i] = self.evaluate_in_scope( - arg.value, scope, target_name, target_line - ) + args[i] = self.evaluate_in_scope(arg.value, scope, target_line) i += 1 # TODO clear ones that are still default @@ -303,7 +306,7 @@ def evaluate_in_scope( for arg in item.args: if isinstance(arg.keyword, cst.Name): d[arg.keyword.value] = self.evaluate_in_scope( - arg.value, scope, target_name, target_line + arg.value, scope, target_line ) # TODO something with **kwargs return d @@ -312,11 +315,11 @@ def evaluate_in_scope( for el2 in item.elements: if isinstance(el2, cst.DictElement): d[self.evaluate_in_scope(el2.key, scope)] = self.evaluate_in_scope( - el2.value, scope, target_name, target_line + el2.value, scope, target_line ) return d elif isinstance(item, cst.Subscript): - lhs = self.evaluate_in_scope(item.value, scope, target_name, target_line) + lhs = self.evaluate_in_scope(item.value, scope, target_line) if isinstance(lhs, str): # A "??" entry, propagate return "??" @@ -324,7 +327,7 @@ def evaluate_in_scope( # TODO: Figure out why this is Sequence if isinstance(item.slice[0].slice, cst.Index): rhs = self.evaluate_in_scope( - item.slice[0].slice.value, scope, target_name, target_line + item.slice[0].slice.value, scope, target_line ) try: if isinstance(lhs, dict): @@ -338,8 +341,8 @@ def evaluate_in_scope( # LOG.warning(f"Omit2 {type(item.slice[0].slice)!r}") return "??" elif isinstance(item, cst.BinaryOperation): - lhs = self.evaluate_in_scope(item.left, scope, target_name, target_line) - rhs = self.evaluate_in_scope(item.right, scope, target_name, target_line) + lhs = self.evaluate_in_scope(item.left, scope, target_line) + rhs = self.evaluate_in_scope(item.right, scope, target_line) if lhs == "??" or rhs == "??": return "??" if isinstance(item.operator, cst.Add): @@ -350,8 +353,8 @@ def evaluate_in_scope( else: return "??" elif isinstance(item, cst.AugAssign): - lhs = self.evaluate_in_scope(item.target, scope, target_name, target_line) - rhs = self.evaluate_in_scope(item.value, scope, target_name, target_line) + lhs = self.evaluate_in_scope(item.target, scope, target_line) + rhs = self.evaluate_in_scope(item.value, scope, target_line) if lhs == "??" or rhs == "??": return "??" if isinstance(item.operator, cst.AddAssign): diff --git a/dowsing/tests/setuptools.py b/dowsing/tests/setuptools.py index 320c5cc..5e99c7b 100644 --- a/dowsing/tests/setuptools.py +++ b/dowsing/tests/setuptools.py @@ -373,3 +373,23 @@ def test_self_reference_assignments(self) -> None: self.assertEqual(d.name, "foobar") self.assertEqual(d.version, "base.suffix") self.assertListEqual(d.classifiers, ["123", "abc", "xyz"]) + + def test_circular_references(self) -> None: + d = self._read( + """\ +from setuptools import setup + +name = "foo" + +foo = bar +bar = version +version = foo + +setup( + name=name, + version=version, +) + """ + ) + self.assertEqual(d.name, "foo") + self.assertEqual(d.version, "??") From 1b7b694e88fd483914acf20ba8273b56840e15ef Mon Sep 17 00:00:00 2001 From: John Reese Date: Tue, 8 Feb 2022 23:59:50 -0800 Subject: [PATCH 4/6] x=x test case --- dowsing/tests/setuptools.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dowsing/tests/setuptools.py b/dowsing/tests/setuptools.py index 5e99c7b..3bf1119 100644 --- a/dowsing/tests/setuptools.py +++ b/dowsing/tests/setuptools.py @@ -385,6 +385,8 @@ def test_circular_references(self) -> None: bar = version version = foo +classifiers = classifiers + setup( name=name, version=version, @@ -393,3 +395,4 @@ def test_circular_references(self) -> None: ) self.assertEqual(d.name, "foo") self.assertEqual(d.version, "??") + self.assertEqual(d.classifiers, ()) From e42a849b0602768408b52474d0af6525506a4f8f Mon Sep 17 00:00:00 2001 From: Tim Hatch Date: Sat, 23 Nov 2024 19:31:30 -0800 Subject: [PATCH 5/6] Add test confirming builtin redefinition is ok (see pr comment) --- dowsing/tests/setuptools.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dowsing/tests/setuptools.py b/dowsing/tests/setuptools.py index 3bf1119..5ba2a8c 100644 --- a/dowsing/tests/setuptools.py +++ b/dowsing/tests/setuptools.py @@ -396,3 +396,24 @@ def test_circular_references(self) -> None: self.assertEqual(d.name, "foo") self.assertEqual(d.version, "??") self.assertEqual(d.classifiers, ()) + + def test_redefines_builtin(self) -> None: + d = self._read( + """\ +import setuptools +with open("CREDITS.txt", "r", encoding="utf-8") as fp: + credits = fp.read() + +long_desc = "a" + credits + "b" +name = "foo" + +kwargs = dict( + long_description = long_desc, + name = name, +) + +setuptools.setup(**kwargs) +""" + ) + self.assertEqual(d.name, "foo") + self.assertEqual(d.description, "??") From 10a837a691e4093ad0160fd99a3571ce1aac64d9 Mon Sep 17 00:00:00 2001 From: Tim Hatch Date: Sat, 23 Nov 2024 19:37:48 -0800 Subject: [PATCH 6/6] Fix typing glitch --- dowsing/tests/setuptools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dowsing/tests/setuptools.py b/dowsing/tests/setuptools.py index 5ba2a8c..88840a7 100644 --- a/dowsing/tests/setuptools.py +++ b/dowsing/tests/setuptools.py @@ -372,7 +372,7 @@ def test_self_reference_assignments(self) -> None: ) self.assertEqual(d.name, "foobar") self.assertEqual(d.version, "base.suffix") - self.assertListEqual(d.classifiers, ["123", "abc", "xyz"]) + self.assertSequenceEqual(d.classifiers, ["123", "abc", "xyz"]) def test_circular_references(self) -> None: d = self._read(