Skip to content

Commit b324a3e

Browse files
durvesh1992meta-codesync[bot]
authored andcommitted
Fix strict-module is_mutable matching substrings of "mutable" (#123)
Summary: `is_mutable` in `compiler/strict/code_gen_base.py`: ```python def is_mutable(node: AST) -> bool: return isinstance(node, Name) and node.id in ("mutable") ``` `("mutable")` is a parenthesized string, **not a tuple** (no trailing comma), so `node.id in ("mutable")` is a **substring** test, not equality. Any bare-`Name` decorator whose id is a substring of `"mutable"` — e.g. `table`, `able`, `mut`, `mutabl`, `e`, even `@""` — wrongly matches. The sibling helper in the same package, `strict/preprocessor.py`, has the intended form: `node.id == "mutable"`. ### Impact `is_mutable` gates immutability/freezing of strict-module classes: - `find_immutability_flag` strips matching decorators: ```python node.decorator_list = [d for d in node.decorator_list if not is_mutable(d)] return old_size == len(node.decorator_list) ``` A class decorated with a substring-of-"mutable" name (notably `table`) has that decorator **silently dropped** from `decorator_list` (so the decorator never runs) and is reported as mutable, so it is **not frozen**. - `FindClassDef.visit_ClassDef` uses it to decide whether the module contains a freezable class, so detection is also wrong. ### Fix Use `== "mutable"`, matching `preprocessor.py`. One line. ### Tests Added `test_strict/test_is_mutable.py`: | decorator | before | after | |-----------|--------|-------| | `nutritious` | matches ✓ | matches ✓ | | `table`, `able`, `mut`, `e`, `@""` | **matches (bug)** | does not match ✓ | | `frozen` | no match ✓ | no match ✓ | Pull Request resolved: #123 Reviewed By: mpage Differential Revision: D110795208 Pulled By: alexmalyshev fbshipit-source-id: 7b1d0b540359fddb1050c1b989b6e49273e19e04
1 parent 0b234df commit b324a3e

2 files changed

Lines changed: 34 additions & 1 deletion

File tree

cinderx/PythonLib/cinderx/compiler/strict/code_gen_base.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929

3030

3131
def is_mutable(node: AST) -> bool:
32-
return isinstance(node, Name) and node.id in ("mutable")
32+
return isinstance(node, Name) and node.id == "mutable"
3333

3434

3535
class FindClassDef(NodeVisitor):
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
3+
# pyre-unsafe
4+
from __future__ import annotations
5+
6+
import ast
7+
import unittest
8+
9+
from cinderx.compiler.strict.code_gen_base import is_mutable
10+
11+
12+
class IsMutableTests(unittest.TestCase):
13+
def test_matches_only_mutable(self) -> None:
14+
self.assertTrue(is_mutable(ast.Name(id="mutable")))
15+
16+
def test_rejects_substrings_of_mutable(self) -> None:
17+
# Regression: `node.id in ("mutable")` was a substring test (the value
18+
# is a str, not a tuple), so any bare-Name decorator whose id is a
19+
# substring of "mutable" (e.g. @table, @able, @mut, @e) wrongly matched
20+
# and was stripped from the class, disabling freezing.
21+
for name in ("table", "able", "mut", "mutabl", "utable", "e", ""):
22+
with self.subTest(name=name):
23+
self.assertFalse(is_mutable(ast.Name(id=name)))
24+
25+
def test_rejects_unrelated_names(self) -> None:
26+
self.assertFalse(is_mutable(ast.Name(id="frozen")))
27+
28+
def test_rejects_non_name_nodes(self) -> None:
29+
self.assertFalse(is_mutable(ast.Constant(value="mutable")))
30+
31+
32+
if __name__ == "__main__":
33+
unittest.main()

0 commit comments

Comments
 (0)