Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(unittest): check not capturing operands #950

Merged
merged 2 commits into from
Oct 8, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Oct 8, 2023

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 typedescs 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.

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.

Resolution
----------

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.
@zerbina zerbina added bug Something isn't working stdlib Standard library labels Oct 8, 2023
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

wrt longer term solution, I think stuff like this should be more generics + comptime (similar to how one might do it in Zig) than macro heavy.

@saem
Copy link
Collaborator

saem commented Oct 8, 2023

/merge

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Oct 8, 2023
Merged via the queue into nim-works:devel with commit 0030a8a Oct 8, 2023
18 checks passed
@zerbina zerbina deleted the unittest-fix-check-bug branch October 12, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stdlib Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants