fix(optuna): harden literal parsing across both desubstitution paths#7
Open
GrigoryEvko wants to merge 1 commit into
Open
fix(optuna): harden literal parsing across both desubstitution paths#7GrigoryEvko wants to merge 1 commit into
GrigoryEvko wants to merge 1 commit into
Conversation
The AST-level `_EvalCleaner` checked `isinstance(arg, (Constant, List, Tuple, Set, Dict))` to decide whether an `eval(...)` argument is a literal. This missed `UnaryOp(USub, Constant)`, so `eval(-5)` survived in the AST path while the source-level `_clean_eval_in_source` stripped it correctly. Switch to `ast.literal_eval` as the predicate so both paths share the same logic. Also broaden the `(ValueError, SyntaxError)` except clauses around `literal_eval` in `_coerce_param_value` and `_clean_eval_in_source` — it can also raise `MemoryError`, `RecursionError`, and `TypeError` on pathological inputs. Verified against the source-level path on 12 sample inputs; both produce identical output post-fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The AST-level
_EvalCleaner(add_tuned_comment=Falsepath) checksisinstance(arg, (Constant, List, Tuple, Set, Dict))to decide whether aneval(...)call wraps a literal that can be inlined. This missesUnaryOp(USub, Constant), soeval(-5)survives in the AST path while the source-level_clean_eval_in_sourcestrips it correctly viaast.literal_eval. The two desubstitution modes silently produced different output for the same input.Reproducer against current main:
Fix: in
_EvalCleaner.visit_Call, replace the isinstance check withast.literal_eval(arg)as a try/except predicate so both paths share the same literal-detection logic. Verified across 12 sample inputs — the AST and source paths now produce identical output. Also broaden the(ValueError, SyntaxError)catches aroundliteral_evalin_coerce_param_valueand_clean_eval_in_sourceto also catchMemoryError,RecursionError, andTypeError.