From fb1332d5292453903444f97f2fb41932c236f96e Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Thu, 30 Mar 2023 23:51:13 -0400 Subject: [PATCH] (core) Fix bug with renaming when a formula uses a local name for a user table. Summary: When a formula used a local variable referring to a user table (which is a global name), e.g. `myvar = MyTable; myvar.lookupOne(...)`, renaming logic mistakenly used the inferred name (`MyTable`) in places where the actual variable name (`myvar`) should have been used. Additionally, we should not rename local variables, even if they match a global name. This fixes both issues. Test Plan: Added a test case that checks that local variables aren't renamed. Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3846 --- sandbox/grist/codebuilder.py | 12 +++++++++--- sandbox/grist/test_renames2.py | 24 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/sandbox/grist/codebuilder.py b/sandbox/grist/codebuilder.py index de77cfc2..bcd8086d 100644 --- a/sandbox/grist/codebuilder.py +++ b/sandbox/grist/codebuilder.py @@ -195,6 +195,11 @@ def _is_table(node): return (isinstance(node, astroid.nodes.ClassDef) and node.decorators and node.decorators.nodes[0].as_string() == 'grist.UserTable') +def _is_local(node): + """ + Returns true if node is a Name node for an innermost variable. + """ + return isinstance(node, astroid.nodes.Name) and node.name in node.scope().locals @contextlib.contextmanager @@ -394,14 +399,15 @@ def parse_grist_names(builder): in_text, in_value, in_patch = patch_source if in_value: return (in_value, in_patch.start, table_id, col_id) + return None parsed_names = [] for node in asttokens.util.walk(atok.tree): if isinstance(node, astroid.nodes.Name): obj = infer(node) - if _is_table(obj): + if _is_table(obj) and not _is_local(node): start, end = atok.get_text_range(node) - parsed_names.append(make_tuple(start, end, obj.name, None)) + parsed_names.append(make_tuple(start, end, node.name, None)) elif isinstance(node, astroid.nodes.Attribute): obj = infer(node.expr) @@ -431,7 +437,7 @@ def save_to_linecache(source_code): Makes source code available to friendly-traceback and traceback formatting in general. """ if six.PY3: - import friendly_traceback.source_cache + import friendly_traceback.source_cache # pylint: disable=import-error friendly_traceback.source_cache.cache.add(code_filename, source_code) else: diff --git a/sandbox/grist/test_renames2.py b/sandbox/grist/test_renames2.py index b57e2aba..ce0ed111 100644 --- a/sandbox/grist/test_renames2.py +++ b/sandbox/grist/test_renames2.py @@ -395,3 +395,27 @@ class TestRenames2(test_engine.EngineTestCase): self.assertTableData("People", cols="subset", data=self.people_data) self.assertTableData("Games", cols="subset", data=self.games_data) + + def test_renames_i(self): + # Rename when using a local variable referring to a user table. + # Test also that a local variable that happens to match a global name is unaffected by renames. + self.modify_column("Games", "winner", formula=( + "myvar = Entries\n" + "People = Entries\n" + "myvar.lookupOne(game=$id, rank=1).person\n" + "People.lookupOne(game=$id, rank=1).person\n" + )) + self.apply_user_action(["RenameColumn", "Entries", "game", "juego"]) + self.apply_user_action(["RenameTable", "People", "Persons"]) + + # Check that renames worked. + new_col = self.engine.docmodel.columns.lookupOne(tableId='Games', colId='winner') + self.assertEqual(new_col.formula, ( + "myvar = Entries\n" + "People = Entries\n" + "myvar.lookupOne(juego=$id, rank=1).person\n" + "People.lookupOne(juego=$id, rank=1).person\n" + )) + + self.assertTableData("Persons", cols="subset", data=self.people_data) + self.assertTableData("Games", cols="subset", data=self.games_data)