diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000000..63356c599c --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,50 @@ +# Reproduction Process + +Observations: + - To repoduce the issue, I first commented out the call to get() in line 1487 and replaced it with a call to flag instead. + - The terminal blanks and doesn't output anything when the flag() method is called. + - I expect the computer to know what to do when it is given a function like flag() and output the correct result. + +Environment Setup: I did not face any problems during the environment setup process as I have been using. + +Steps to Reproduce: + 1. Open the file case_validator.py using the file path toolchain/mfc/case_validator.py + 2. Go to line 1487 in the check_chemistry() method + 3. Replace the value assigned to the chemistry variable with self.flag("chemistry") + 4. Reproduce the issue on your terminal by running the following: python3 toolchain/mfc/gen_case_constraints_docs.py | grep "ANALYZER" + +Branch Link: https://github.com/mansibrahman03/MFlowCode/tree/main + +# Solution Approach + +Implementation: + +1. At the moment, case_validator.py reads boolean case flags with the verbose form self.get("name", "F") == "T" in 134 places. To clean this code up I want to implement a map function. However, I am prevented from doing so at the moment because of the hard coding in ast_analyzer.py in the _build_local_param_map() method in line 232: call.func.attr == "get". Thus, when the analyzer sees the word "flag", it doesn't recognize it and blanks out. + +2. My solution is to first make sure the analyzer recognizes flag() by adding a conditional statement in _build_local_param_map() in ast_analyzer.py. I will then implement a flag() method in case_validator.py that maps the verbose form to a simple boolean value. + +3. Files I will touch include: + - case_validator.py + - ast_analyzer.py + +4. I will verify my approach works by calling flag() instead of get() in line 1487 of case_validator.py. If the analyzer correctly returns a value instead of blank, it successfully recognized the flag() function. I will also test the flag() method itself to ensure it correctly returns true or false. + +# Implementation Notes + +Implementation Progress: I fixed _build_local_param_map() in ast_analyzer.py to recognize flag() method and added a flag() helper method in case_validator.py. + +Challenges Faced: The main challenge was understanding the codebase. + +Testing Strategy: I tested the flag method itself to ensure it correctly return a bool for all possible params (T, F, null). I also test to see that the old broken code fails and the new code works. + +Branch Link: https://github.com/mansibrahman03/MFlowCode/tree/main + +# PR + +PR Link: [Direct link to your submitted pull request](https://github.com/MFlowCode/MFC/pull/1622) + +PR Description: I added a flag() method in toolchain/mfc/case_validator.py that functions to replace the verbose idiom "self.get(x,F)==T" repeated in 134 places across the file. I also updated ast_analyzer.py to recognize calls made to flag(). + +Maintainer Feedback: I am currently waiting for maintainer to respond. + +Status: Awaiting review diff --git a/toolchain/mfc/case_validator.py b/toolchain/mfc/case_validator.py index 4039cc94ad..a134ba9cab 100644 --- a/toolchain/mfc/case_validator.py +++ b/toolchain/mfc/case_validator.py @@ -246,6 +246,10 @@ def __init__(self, params: Dict[str, Any]): def get(self, key: str, default=None): """Get parameter value with default""" return self.params.get(key, default) + + def flag(self, name: str) -> bool: + """True if and only if the case sets `name` to "T".""" + return self.get(name, "F") == "T" def is_set(self, key: str) -> bool: """Check if parameter is set (not None and present)""" @@ -1483,8 +1487,7 @@ def check_chemistry(self): is provided. No static validation is performed here - chemistry will fail at runtime if misconfigured. """ - # Fetch global chemistry and diffusion flags - chemistry = self.get("chemistry", "F") == "T" + chemistry = self.flag("chemistry") diffusion = self.get("chem_params%diffusion", "F") == "T" num_fluids = self.get("num_fluids") diff --git a/toolchain/mfc/params/ast_analyzer.py b/toolchain/mfc/params/ast_analyzer.py index ad7d94fd42..e58e001579 100644 --- a/toolchain/mfc/params/ast_analyzer.py +++ b/toolchain/mfc/params/ast_analyzer.py @@ -229,12 +229,13 @@ def _build_local_param_map(self, func: ast.FunctionDef) -> Dict[str, str]: isinstance(call.func, ast.Attribute) and isinstance(call.func.value, ast.Name) and call.func.value.id == "self" - and call.func.attr == "get" + and call.func.attr in ("get", "flag") and call.args and isinstance(call.args[0], ast.Constant) and isinstance(call.args[0].value, str) ): param_name = call.args[0].value + print(f"Issue reproduced: {param_name}") for target in node.targets: if isinstance(target, ast.Name): m[target.id] = param_name diff --git a/toolchain/mfc/params_tests/test_flag_helper.py b/toolchain/mfc/params_tests/test_flag_helper.py new file mode 100644 index 0000000000..a685481cd8 --- /dev/null +++ b/toolchain/mfc/params_tests/test_flag_helper.py @@ -0,0 +1,76 @@ +""" +Tests for CaseValidator.flag() and ast_analyzer._build_local_param_map fix. +""" + +import ast +import unittest + +from ..case_validator import CaseValidator +from ..params.ast_analyzer import CaseValidatorAnalyzer + + +def _param_map(source: str) -> dict: + """Parse a single method body and return its local-param map.""" + wrapped = "class _V:\n" + "\n".join(" " + l for l in source.splitlines()) + func = ast.parse(wrapped).body[0].body[0] + a = CaseValidatorAnalyzer.__new__(CaseValidatorAnalyzer) + return a._build_local_param_map(func) + + +class TestFlag(unittest.TestCase): + + def test_true_when_set_to_T(self): + self.assertTrue(CaseValidator({"chemistry": "T"}).flag("chemistry")) + + def test_false_when_set_to_F(self): + self.assertFalse(CaseValidator({"chemistry": "F"}).flag("chemistry")) + + def test_false_when_absent(self): + self.assertFalse(CaseValidator({}).flag("chemistry")) + + def test_only_uppercase_T_is_truthy(self): + """Lowercase, integers, and booleans must not count as set.""" + for val in ("t", "True", 1, True, None): + self.assertFalse(CaseValidator({"x": val}).flag("x"), f"Expected False for {val!r}") + + def test_returns_bool(self): + self.assertIsInstance(CaseValidator({"x": "T"}).flag("x"), bool) + + +class TestASTCoupling(unittest.TestCase): + + def test_flag_call_recorded_by_analyzer(self): + """self.flag('x') must register 'x' in the param map, same as self.get('x','F')=='T'.""" + m = _param_map("def check(self):\n chemistry = self.flag('chemistry')") + self.assertEqual(m.get("chemistry"), "chemistry") + + def test_get_call_still_recorded(self): + """Existing self.get() pattern must not regress.""" + m = _param_map("def check(self):\n d = self.get('chem_params%diffusion', 'F') == 'T'") + self.assertEqual(m.get("d"), "chem_params%diffusion") + + def test_old_matcher_would_have_missed_flag(self): + """Demonstrate the pre-fix bug: attr=='get' silently dropped flag() calls.""" + source = "def check(self):\n chemistry = self.flag('chemistry')" + wrapped = "class _V:\n" + "\n".join(" " + l for l in source.splitlines()) + func = ast.parse(wrapped).body[0].body[0] + + broken = {} + for node in ast.walk(func): + if isinstance(node, ast.Assign): + v = node.value.left if isinstance(node.value, ast.Compare) else node.value + if isinstance(v, ast.Call) and isinstance(v.func, ast.Attribute): + if v.func.value.id == "self" and v.func.attr == "get": # old check + for t in node.targets: + if isinstance(t, ast.Name): + broken[t.id] = v.args[0].value + + self.assertNotIn("chemistry", broken, "Old matcher should miss flag() — that was the bug") + + a = CaseValidatorAnalyzer.__new__(CaseValidatorAnalyzer) + fixed = a._build_local_param_map(func) + self.assertIn("chemistry", fixed, "Fixed matcher must capture flag() calls") + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file