testwhat icon indicating copy to clipboard operation
testwhat copied to clipboard

check_function fails when function call uses "..."

Open machow opened this issue 8 years ago • 1 comments

*** =solution

f <- function(a, b=1, ...) sum(a + b, ...)

*** =sct

f <- ex() %>% check_fun_def('f') %>% check_body() %>% check_function('sum', index=1)

message

"There is something wrong in the following function call sum(a + b, …): … Used in a situation where it does not exist"

edit: it looks like ... is discussed here, but it may just be about functions that use ... in their signature.

machow avatar Dec 23 '16 16:12 machow

After investigating this, I found the underlying issue:

  • When doing check_function("sum"), you're looking for all usages of sum() through the parse data. In addition, standardize_call() will use match.call() to standardize the function call (because there are many ways to specify args in Python).
  • When ... is passed to a function, match.call() will look in the environment to figure out the structure of the ellipsis to do proper destructuring (see R source's C code), but the environment that match.call gets (the student/solution env, that does not contain a definition of ...) does not match perfectly the f() function's execution environment. Hence, it fails.
  • In this specific, check_function() is not completely environment-agnostic, while it ideally only depends on the student submission (text) and nothing more.

There are 3 ways to go about this:

  1. [hacky, but fast] add an option in check_function() that you don't want the standardization to happen. This is fine if you're not continuing your check_function() call with check_arg().
  2. [cleaner, reasonably fast] move the call standardization to when it is actually required (when doing check_arg()), but update the docs with the warning that your checking ability is limited (match.call will simply fail if you want to check arguments on a deeper level).
  3. [cleanest, crazy amount of work] add 'context/environment passing and specification' logic like pythonwhat did with set_context_vals to programmatically build sub-environments on the fly to simulate what's happening during runtime. This feature of pythonwhat caused loads of confusion for the people using it, so I don't think this is a good idea.

I'll probably run with option 2 when the time is right, but this is low-priority, as it's very rare, it's not blocking CD and this issue has been around for over 16 months. Iceboxing until I can't contain myself any longer.

filipsch avatar Apr 17 '18 14:04 filipsch