godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.91k stars 20.16k forks source link

Exec/eval function #8003

Closed afftor closed 6 years ago

afftor commented 7 years ago

Suggestion: Give us exec/eval function to read from string. Puzzles me terribly we don't have it and workaround does not look very good to me.

bojidar-bg commented 7 years ago

@afftor Please, mention why you need such a function, since implementing it doesn't look very good to us :wink:. Probably, what you want is already available somehow....

afftor commented 7 years ago

I use dictionaries to store data with conditions stored as strings in them to check custom class object for any sort of attributes I might want. Naturally, I could probably make multiple entries for similar conditions and compare them in functions, but thing is there's really too many things regarding said class I might want to check in different ways.

Zylann commented 7 years ago

How about storing conditions as funcrefs?

afftor commented 7 years ago

@Zylann by that means making separate function for every condition check existing there? From what I get that's barely any different from just storing function name in string and calling 'call()' on it.

Zylann commented 7 years ago

What's the problem then? You want to execute code on stuff dynamically, so I don't feel eval() is needed here. What you need is either call(args), funcrefs, or lambdas (coming soon).

Also eval is:

afftor commented 7 years ago

Because I don't want to spam functions consisting only of single 'if' condition for every piece of entry I want to check upon. I might not really understand what funcref does exactly but I mostly trying to find more universal way which will take less time to implement.

Zylann commented 7 years ago

I suggest you learn what funcrefs and lambdas are, because you might not realize what you miss^^

funcrefs are functions stored in variables that you can store anywhere (including dictionaries), and call later without having to explicitely call "obj.the_function".

Lambdas are similar, functions defined on-the-fly that don't even need a name, and are able to capture variables of the function they are defined in. They can also be stored in variables themselves to be called later.

Do you have a concrete code example of what you are trying to achieve? Not saying that eval() should be banned, but I don't understand why you need it from what you explained so far.

afftor commented 7 years ago

https://hastebin.com/eqipupodim.cs this is basically what I'm trying to do right now.

Zylann commented 7 years ago
#------------------------------------------------------------------------------
# Assuming lambdas exist

var dict = {
    action1 = {reqs = func(obj): object.stat1 >= 1, action = 'foo'}
    action2 = {reqs = func(obj): object.stat2 < 3, action = 'bar'}
}

class objecttemplate:
    var stat1 = 0
    var stat2 = 1
    var stat3 = 0

func execute():
    var object = objecttemplate.new()
    for i in dict.values():
        if i.reqs(object) == true:
            print('success')
        else:
            print('failure')

#--------------------------------------------------------------------------------
# With current version (it's one way of doing it, there are plenty others)

# There might be a way not to detect actions automatically
# like using get_method_list or check_ functions, but I'll keep it simple
var actions = [
    "action1",
    "action2"
]

func action1(obj):
    if object.stat1 >= 1:
        print("Do action1")
        return true
    return false

func action2(obj):
    if object.stat1 < 3:
        print("Do action2")
        return true
    return false

func execute():
    var object = objecttemplate.new()
    for i in actions:
        if call(i, object):
            print("Success")
        else:
            print("Failure")

I understand why it feels quicker to use something like eval(), but having to fallback on eval() just for shorter writing is not a good design IMO (at least, for this use case). GDScript just needs a bit more dynamism regarding functions.

afftor commented 7 years ago

Well your solution does exactly what I'm trying to avoid: bloats code exponentially. It's even worse when I'd want to check different objects in same loop, so I'm still with my suggestion.

Zylann commented 7 years ago

The "current" solution is not ideal indeed, but not more exponential than yours (and has lots of alternatives if you don't want functions in your class). It also does explicitely what you want to do with unchecked code put somewhere else. Also note how fine lambdas look^^ In all my developer life, working with many languages, I never ever needed eval() except when the player was expected to actually write code. I'm sure GDScript can support something better than this to make code shorter.

afftor commented 7 years ago

True, lambda does look like something I could use instead. Hopefully it won't take too long.

williamd1k0 commented 7 years ago

eval/exec functions are really hacky and very dangerous, avoid them always (except when the player was expected to actually write code²).

You can write your own evaluation method, like this simple example:

extends Node

var data = {
    'action1': {
        'reqs': ['stat1', '>=', 1],
        'action': 'foo'
    },
    'action2': {
        'reqs': ['stat2', '<', 3],
        'action': 'bar'
    }
}

class Evaluate:

    func evaluate(property, condition, value):
        print(get(property), ' ', condition, ' ', value)
        if condition == '==':
            return get(property) == value
        elif condition == '!=':
            return get(property) != value
        elif condition == '>':
            return get(property) > value
        elif condition == '>=':
            return get(property) >= value
        elif condition == '<':
            return get(property) < value
        elif condition == '<=':
            return get(property) <= value

class ObjectTemplate:
    extends Evaluate
    var stat1 = 0
    var stat2 = 1
    var stat3 = 0

func _ready():
    execute()

func execute():
    var obj_test = ObjectTemplate.new()
    for i in data.values():
        if obj_test.evaluate(i.reqs[0], i.reqs[1], i.reqs[2]):
            print(i.action, ' success')
        else:
            print(i.action, ' failure')

It's more simple and safe than arbitrary evaluation.

(and it works very well) image

:-1: -1 for eval/exec

razcore-rad commented 6 years ago

Indeed I'm also against eval/exec

akien-mga commented 6 years ago

The consensus seems to be not to implement this feature, so closing.

LikeLakers2 commented 6 years ago

I'm for an eval function. They aren't really dangerous unless you're really reckless with how you use the function. I've never had any issues implementing an eval function because I've always made sure access to it is restricted. It's not exactly hard to put in a if statement or two to do that, y'know.

I've figured out how to create an eval function within GDscript:

# Eval a single function
# Takes a string as a parameter.
func eval_code(code):
    var script = GDScript.new()
    script.source_code += "extends Node\n"
    script.source_code += "func _eval():\n\tpass"
    for line in code.split("\n"):
        script.source_code += "\n\t" + line
    # Reload the script since setting the source_code doesn't recompile it, according to the docs
    script.reload()
    return script._eval()

# Eval an entire script, minus the extends statement.
# This script must have a _eval() function as a starting point.
# Takes a string as a parameter.
func eval_script(script_code):
    var script = GDScript.new()
    script.source_code += "extends Node\n"
    script.source_code += script_code
    # Reload the script since setting the source_code doesn't recompile it, according to the docs
    script.reload()

    # For some reason this always returns false, even when it does have _eval()
    # EXCEPT for when you assign this script to a node first!
    #if script.has_method("_eval"):
    #   print("Result:" + str(script._eval()))
    #else:
    #   # If we're here, trying to execute _eval anyways won't work
    #   print("Error: Script has no _eval() function")
    return script._eval()

(Note that either, in their current states, will crash the game if given improper input.)

EDIT: Didn't see Akien's response until I posted mine. Woops!

Zylann commented 6 years ago

Have any singleton in your project? Boom, eval code has access to it, gets the tree, does whatever it wants with it :)

vnen commented 6 years ago

They aren't really dangerous unless you're really reckless with how you use the function.

Problem is: beginners usually don't know that and will use it recklessly (or at least not as safely as they think it is).

As @Zylann said earlier, it's not the only issue with eval. There is usually a better design you could follow and they are hard to debug.

Well, I'm guilty of making an expression evaluator by using scripts made on the fly (like what @LikeLakers2 described). But I'm fully aware of its implications (the way I use it compiles the expression once and run with different values multiple times, so it isn't as slow). Since it's not something easily accessible, people making this are bound to know what they're venturing into. With an eval function, people would use it without fully knowing the issues.

Also, I don't think it's easy to implement in the current system anyway.

LikeLakers2 commented 6 years ago

Problem is: beginners usually don't know that and will use it recklessly (or at least not as safely as they think it is).

You're right, they usually don't. Though I'm fairly sure anyone will pay attention to a "THIS IS A DANGEROUS FUNCTION" in the description, if we added that in.

Also, I don't think it's easy to implement in the current system anyway.

While it can't serve the same function as what the OP might want, what about a tool in the editor that will let the user execute arbitrary GDscript? Could be useful for testing out code for plugins, or simply automating processes if they want to change a lot of nodes in their scene in one fell swoop, with possibly even other uses.

vnen commented 6 years ago

@LikeLakers2 that sounds like #1613.

williamd1k0 commented 6 years ago

Plot twist? https://github.com/godotengine/godot/commit/934c641a15337201fe8877c6b28ee538882f5c2a

rredesigns commented 6 years ago

It would be AWESOME to have eval() if it works as it does in Python. I would use it to define variables evaluating strings.

With Python I made it to parse a .js file using eval, since it's arrays and dicts are formatted exactly the same way, it was pretty easy. To this day I don't know of any other method to do the same.

williamd1k0 commented 6 years ago

@rredesigns With Python I made it to parse a .js file using eval, since it's arrays and dicts are formatted exactly the same way, it was pretty easy.

Actually JavaScript doesn't have the same syntax for "dicts" as in Python. Anyway, you don't need to use eval to parse JavaScript objects/arrays in Python or GDScript. Both Python and GDScript can parse the JSON format natively.

# Python
import json
dictionary = json.loads('{"foo":"bar"}')
# GDScript
var dictionary = JSON.parse('{"foo":"bar"}').result
rredesigns commented 6 years ago

@williamd1k0 Please tell me you are kidding. -_-

I will make it short. Grab this Js file and parse it to convert it into a valid Python dict without using eval().

Take all the time you need. And if you manage to do that with GDScript too, I will put up an altar for you at the front of my house. ;)

williamd1k0 commented 6 years ago

@rredesigns I'm totally not kidding.

Grab this Js file and parse it to convert it into a valid Python dict without using eval().

This js file doesn't need anything special to be parsed into python/gdscript dict. This is just a JSON with a few disposable characters in the beginning.

# Python
import json

text = open('rate.js').read()
text = text[text.index('=')+1:].strip()
result = json.loads(text)
print(result)
# GDScript
extends Node

var f = File.new()
var result

func _ready():
    f.open('rate.js', f.READ)
    var text = f.get_as_text()
    text = text.substr(text.find('=')+1, text.length()).strip_edges()
    result = JSON.parse(text).result
    print(result)
rredesigns commented 6 years ago

Well, for this particular case it could be done that way. I suggested this one because it was the last of these scenarios I found myself using eval() for.

And even here, compare that code to this:

data = eval(response.text[17:])

No extra imports, no extra steps. Easier to read, easier to write. It seemed convenient back then. :)

I assume it would be equally easy if this existed in GDScript. I tried before with this same file and Json parsing, but I forgot I had to leave out the first bunch of characters, so it threw an error and nothing else.

I think I will use this untill there's a better way, thanks a ton.

felipetavares commented 6 years ago

@rredesigns In that particular case eval is not better in any way (but they are both hacky solutions; the real issue here is that your data should not be in a js file); @williamd1k0 code is just more robust than yours because it searches dynamically for the first = on the string (and also loads from a file in the disk, so it's unfair to compare 'steps').

You could just switch eval for json.loads, making no other changes (besides import json), and it would work just fine.

Please read up on why eval is not a good idea, it has been discussed above and there is plenty of material about that on the web.

rredesigns commented 6 years ago

@felipetavares I have no idea how eval() works internally to be honest, but if you say it is hacky, I will keep in mind.

Of course in an ideal world this would be a Json file and it would have worked as usual, but it wasn't the case, this .js was feeding a currency rates table in a web and I needed it for a Python bot. I was 100% certain that it was always formatted the same way, so my algorithm worked with that, and obviously I had nothing to do with the webpage so this .js is all I had to work with.

And of course, I know exactly how dangerous that is. I always do some checks on the strings to make sure they have only the characters they should have and nothing else, it's pretty basic stuff.

For the same reason I think this wouldn't be an issue for those who don't know how to use it, since, if they don't know how, then they will most likely never use it (untill they have this very specific need and you would expect they are aware of the implicancies).

I know it can be worked around, as I saw a while ago in this thread, but not all cases will be this easy and straightforward I'm afraid. Luckily, I don't use eval() that much, so is not urgent to me, yet it would be a nice feature to have.

vnen commented 6 years ago

I have no idea how eval() works internally to be honest

That's the main reason I'm against it: people wanting to use often don't understand the consequences. Again, you can always create a GDScript on-the-fly by creating a new instance and setting its source code, so there's one way to "eval" arbitrary code.

rredesigns commented 6 years ago

Ok, you have to get out of this "all programmers are potentially stupid" mentality. -_-

If people say they'd like a feature like this, it probably means they are familiarized with it's implementation in another language and know how to use it properly.

What I meant with the statement you quoted was, that I don't know how the interpreter is designed to implement eval() under the hood, not that I don't know the consequences of using it wrong.

LikeLakers2 commented 6 years ago

Ok, you have to get out of this "all programmers are potentially stupid" mentality. -_-

If people say they'd like a feature like this, it probably means they are familiarized with it's implementation in another language and know how to use it properly.

Thing is, it's very easy to create a security hole in your game using eval() if you don't know what you're doing -- and even people who think they know what they're doing might still create a security hole. I think that's what @vnen is getting at.

vnen commented 6 years ago

If people say they'd like a feature like this, it probably means they are familiarized with it's implementation in another language and know how to use it properly.

I wouldn't count on the "know how to use it properly". And it's not about "all programmers are potentially stupid", it's about the amount of beginners with no previous programming knowledge that Godot attracts. The problem is that while people requesting this know what it's used for, once it's added other people will try to use without that knowledge because they just found in the docs.

A lot of languages do without eval() just fine. As for parsing JS, GDScript is not really suitable for this task and eval() would be a convenient shortcut just because the syntax similarity that could fail in many cases.

Finally, a lot of cases that would make this useful can be solved by the new expression evaluator.

williamd1k0 commented 6 years ago

@rredesigns data = eval(response.text[17:]) No extra imports, no extra steps. Easier to read, easier to write. It seemed convenient back then. :)

Sorry but you are just being an one-liner that doesn't know what is an actual optimized/safe/cheap code. And it's not about "all programmers are potentially stupid"², but please stop trying to defend your point because you are totally wrong.

No extra imports

Importing modules knowing what it does is not a problem

no extra steps

Just don't be such a one-liner like that

Easier to read

[17:] is hard-coded, it's not easy to read

easier to write

Just don't be such a one-liner like that²

akien-mga commented 6 years ago

Well, this is going around in circles now, locking.