From 6ca7c28157b8d9a714d8c0203ca8c72803c33bd8 Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sat, 7 Nov 2020 21:05:56 +1000 Subject: [PATCH 1/5] bpo-42282: Fold constants inside named expressions * The AST optimiser wasn't descending into named expressions, so any constant subexpressions weren't being folded at compile time * Remove "default:" clauses inside the AST optimiser code to reduce the risk of similar bugs passing unnoticed in future compiler changes --- .../2020-11-07-21-02-05.bpo-42282.M1W4Wj.rst | 3 ++ Python/ast_opt.c | 46 +++++++++++++++---- 2 files changed, 39 insertions(+), 10 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-11-07-21-02-05.bpo-42282.M1W4Wj.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-11-07-21-02-05.bpo-42282.M1W4Wj.rst b/Misc/NEWS.d/next/Core and Builtins/2020-11-07-21-02-05.bpo-42282.M1W4Wj.rst new file mode 100644 index 00000000000000..74f5c3362385c9 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-11-07-21-02-05.bpo-42282.M1W4Wj.rst @@ -0,0 +1,3 @@ +Optimise constant subexpressions that appear as part of named expressions +(previously the AST optimiser did not descend into named expressions). +Patch by Nick Coghlan. diff --git a/Python/ast_opt.c b/Python/ast_opt.c index 22ca6f23aefa30..111e97867d9a0c 100644 --- a/Python/ast_opt.c +++ b/Python/ast_opt.c @@ -63,8 +63,8 @@ fold_unaryop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) case NotIn: op = In; break; - default: - op = 0; + // No default case, so the compiler will emit a warning if new + // unary operators are added without being handled here } if (op) { asdl_seq_SET(arg->v.Compare.ops, 0, op); @@ -224,7 +224,7 @@ fold_binop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) PyObject *lv = lhs->v.Constant.value; PyObject *rv = rhs->v.Constant.value; - PyObject *newval; + PyObject *newval = NULL; switch (node->v.BinOp.op) { case Add: @@ -263,7 +263,15 @@ fold_binop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) case BitAnd: newval = PyNumber_And(lv, rv); break; - default: // Unknown operator + // No builtin constants implement the following operators + case MatMult: + break; + // No default case, so the compiler will emit a warning if new binary + // operators are added without being handled here + } + + // If no new value was calculated, there's nothing to do + if (newval == NULL) { return 1; } @@ -457,8 +465,11 @@ astfold_mod(mod_ty node_, PyArena *ctx_, _PyASTOptimizeState *state) case Expression_kind: CALL(astfold_expr, expr_ty, node_->v.Expression.body); break; - default: + // The following top level nodes don't participate in constant folding + case FunctionType_kind: break; + // No default case, so the compiler will emit a warning if new top level + // compilation nodes are added without being handled here } return 1; } @@ -567,8 +578,14 @@ astfold_expr(expr_ty node_, PyArena *ctx_, _PyASTOptimizeState *state) return make_const(node_, PyBool_FromLong(!state->optimize), ctx_); } break; - default: + case NamedExpr_kind: + CALL(astfold_expr, expr_ty, node_->v.NamedExpr.value); + break; + case Constant_kind: + // Already a constant, nothing further to do break; + // No default case, so the compiler will emit a warning if new expression + // kinds are added without being handled here } return 1; } @@ -686,8 +703,17 @@ astfold_stmt(stmt_ty node_, PyArena *ctx_, _PyASTOptimizeState *state) case Expr_kind: CALL(astfold_expr, expr_ty, node_->v.Expr.value); break; - default: - break; + // The following statements don't contain any subexpressions to be folded + case Import_kind: + case ImportFrom_kind: + case Global_kind: + case Nonlocal_kind: + case Pass_kind: + case Break_kind: + case Continue_kind: + break; + // No default case, so the compiler will emit a warning if new statement + // kinds are added without being handled here } return 1; } @@ -700,8 +726,8 @@ astfold_excepthandler(excepthandler_ty node_, PyArena *ctx_, _PyASTOptimizeState CALL_OPT(astfold_expr, expr_ty, node_->v.ExceptHandler.type); CALL_SEQ(astfold_stmt, stmt, node_->v.ExceptHandler.body); break; - default: - break; + // No default case, so the compiler will emit a warning if new handler + // kinds are added without being handled here } return 1; } From 45e3d50ae29fab97065a91fee1e4d9f98bbbe6d2 Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sat, 7 Nov 2020 21:28:38 +1000 Subject: [PATCH 2/5] Fix inverted comparison op folding --- Python/ast_opt.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Python/ast_opt.c b/Python/ast_opt.c index 111e97867d9a0c..f8417befd977cb 100644 --- a/Python/ast_opt.c +++ b/Python/ast_opt.c @@ -49,7 +49,7 @@ fold_unaryop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) of !=. Detecting such cases doesn't seem worthwhile. Python uses for 'is subset'/'is superset' operations on sets. They don't satisfy not folding laws. */ - int op = asdl_seq_GET(arg->v.Compare.ops, 0); + cmpop_ty op = asdl_seq_GET(arg->v.Compare.ops, 0); switch (op) { case Is: op = IsNot; @@ -63,6 +63,15 @@ fold_unaryop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) case NotIn: op = In; break; + // The remaining comparison operators can't be safely inverted + case Eq: + case NotEq: + case Lt: + case LtE: + case Gt: + case GtE: + op = 0; // The AST enums leave "0" free as an "unused" marker + break; // No default case, so the compiler will emit a warning if new // unary operators are added without being handled here } From 3addcc961af087d10c532bd80c2463a074db0a2a Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sat, 7 Nov 2020 21:46:50 +1000 Subject: [PATCH 3/5] Fix comment --- Python/ast_opt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/ast_opt.c b/Python/ast_opt.c index f8417befd977cb..467e31a7263809 100644 --- a/Python/ast_opt.c +++ b/Python/ast_opt.c @@ -73,7 +73,7 @@ fold_unaryop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) op = 0; // The AST enums leave "0" free as an "unused" marker break; // No default case, so the compiler will emit a warning if new - // unary operators are added without being handled here + // comparison operators are added without being handled here } if (op) { asdl_seq_SET(arg->v.Compare.ops, 0, op); From f8e52c0e002a893d0a0cce005f68c0f0ca009a4d Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sat, 7 Nov 2020 22:04:10 +1000 Subject: [PATCH 4/5] Still clear errors that occur in constant folding --- Python/ast_opt.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Python/ast_opt.c b/Python/ast_opt.c index 467e31a7263809..bb5dc663e32317 100644 --- a/Python/ast_opt.c +++ b/Python/ast_opt.c @@ -279,10 +279,8 @@ fold_binop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) // operators are added without being handled here } - // If no new value was calculated, there's nothing to do - if (newval == NULL) { - return 1; - } + // Even if no new value was calculated, make_const may still + // need to clear an error (e.g. for division by zero) return make_const(node, newval, arena); } From a4741aa27eb7f81016553d2be12630ee92db5ace Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sat, 7 Nov 2020 22:14:02 +1000 Subject: [PATCH 5/5] Minor code tweak (need to trigger new CI run anyway) --- Python/ast_opt.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Python/ast_opt.c b/Python/ast_opt.c index bb5dc663e32317..8c958ca7f13763 100644 --- a/Python/ast_opt.c +++ b/Python/ast_opt.c @@ -7,6 +7,8 @@ static int make_const(expr_ty node, PyObject *val, PyArena *arena) { + // Even if no new value was calculated, make_const may still + // need to clear an error (e.g. for division by zero) if (val == NULL) { if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) { return 0; @@ -274,14 +276,11 @@ fold_binop(expr_ty node, PyArena *arena, _PyASTOptimizeState *state) break; // No builtin constants implement the following operators case MatMult: - break; + return 1; // No default case, so the compiler will emit a warning if new binary // operators are added without being handled here } - // Even if no new value was calculated, make_const may still - // need to clear an error (e.g. for division by zero) - return make_const(node, newval, arena); }