Guard against unused arguments in script interfae about espresso HOT 2 OPEN

I'm not totally confident we can implement such behavior in the script interface. It was originally designed to emulate the behavior of a Python def call_method(self, **kwargs) function. Users are free to pass extra unused arguments, just like they are free to assign extra unused class members (e.g. np.random.seed = 42 fiasco).

I can think of a few challenges to the deployment of the proposed solution:

1. Since C++11, class destructors are noexcept, thus throwing an exception invokes undefined behavior. The ISO C++ FAQ question How can I handle a destructor that fails? provides more details and nuance.
   • your code example triggers -Wterminate, which is always enabled in GCC (I wouldn't recommend disabling it)
   • we heavily rely on try/catch statements in the script interface: we are guaranteed to invoke UB somewhere
2. There are valid reasons for not reading all input arguments: some optional arguments defined in the Python code are redundant (or irrelevant) depending on which other arguments the user provided.
3. Such a class introduces a mutable state, which increases the complexity of the code. There are places in the script interface, like in NonBondedInteractionHandle, where we build a temporary variant map containing a subset of the original variant map. Should we copy the accessed_keys too? Or should we copy it by reference and not mark the copied values, so that we know when a value was accessed though the subset?
4. What happens when we read one argument of the variant map, and based on the value, forward the map to the script interface parent class call_method()? The argument is now marked as having been accessed, although it wasn't necessarily used to initialize data.
5. What should happen when we call count() or contains() to check for the presence of an argument? Currently it doesn't increment accessed_keys, but if we accidentally use find() to check for the presence of an argument, accessed_keys is incremented. This was common practice in ESPResSo, and while I did replace most of find() by count() in the C++20 PR, I probably missed one or two. Should we mark an argument as accessed, even though we didn't read its value? We cannot really know if the user is going to dereference the iterator, and writing a custom iterator class to detect dereference operations would introduce more complexity.

I've thought about this issue in the past, but couldn't find satisfactory solutions:

1. Write in a constexpr std::vector<std::unordered_set<std::string>> the list of all possible combinations of arguments and check the variant map satisfies one of them
   • won't work with optional arguments that are assigned default values at the C++ level
   • really easy to forget to update the vector when adding or renaming arguments
   • combinatorial explosion: see how the ParticleHandle class relies on contradicting_arguments_quat to handle the case where the user provides too many valid arguments
   • doesn't scale: one has to do that for every single class method, otherwise the user will assume there was no issue with the keyword arguments if no exception is raised by a method that forgot to implement this check
2. Use static analysis at the Python level to check keys **kwargs against the docstring
   • unclear how to fetch the docstring: one would have to go through the Python traceback to find the original class, which isn't a cheap operation
   • if it's a derived class, one has to also walk through the class inheritance hierarchy to find the relevant docstring, and this might fail if a docstring says See :class:`HydrodynamicInteraction` for the list of parameters

Can we find examples of Python projects where **kwargs are checked to see if they contain unexpected keys? In my experience, Python projects that do these checks only do it on a small fraction of all **kwargs functions. It would be helpful to see if there are projects that do so systematically, and whether they use tooling, for example to parse docstrings, or wrap the **kwargs in a special dict that implements the behavior proposed in the original post.

from espresso.

Maybe we could adapt the automatic class method generation to something like:

@script_interface_register
class Feature(ScriptInterfaceHelper):
   def foo(self, kT=None, gamma=None):
        kwargs = locals()
        del kwargs["self"]
        self.call_method("foo", **kwargs)

This way, unexpected arguments always raise an exception. Maybe there is a way to express that in a more compact form with a decorator. The danger with such a solution is that locals() returns all variables inside the function. That's good for capturing derived parameters (when their calculation is more convenient in Python than in C++), but temporary variables are also picked up, which can backfire on unsuspecting developers. For example if their type is not convertible to one of the types held in our Variant, an exception is raised. We would also have to deal with None values in C++.

Here is a MWE to check the output of locals() in a class method context:

import unittest
global_var = 5

class P:
    def foo(self, a, b=None):
        c = a
        print(locals())

P().foo(1, b=3)

Output:

{'self': <__main__.P object at 0xa88562fafe0>, 'a': 1, 'b': 3, 'c': 1}

from espresso.