From 0030a8af18118f7c70c841b6150050663312c4b0 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 8 Oct 2023 19:13:33 +0200 Subject: [PATCH] fix(unittest): check not capturing operands (#950) ## Summary Fix the value of some operand expressions to `not`, `==`, etc. within `check` calls not being captured, which would result in incorrect `check` failure/success. ## Details ### Issue description For displaying the value of the operands in case of a test failure, the `check` macro "captures" the values of the operands prior to evaluating the condition expression. This is implemented in `inspectArgs` by assigning the operand expressions to temporaries and then accessing the temporaries. Type expressions cannot be assigned to run-time variables, so they were guarded against, by testing for whether the nodes have `ntyTypeDesc` as the type kind. However, since the AST passed to `check` is untyped, this cannot work, but so far it did, due to an implementation bug of `typeKind`, where the value returned for `typeKind` called on a `NimNode` that has no type results in undefined behaviour. In this specific case, the bug resulted in the `notin {ntyTypeDesc}` expressions always to always evaluate to `false`, which in turn disabled capturing for some expressions. ### The solution The decision of whether to assign the expression can only happen when the expressions are typed. A two-phase macro could be used, but that would require larger changes to `check`, so a different solution is chosen: instead of an assignment, `inspectArgs` emits a call to the new `materialize` template, passing along a generated `nksTemplate` symbol. To access the materialized (or not) value, `inspectArgs` emits calls with the generated template symbol. When the `materialize` calls are later expanded, a template with the passed-long symbol is produced. Using two `materialize` overloads, one for `typedesc`s and for everything else, doesn't work at the moment: if the value expression is erroneous, no `materialize` template is invoked, which would leave the gensym'ed symbol in an improper state, later causing the compiler to crash when it attempts to use it during overload resolution. In addition, for improved clarity, `inspectArgs` is changed to use a `case` statement. --- lib/pure/unittest.nim | 54 ++++++++++--------- .../metaprogramming/tunittest_impure.nim | 54 +++++++++++++++++++ 2 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 tests/stdlib/metaprogramming/tunittest_impure.nim diff --git a/lib/pure/unittest.nim b/lib/pure/unittest.nim index a0a04312a57..bbef76189bd 100644 --- a/lib/pure/unittest.nim +++ b/lib/pure/unittest.nim @@ -683,6 +683,15 @@ proc untype(arg: NimNode): NimNode = for n in arg: result.add untype(n) +template materialize(val: untyped, name: untyped) = + ## Helper template for ``check``. Captures `val` and makes it accessible + ## through a template named `name`. + when val is typedesc: + template name(): untyped = val + else: + let tmp = val + template name(): untyped = tmp + macro check*(conditions: untyped): untyped = ## Verify if a statement or a list of statements is true. ## A helpful error message and set checkpoints are printed out on @@ -711,30 +720,27 @@ macro check*(conditions: untyped): untyped = ">=", "<", ">", "!=", "is", "isnot"]: for i in 1 ..< exp.len: - if exp[i].kind notin nnkLiterals: - let argStr = exp[i].toStrLit - let paramAst = exp[i] - if exp[i].kind == nnkIdent: - result.printOuts.add newCall(bindSym"print", argStr, paramAst) - if exp[i].kind in nnkCallKinds + {nnkDotExpr, nnkBracketExpr, nnkPar} and - (exp[i].typeKind notin {ntyTypeDesc} or $exp[0] notin ["is", "isnot"]): - let callVar = genSym(nskVar, "c") - result.assigns.add newVarStmt(callVar, paramAst) - result.check[i] = callVar - result.printOuts.add newCall(bindSym"print", argStr, callVar) - if exp[i].kind == nnkExprEqExpr: - # ExprEqExpr - # Ident "v" - # IntLit 2 - result.check[i] = exp[i][1] - if exp[i].typeKind notin {ntyTypeDesc}: - let arg = genSym(nskVar, "p") - result.assigns.add newVarStmt(arg, paramAst) - result.printOuts.add newCall(bindSym"print", argStr, arg) - if exp[i].kind != nnkExprEqExpr: - result.check[i] = arg - else: - result.check[i][1] = arg + let argAst = exp[i] + case exp[i].kind + of nnkLiterals: + discard "literals are pure; they can be used as is" + of nnkExprEqExpr: + # named argument: + # ExprEqExpr + # Ident "v" + # IntLit 2 + result.check[i] = exp[i][1] + else: + # the argument expression might have side-effects or be impure, + # meaning that it cannot be duplicated without changing -- it has to + # be materialized into a temporary + let access = genSym(nskTemplate, "c") + result.assigns.add newCall(bindSym"materialize", argAst, access) + # ``materialize`` injects a template with the given `access` symbol. + # Invoking the template then yields the value + result.check[i] = newCall(access) + result.printOuts.add: + newCall(bindSym"print", toStrLit(argAst), newCall(access)) case checked.kind of nnkCallKinds: diff --git a/tests/stdlib/metaprogramming/tunittest_impure.nim b/tests/stdlib/metaprogramming/tunittest_impure.nim new file mode 100644 index 00000000000..63757e41229 --- /dev/null +++ b/tests/stdlib/metaprogramming/tunittest_impure.nim @@ -0,0 +1,54 @@ +discard """ + targets: "c js vm" + knownIssue.vm: "std/streams is not supported" + description: ''' + Ensure that the values of impure expressions used as operands in + ``check`` expression are properly captured (assigned to + temporaries) and, in case of failure, displayed + ''' +""" + +import std/[unittest, sequtils, strutils, exitprocs] + +var + output: seq[string] ## set after a ``check`` failure + wasFailure: bool + +type MockFormatter = ref object of OutputFormatter + +method failureOccurred(f: MockFormatter, checkpoints: seq[string], _: string) {.gcsafe.} = + # only include the printed values + output = filterIt(checkpoints, "Check failed:" notin it) + wasFailure = true + +# we use our own formatter: +resetOutputFormatters() +addOutputFormatter(MockFormatter()) + +template verify(o: varargs[string]) = + ## Compares the output of the previous ``check`` failure against `o`. + doAssert wasFailure, "check didn't fail" + doAssert output == o, "got: " & $output + wasFailure = false + +proc modify(x: var int): var int = + inc x + x + +# --- the actual test cases + +var a = 0 +check a == modify(a) +verify "a was 0", "modify(a) was 1" + +check (inc a; a) == (inc a; a) +verify "\ninc a\na was 2", "\ninc a\na was 3" + +check [modify(a)] == [modify(a)] +verify "[modify(a)] was [4]", "[modify(a)] was [5]" + +check (modify(a),) == (modify(a),) +verify "(modify(a),) was (6,)", "(modify(a),) was (7,)" + +# reset the program result so that no test failure is reported +setProgramResult(0) \ No newline at end of file