rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.79k stars 177 forks source link

Custom Syntax won't work #208

Closed TimUntersberger closed 4 years ago

TimUntersberger commented 4 years ago

Version 0.17.0

I was experimenting a bit with defining a custom syntax and I can't manage to create a working custom syntax.

implementation_func

fn implementation_func(
    engine: &Engine,
    scope: &mut Scope,
    mods: &mut Imports,
    state: &mut EvalState,
    lib: &Module,
    this_ptr: &mut Option<&mut Dynamic>,
    inputs: &[Expression],
    level: usize
) -> Result<Dynamic, Box<EvalAltResult>> {
    let expression = inputs.get(0).unwrap();
    let value = engine.eval_expression_tree(scope, mods, state, lib, this_ptr, expression, level)?;

    println!("{:?}", value);

    Ok(().into())
}

main

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let mut engine = Engine::new();

    engine.register_custom_syntax(
        &["test", "$expr$"],
        0,
        implementation_func
    )?;

    engine
        .eval::<Dynamic>(
            r#"
            test 0;
            "#,
        )
        .unwrap();

    Ok(())
}

error

memory allocation of 443792878928 bytes failederror: process didn't exit successfully: `target\debug\wwm.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

Running the code without the test 0; line results in the following error

error

error: process didn't exit successfully: `target\debug\wwm.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION)

If I remove the register_custom_syntax call the program exists successfully.

The syntax I provided as an example is very simple and the resulting DSL should more look like the following code snippet.

enable work_mode;
enable launch_on_startup;

set work_mode true;
set min_height 200;

bind "Alt+Enter" to launch("wt.exe");

for i in range(1,10) {
  let key = "Alt+" + i;
  bind key to change_workspace(i);
}

mode resize {
  bind "H" to resize("Left", 2);
  bind "Shift+H" to resize("Left", -2);
};

I am try to see whether I can use rhai as the configuration language for WWM

TimUntersberger commented 4 years ago

I also find it quite confusing that in the test a different signature is being used as in the documentation. Were there any significant changes in the last week?

Custom syntax now works even without the internals feature.

Sorry didn't see this comment. Is there any ETA on when this gets published and would it be possible to use the feature without internals in my case?

schungx commented 4 years ago

Sorry didn't see this comment. Is there any ETA on when this gets published and would it be possible to use the feature without internals in my case?

Likely 0.18.0, where the main feature is true closures.

In the meantime, you can pull directly from this repo.

Does your custom syntax work now? Or does it still crash?

EDIT: I put the following into the latest version and it works fine:

    fn implementation_func(
        engine: &Engine,
        context: &mut EvalContext,
        scope: &mut Scope,
        inputs: &[Expression],
    ) -> Result<Dynamic, Box<EvalAltResult>> {
        let expression = inputs.get(0).unwrap();
        let value = engine.eval_expression_tree(context, scope, expression)?;

        println!("{:?}", value);

        Ok(().into())
    }

    let mut engine = Engine::new();

    engine.register_custom_syntax(&["test", "$expr$"], 0, implementation_func)?;

    engine.consume("test 0;")?;
schungx commented 4 years ago

The syntax I provided as an example is very simple and the resulting DSL should more look like the following code snippet.

Rhai should handle your design just fine. However, there doesn't seem to be anything stopping you from implementing all of these as function calls, so your users do not need to learn yet another language...

import "WWM" as wwm;

wwm::enable(wwm::features::work_mode);
wwm::enable(wwm::features::launch_on_startup);

wwm::set(wwm::features::work_mode, true);
wwm::set(wwm::settings::min_height, 200);

wwm::bind("Alt+Enter", || wwm::launch("wt.exe"));

for i in range(1,10) {
  let key = "Alt+" + i;
  wwm::bind(key, || wwm::change_workspace(i));
}

wwm::mode(resize, || {
  wwm::bind("H", || wwm::resize("Left", 2));
  wwm::bind("Shift+H", wwm::resize("Left", -2));
});

Of course, you can say it doesn't look quite as clean as a DSL...

schungx commented 4 years ago

Another suggestion: Sometimes judicial usage of symbols instead of introducing more text keywords (leaving keywords only at the start) make things clearer:

enable work_mode;
enable launch_on_startup;

set work_mode = true;
set min_height = 200;

on_key "Alt+Enter" -> launch("wt.exe");

for i in range(1,10) {
  let key = "Alt+" + i;
  on_key key -> change_workspace(i);
}

mode resize {
  on_key "H" -> resize("Left", 2);
  on_key "Shift+H" -> resize("Left", -2);
};
TimUntersberger commented 4 years ago

Using my code on the latest release 0.17.0 doesn't work, but using your version on master works.

Thank you!

If you don't have any question regarding version 0.17.0 I will close the issue.

Edit:

Right now I am trying to register a function that tells me what kind of binding the user wants to map to. The problem is I don't really understand how I can return the enum in the registered function and whether I should use the ImmutableString given to me or create a new String.

Binding

enum Binding {
    Launch(ImmutableString)
}

Register

engine.register_fn("launch", |program: ImmutableString| -> Binding {
    Binding::Launch(program)
})
schungx commented 4 years ago

You can just return the enum if you want, as long as you #[derive(Clone)] on it. Rhai requires custom types to be clonable.

Usually, using custom types is an overkill. Just return an integer will be fastest. A custom type goes through a boxed trait object and is much slower than primary types.

If you need to return different data depending on different configurations, you can consider a map.

As for ImmutableString, if you can use it directly, it'll be faster as it is a shared immutable string. If not, returning String will simply create a fresh cloned copy of the string and return it to you.

Depending on your intended call frequency and need for speed.

Your code may look better like this:

engine.register_fn("launch", |program: &str| -> Binding {
    Binding::Launch(program.to_string())
});

or

engine.register_fn("launch", |program: String| -> Binding {
    Binding::Launch(program)
});
TimUntersberger commented 4 years ago

You are really helpful!

Should i post my questions somewhere else or can I just ask in this issue (I will close it for now)?

Is it possible to import modules without the as ... part?

I have the following structure image

Each script is just a list of statements that are not really exporting anything so having to think of a name just to import them is really annoying.

Right now the main file looks like this

import "keybindings" as kb;
import "rules" as r;
import "modes/resize" as mr;

Would it be possible to write it like this?

import "keybindings" as _;
import "rules" as _;
import "modes/resize" as _;

Or even this?

import "keybindings";
import "rules";
import "modes/resize";

I could define my own syntax, but this feels like something that I shouldn't do.

schungx commented 4 years ago

Is it possible to import modules without the as ... part?

The problem is... the module path may not be a valid variable name. What are we going to do when it is not?

schungx commented 4 years ago

Should i post my questions somewhere else or can I just ask in this issue (I will close it for now)?

Probably another issue if it is a different question!

TimUntersberger commented 4 years ago

The problem is... the module path may not be a valid variable name. What are we going to do when it is not?

I was thinking more like you don't really import the file, you only execute it (meaning the module doesn't get assigned to a variable).

schungx commented 4 years ago

I was thinking more like you don't really import the file, you only execute it (meaning the module doesn't get assigned to a variable).

Hhhhmmm... you mean use it to initialize the system or something? If you don't export anything, how do you use any feature inside the module?

This is probably better to register an initialization function to be called in the beginning of the script, like init_keybindings.

TimUntersberger commented 4 years ago

I was assuming that the imports work similiar to nodejs require.

Example

main.rhai

import "test" as asdf;

print("world");

test.rhai

print("hello");

stdout

hello
world

If you don't want this in rhai by default I will probably just have to implement a custom syntax for this.

Edit:

This probably won't be useful in a normal language context, but it is very conveniant for something like a config where the user almost only writes statements and not expression.

schungx commented 4 years ago

I was assuming that the imports work similiar to nodejs require.

Yes it does. It executes the script to construct a module. However, if you don't export anything in the script, then the net effect is that you've executed the child script and nothing else.

I think I understand what you're trying to do. You are essentially using import as a simple include statement to run child scripts. import is probably not the perfect keyword for this... use probably fits the purpose better.

However, what you want can be done. We can have modules imported but not under any variable name. That module will simply be executed but not referred to. This can be done quite easily.

I'll add that feature in.

schungx commented 4 years ago

If you pull from the latest drop in https://github.com/schungx/rhai, you can now omit the as part of the import statement.

TimUntersberger commented 4 years ago

Thank you that was really fast 😮

I currently am defining my own syntax for import "...", because I need to be able to have one scope for the whole engine.

I tried to implement my own ModuleResolver, but I got a lot of lifetime errors so I now just have the following defined

engine.register_custom_syntax(
    &["import", "$expr$"], 
    0, 
    |engine, ctx, scope, inputs| {
        let cwd = scope.get_value::<String>("__cwd").ok_or("Failed to get __cwd")?;
        let file_name = get_string!(engine, ctx, scope, inputs, 0) + ".rhai";

        let mut path = PathBuf::new();

        path.push(cwd);
        path.push(file_name);

        engine.consume_file_with_scope(scope, path)?;

        Ok(().into())
    },
)?;
CustomResolver One big problem I had, was that the `new_position` function is only public inside the crate, so I had to copy it over and change the return type from `Box` to `&mut Box` ```rust use rhai::{Engine, EvalAltResult, Module, ModuleResolver, Position, Scope, AST}; use std::{cell::RefCell, collections::HashMap, path::PathBuf}; /// This function is private in rhai. /// I just straight up copied it over. fn new_position(res: &mut Box, new_position: Position) -> &mut Box { if res.position().is_none() { res.set_position(new_position); } res } pub struct CustomResolver<'a> { path: PathBuf, extension: String, cache: RefCell>, scope: &'a Scope<'a>, } impl<'a> CustomResolver<'a> { pub fn new(scope: &'a Scope<'a>) -> Self { Self { path: PathBuf::default(), extension: String::from("rhai"), cache: RefCell::new(HashMap::new()), scope, } } } impl<'a> ModuleResolver for CustomResolver<'a> { fn resolve( &self, engine: &Engine, path: &str, pos: Position, ) -> Result> { // Construct the script file path let mut file_path = self.path.clone(); file_path.push(path); file_path.set_extension(&self.extension); // Force extension let ast = self .cache .borrow() .get(&file_path) .unwrap_or({ let ast = engine.compile_file(file_path.clone())?; // Put it into the cache self.cache.borrow_mut().insert(file_path, ast); &ast }); Module::eval_ast_as_new(self.scope, ast, engine) } } ```

The reason I need a global scope is because when I create the engine I initialize two variables __keybindings and __set. Whenever a bind ..... gets executed some metadata about the binding gets added to the __keybindings map and the same time for set and __set.

Is there a better way of having a global scope without having to mess with lifetimes a lot?

I tried using the real Config struct inside of the custom syntax callbacks, but rust told me that the config has to have static lifetime to not be outlived by the callback.

Maybe I still suck atlifetimes 🤷

Edit:

After looking at your latest commit I saw this new documentation where the module script contains initialization statements that puts the functions registered with the Engine into a particular state. Does this mean it uses the Scope of the file that contains the import ".." statement?

schungx commented 4 years ago

Your lifetime is tripping up Rhai because Rhai really doesn't like any type that has a non-'static reference in it. Your 'a lifetime is preventing it from being used. Rhai really doesn't want to work with references; it wants to work with solid data.

So, what you need to simply to give it data instead. Using RefCell (or Mutex if you need multi-threaded).

Check out the side-effects test to see an example on how to maintain a shared state outside Rhai that you can use Rhai to modify.

The steps you need are:

1) Create your type containing your data:

struct Config {
    pub keybindings: ???,
    pub set: ???
}

2) Create a shared version of it: Rc<RefCell<Config>>

3) Register the type: engine.register_with_name::<Rc<RefCell<Config>>>("Config");

4) Register methods: e.g. add_key_binding, set etc.

5) Clone the shared object and push it into your custom scope: scope.push_constant("config", config.clone());

6) You can then use config in your scripts, and setting properties on it will be reflected outside of Rhai.

schungx commented 4 years ago

After looking at your latest commit I saw this new documentation where the module script contains initialization statements that puts the functions registered with the Engine into a particular state. Does this mean it uses the Scope of the file that contains the import ".." statement?

If you want to use a custom scope for loading a module, you'll need to write your own custom syntax. What you have is fine, except that I'd suggest using the word use instead of import. You can then push the Config object into the scope before you run the script.

Otherwise, it is easy to actually register functions that capture the configuration object itself. For example:

let config = Rc::new(RefCell::new(Config { ... }));          // This is your main config object

let wrapper = config.clone();     // This is your wrapper for use in Rhai
engine.register("add_key_binding", move |key: &str, binding: ???|  wrapper.keybindings.insert(key.to_string(), binding));

// Calling add_key_binding in Rhai will modify config.

The move tells Rust that you want to move wrapper inside that closure. If you register your function this way with Rhai, add_key_binding can be used anywhere, even inside a module load. You no longer need a custom syntax. You can just do import "abc".

Check out the on_print docs and you'll see an example of this technique, using Arc<RwLock<...>>.

TimUntersberger commented 4 years ago

You have been really helpful! Thank you a lot!

Hopefully the last question I have.

Example

custom syntax

engine.register_custom_syntax(
    &["example", "$block$"],
    0,
    |engine, ctx, scope, inputs|  {
        println!("before");
        // somehow execute AST block .... 
        println!("after");

        Ok(().into())
    },
)?;

usage

example {
    print("hello world");
}

output right now

hello world
before
after

output I want

before
hello world
after

Is there any way too achieve this? Can I somehow tell the engine to not evaluate AST blocks when registering a custom syntax?

schungx commented 4 years ago

Strange, I tried the following and it worked just like you want it:

    let mut engine = Engine::new();

    engine.register_custom_syntax(&["example", "$block$"], 0, |engine, ctx, scope, inputs| {
        println!("before");
        engine.eval_expression_tree(ctx, scope, &inputs[0])?;
        println!("after");

        Ok(().into())
    })?;

    engine.consume("example { print(42); }")?;

prints:

before
42
after

The expression block is never evaluated. It is passed into your custom syntax closure as an AST sub-tree.

TimUntersberger commented 4 years ago

I noticed that I accidently evaluted the block twice. Sorry.