From f51c0f4db3c733bf731eb6d12a4990a920d1a089 Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 18:41:45 +0100 Subject: [PATCH 1/7] Pull out a helper for checking whether a candidate name is allowed This will make the following changes clearer. --- src/fluent_compiler/codegen.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index 59e3ff8..d972995 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -160,15 +160,21 @@ def _add(final): # take into account parent scope when assigning names. used = self.all_reserved_names() + # We need to also protect against using keywords ('class', 'def' etc.) # i.e. count all keywords as 'used'. # However, some builtins are also keywords (e.g. 'None'), and so # if a builtin is being reserved, don't check against the keyword list if not is_builtin: used = used | set(keyword.kwlist) - while attempt in used: + + def _is_name_allowed(name: str) -> bool: + return name not in used + + while not _is_name_allowed(attempt): attempt = cleaned + str(count) count += 1 + return _add(attempt) def reserve_function_arg_name(self, name): From 19309c8200d92ca81bb9d7122e36a4a522f0a54e Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 18:43:26 +0100 Subject: [PATCH 2/7] Use keyword.iskeyword rather than expanding the set of reserved names Prior to this change, if we were checking whether a non-builtin name was allowed, we would expand the set of reserved names to include all keywords. This change switches to using the iskeyword function instead, which will allow us to stop using the set of reserved names altogether in a future commit. --- src/fluent_compiler/codegen.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index d972995..2095988 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -161,14 +161,14 @@ def _add(final): used = self.all_reserved_names() - # We need to also protect against using keywords ('class', 'def' etc.) - # i.e. count all keywords as 'used'. - # However, some builtins are also keywords (e.g. 'None'), and so - # if a builtin is being reserved, don't check against the keyword list - if not is_builtin: - used = used | set(keyword.kwlist) - def _is_name_allowed(name: str) -> bool: + # We need to also protect against using keywords ('class', 'def' etc.) + # i.e. count all keywords as 'used'. + # However, some builtins are also keywords (e.g. 'None'), and so + # if a builtin is being reserved, don't check against the keyword list + if (not is_builtin) and keyword.iskeyword(name): + return False + return name not in used while not _is_name_allowed(attempt): From 498cbb6ac2e3d52e22414f7890e2aa964abf831c Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 18:36:38 +0100 Subject: [PATCH 3/7] Add queries to check is a name is reserved Prior to this change, code which was checking whether a name was reserved in a given scope would need to fetch the relevant set of names and check whether the name was in it directly. This change adds query functions which allow the checks to be performed directly. The query functions recurse to the parent scope, rather than needing to fetch a new set containing this scope's names and the parent scope's. --- src/fluent_compiler/codegen.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index 2095988..12424ad 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -122,15 +122,36 @@ def names_in_use(self): names = names | self.parent_scope.names_in_use() return names + def is_name_in_use(self, name: str) -> bool: + if name in self.names: + return True + + if self.parent_scope is None: + return False + + return self.parent_scope.is_name_in_use(name) + def function_arg_reserved_names(self): names = self._function_arg_reserved_names if self.parent_scope is not None: names = names | self.parent_scope.function_arg_reserved_names() return names + def is_name_reserved_function_arg(self, name: str) -> bool: + if name in self._function_arg_reserved_names: + return True + + if self.parent_scope is None: + return False + + return self.parent_scope.is_name_reserved_function_arg(name) + def all_reserved_names(self): return self.names_in_use() | self.function_arg_reserved_names() + def is_name_reserved(self, name: str) -> bool: + return self.is_name_in_use(name) or self.is_name_reserved_function_arg(name) + def reserve_name(self, requested, function_arg=False, is_builtin=False, properties=None): """ Reserve a name as being in use in a scope. From 9151edcd6c950e7830b4a565c211ceaf45110501 Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 18:56:29 +0100 Subject: [PATCH 4/7] Replace uses of all_reserved_names with is_name_reserved --- src/fluent_compiler/codegen.py | 8 +++----- tests/test_codegen.py | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index 12424ad..2b70c73 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -170,7 +170,7 @@ def _add(final): if requested in self.function_arg_reserved_names(): assert requested not in self.names_in_use() return _add(requested) - if requested in self.all_reserved_names(): + if self.is_name_reserved(requested): raise AssertionError(f"Cannot use '{requested}' as argument name as it is already in use") cleaned = cleanup_name(requested) @@ -180,8 +180,6 @@ def _add(final): # To avoid shadowing of global names in local scope, we # take into account parent scope when assigning names. - used = self.all_reserved_names() - def _is_name_allowed(name: str) -> bool: # We need to also protect against using keywords ('class', 'def' etc.) # i.e. count all keywords as 'used'. @@ -190,7 +188,7 @@ def _is_name_allowed(name: str) -> bool: if (not is_builtin) and keyword.iskeyword(name): return False - return name not in used + return not self.is_name_reserved(name) while not _is_name_allowed(attempt): attempt = cleaned + str(count) @@ -207,7 +205,7 @@ def reserve_function_arg_name(self, name): # To keep things simple, and the generated code predictable, we reserve # names for all function arguments in a separate scope, and insist on # the exact names - if name in self.all_reserved_names(): + if self.is_name_reserved(name): raise AssertionError(f"Can't reserve '{name}' as function arg name as it is already reserved") self._function_arg_reserved_names.add(name) diff --git a/tests/test_codegen.py b/tests/test_codegen.py index d6b9018..01ebb13 100644 --- a/tests/test_codegen.py +++ b/tests/test_codegen.py @@ -65,7 +65,7 @@ def test_reserve_name_function_arg(self): scope.reserve_function_arg_name("arg_name") scope.reserve_name("myfunc") func = codegen.Function("myfunc", args=["arg_name"], parent_scope=scope) - self.assertNotIn("arg_name2", func.all_reserved_names()) + self.assertFalse(func.is_name_reserved("arg_name2")) def test_reserve_name_nested(self): parent = codegen.Scope() From af9cd54d256e2af948a96d7dd283b9257995e351 Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 18:59:36 +0100 Subject: [PATCH 5/7] Replace callers of function_arg_reserved_names with is_name_reserved_function_arg --- src/fluent_compiler/codegen.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index 2b70c73..ec40a9d 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -167,7 +167,7 @@ def _add(final): return final if function_arg: - if requested in self.function_arg_reserved_names(): + if self.is_name_reserved_function_arg(requested): assert requested not in self.names_in_use() return _add(requested) if self.is_name_reserved(requested): From 72442eb9e7c63ab901ee9c4dcbc0e5a1b6c46a82 Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 19:03:40 +0100 Subject: [PATCH 6/7] Replace all callers of names_in_use with is_names_in_use --- src/fluent_compiler/codegen.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index ec40a9d..4410dcf 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -168,7 +168,7 @@ def _add(final): if function_arg: if self.is_name_reserved_function_arg(requested): - assert requested not in self.names_in_use() + assert not self.is_name_in_use(requested) return _add(requested) if self.is_name_reserved(requested): raise AssertionError(f"Cannot use '{requested}' as argument name as it is already in use") @@ -332,7 +332,7 @@ def add_assignment(self, name, value, allow_multiple=False): x = value """ - if name not in self.scope.names_in_use(): + if not self.scope.is_name_in_use(name): raise AssertionError(f"Cannot assign to unreserved name '{name}'") if self.scope.has_assignment(name): @@ -391,7 +391,7 @@ def __init__(self, name, args=None, parent_scope=None, source=None): if args is None: args = () for arg in args: - if arg in self.names_in_use(): + if self.is_name_in_use(arg): raise AssertionError(f"Can't use '{arg}' as function argument name because it shadows other names") self.reserve_name(arg, function_arg=True) self.args = args @@ -687,7 +687,7 @@ class VariableReference(Expression): child_elements = [] def __init__(self, name, scope): - if name not in scope.names_in_use(): + if not scope.is_name_in_use(name): raise AssertionError(f"Cannot refer to undefined variable '{name}'") self.name = name self.type = scope.get_name_properties(name).get(PROPERTY_TYPE, UNKNOWN_TYPE) @@ -708,7 +708,7 @@ class FunctionCall(Expression): child_elements = ["args", "kwargs"] def __init__(self, function_name, args, kwargs, scope, expr_type=UNKNOWN_TYPE): - if function_name not in scope.names_in_use(): + if not scope.is_name_in_use(function_name): raise AssertionError(f"Cannot call unknown function '{function_name}'") self.function_name = function_name self.args = list(args) From a9d5d6978d231906b0acbaff94dc760d8926a693 Mon Sep 17 00:00:00 2001 From: James Owen Date: Sun, 26 May 2024 19:04:15 +0100 Subject: [PATCH 7/7] Remove set generation methods from Scope Prior to this change, we had replaced all callers with the query equivalents. This provides a considerable performance improvement when compiling large files, as we are no longer repeatedly creating new sets. This change removes the unused methods which generated the sets as they are no longer needed (and are a footgun). --- src/fluent_compiler/codegen.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/fluent_compiler/codegen.py b/src/fluent_compiler/codegen.py index 4410dcf..f632545 100644 --- a/src/fluent_compiler/codegen.py +++ b/src/fluent_compiler/codegen.py @@ -116,12 +116,6 @@ def __init__(self, parent_scope=None): self._properties = {} self._assignments = {} - def names_in_use(self): - names = self.names - if self.parent_scope is not None: - names = names | self.parent_scope.names_in_use() - return names - def is_name_in_use(self, name: str) -> bool: if name in self.names: return True @@ -131,12 +125,6 @@ def is_name_in_use(self, name: str) -> bool: return self.parent_scope.is_name_in_use(name) - def function_arg_reserved_names(self): - names = self._function_arg_reserved_names - if self.parent_scope is not None: - names = names | self.parent_scope.function_arg_reserved_names() - return names - def is_name_reserved_function_arg(self, name: str) -> bool: if name in self._function_arg_reserved_names: return True @@ -146,9 +134,6 @@ def is_name_reserved_function_arg(self, name: str) -> bool: return self.parent_scope.is_name_reserved_function_arg(name) - def all_reserved_names(self): - return self.names_in_use() | self.function_arg_reserved_names() - def is_name_reserved(self, name: str) -> bool: return self.is_name_in_use(name) or self.is_name_reserved_function_arg(name)