Should ScriptManager's call() method be removed?

[seisatsu]

Now with the new method of calling script functions introduced in 94e1441 as well as ScriptManager's module() method which had already existed, is there any use at all for the call() method?

[seisatsu]

Two issues I can see here.

• Without using call(), we don't log a message.
• Without using call(), we will crash if a script fails.

I don't know how to fix this while keeping the syntactical sugar.

[seisatsu]

I figured out how to encapsulate the new call technique to log messages and catch exceptions with the help of two new classes, ModuleGuard and CallGuard. The old call() function is just a wrapper now. It wasn't removed because some parts of the engine benefit from the way arguments are passed to it and conversion would make them messy.

[pmer]

BTW I think https://github.com/seisatsu/Driftwood/blob/94e14415c2e5299f2ea670c6c950b922e1a87448/src/scriptmanager.py#L90-L91 can be removed

The expression *args will always do the right thing there, even if args is an empty list or tuple or whatever it is.

[pmer]

Oh, just realized those lines were replaced by https://github.com/seisatsu/Driftwood/commit/901b827d68121072b6e714cd3dea9651b3f22fc3#diff-5714995ce01e01c17727f6e509403039R86. Well all three of those new lines can be replaced by the middle of the three because of the above logic.

[pmer]

Hmmmmmmmm, this seems fishy. I think the attempt is to make sure world scripts don't crash the engine, but there's also a little bit of an intent to make sure world scripts don't break other world scripts. The first of these two goals is do-able but the latter isn't.

For example:

# file1.py
Driftwood.script["file2.py"].C().broken()  # not caught—it's a method on a returned object
Driftwood.script["file2.py"].C(True)  # also not caught—it's a class

# file2.py

class C:
    def __init__(self, broken=False):
        if broken:
            raise Exception()
    def broken(self):
        raise Exception()

What if we only try to protect the engine from the game world? I think that would be less confusing. Otherwise I expect it will be hard to reason about when an error will be caught and when it won't. Besides, if an exception isn't caught by the caller it will be found at the nearest engine -> world control point anyway.

[pmer]

I really like the new syntax this introduces.

[seisatsu]

I don't quite understand your last post. I have no intention of preventing scripts from breaking each other though, only preventing the engine from crashing.

[pmer]

Sorry—you're right—it's not clearly worded. I'll try again!

The way it's implemented now achieves the goal of protecting the engine from broken world scripts.

But it also adds a try/catch when a world script calls another world script.

Since it only affects function calls made through the ModuleGuard and not others, this is confusing. Above are two examples where a world script calling another is not protected. That is to say, the current implementation changes the way exceptions are handled between world scripts sometimes but not all the time.

[seisatsu]

You shouldn't put code that can fail inside a constructor, and if you want to call a method of a class you should get the instance from ScriptManager and put it inside a variable first. We can't protect those special cases. This should be documented.

[pmer]

Regarding putting code that can fail in a constructor, consider the following:

class Item:
    def __init__(self, holder_entity):
        ...  # various code here
        holder_entity.inventory.append(self)

This simple code could fail if either holder_entity or holder_entity.inventory are None. Asking game authors to avoid such simple code seems unreasonable.

Or even:

class Item:
    def __init__(self, power_level):
        if power_level > 9000:
            self.is_meme = True

Regarding putting a class in a variable, I can't think of how that changes the above. Could you elaborate?

[seisatsu]

The purpose of the guard classes is to make a corridor to safely carry exceptions from callables out of the engine-level scope ScriptManager runs in and into the script's scope. Once the class instance belongs to a variable in the script, that scope and everything under it is protected and a failure inside an instance method will be handled safely. This doesn't apply to constructors though because they happen before the variable assignment.

[pmer]

I'm actually interested in a different case. I'd like to take a look at when a game author calls one of their own functions they wrote in another file.

[pmer]

I'd like there to be a ScriptManager.guarded_module that returns a ModuleGuard while ScriptManager.module returns an unguarded module. Engine code will use the ModuleGuard when calling any scripts, while scripts are recommended to use unguarded modules.

[seisatsu]

Loading a function or class out of a script has to pass through ScriptManager's scope, so unguarded exceptions here will crash the engine, even if the call was initiated from inside a script.

[pmer]

My apologies, but could you be more precise? I am having trouble picturing a situation causing an engine crash that my suggestion would allow to occur.

[seisatsu]

If we use our new syntax to call a function or constructor inside a module, that function or constructor is executed inside ScriptManager before its result is passed back to the calling script. Therefore, if it is unguarded the entire engine will crash because the exception occurs outside of the protected script scope.

The only time it's perfectly safe to pass an unguarded module is when a script just wants to assign the module to a variable, because it can own the module instance before calling anything inside.

[seisatsu]

Wait, I see what you mean. I'm not sure how to make that distinction happen with our current syntax. __getitem__ doesn't know if we're trying to grab an attribute from the module it returns or not, so it doesn't know whether or not to guard the result. (The engine never needs to just get a module, it's always calling something.)

[seisatsu]

What I could do is make _ModuleGuard's __module attribute public so that scripts can get the raw module out if they need it.

[pmer]

If, every time the engine calls a script it utilizes a ModuleGuard to do so, then no exception that occurs within a script will resuilt in an engine failure, regardless of whether scripts themselves utilize ModuleGuard while calling functions.

Consider the following:

# init.py

def init():
    try:
        Driftwood.script["random_map_generator"].generate(40, 40)
    except:
        Driftwood.script["random_map_generator"].generate(20, 20)

# random_map_generator.py

def generate(width, height):
    if width > 30:
        raise Exception("Too wide")

In the above example, I'd like for the exception to propage up to init.py and for a smaller map to be generated. However, if init.py receives a ModuleGuard then the guard will catch the exception and init.py will not find out.

[seisatsu]

See the post I made just before you posted, and the edit I made to the post you were replying to. I don't know how to do this automatically.

[pmer]

Then you have to go through extra work just to get normal Python exception behavior in scripts...

[seisatsu]

True. I'll need to think about this.

[pmer]

In the engine, we tend to use True/False return values to indicate success, but some Python programmers may prefer to use exceptions. I have seen such a preference in large production codebases and think it is accepted as decently Pythonic.

[pmer]

For instance, it's fairly common in Python to make your own exception classes

class CombatException(Exception):
    pass

and then only catch certain classes of exceptions in a try/catch

try:
    attack_player()  # some large or very complex behavior is in this function
except CombatException:
    ... handle error ...  # but we know how to recover from this type of exception safety

[pmer]

Well, as long as you make it public, even if it's not the default, I suppose that will work.

[seisatsu]

Fixed in 5694dea by raising exceptions through the guard classes.

[pmer]

@seisatsu Does that preserve the protection we want to offer engine code when it's calling scripts?

[seisatsu]

Good point. Latest commit changes the call() method to prevent exceptions from raising to crash the engine. All calls from inside the engine should use call() instead of the new syntax.

[pmer]

Sounds good!

[pmer]

Do we still need ModuleGuard and CallGuard?

[seisatsu]

Yes, I think we still need them to protect ScriptManager while it's executing a method or constructor called by a script before passing back to the script's scope. Otherwise we lose our new syntax and return to using call().

[pmer]

Why do we want to protect against that? In this example above, we don't want a script to be protected.

This one, too, if attack_player is from another module.

[seisatsu]

Because if an exception hits inside ScriptManager's scope the engine crashes immediately. That's the whole point of this mess. You have to push the exception through the guards to the script scope before it blows, or discard the exception with call().

[pmer]

If the engine puts a ScriptManager.call() in front of all script calls then I think we're good in the protection side.

Hmm, that's an interesting point about the syntax. Would it be bad to return a Module object directly?

[seisatsu]

I think I was mistaken about the syntax actually. This whole thing is confusing. We may not need the guards. Let me check.

[seisatsu]

So, I tested it, and without the guard classes no exceptions are raised through the calls and the engine takes no note if a script fails; no error is logged.

[pmer]

Let's add a log message to https://github.com/seisatsu/Driftwood/commit/040092cc8899be67d0b9c33cf72b7a752c600710#diff-5714995ce01e01c17727f6e509403039R92

[seisatsu]

Actually yes that's because the log calls are in the guards.

[seisatsu]

I think we're resolved now.

[pmer]

Hahah, yep, it certainly is confusing. :)

Here's another case to consider: passing information through exceptions.

# exceptions.py

class SaveException(Exception):
    def __init__(self, message):
        self.message = "Save game error: " + message

# save_files.py

def load_game(filename):
    ...
    if save_file_version != 4:
        raise SaveException("Wrong version number")

# main_menu.py

def load_game_button_clicked():
    try:
        Driftwood.script["save_files"].load_game("last_game.db")
    except SaveException e:
        display_error(e.message)

The message parameter will get lost if we don't pass the exact exception through. Also, calling the SaveException constructor with the wrong number to of arguments will raise a TypeError.

[pmer]

Cool, looks good to me.