Breaking changes on patch versions

[SuperCuber]

According to semver, on packages with major version 0, anything may change at any time so nothing should be considered stable, however most crates.io packages use the convention that the numbers are "shifted" - so 0.x.y+1 should not introduce breaking changes from 0.x.y, similar to how x.y+1.* should not break from x.y.* This is not the case for 0.19.0 -> 0.19.1, as there are a number of breaking changes.

I suggest following the crates.io convention on this matter, since cargo updateing my code broke it from compiling.

---

Unrelated, but some (array) is usually called any in other languages, and even in rust itself - in #[cfg(any(flag1, flag2))]

none is interesting but obviously it's just !any so many languages don't have it for the same reason they don't have unless (if not) - it's redundant.

---

Comments became a bit scattered so I'll put everything discussed:

• [x] Patch version breaking changes
• [x] none (array)
• [x] eval_ast_as_new not merging namespaces correctly
• [ ] non caching FileModuleResolver option

[schungx]

Well, Rhai is still in a heavy developmental phase so it is sorta hard not to introduce breaking changes in any version... regardless of how small...

[SuperCuber]

Cargo by default disagrees: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#caret-requirements

While SemVer says there is no compatibility before 1.0.0, Cargo considers 0.x.y to be compatible with 0.x.z, where y ≥ z and x > 0.

I'm not saying that you absolutely have to follow this convention; People can just specify a specific patch version. But I think it's something to consider because most people will expect it.

[schungx]

none is interesting but obviously it's just !any so many languages don't have it for the same reason they don't have unless (if not) - it's redundant.

Good point on none... I think I'll remove it in that case. some, however, is JavaScript (but I guess I didn't follow their every...). The reason I picked some is because all and any both start with a and is not easily discernible; all and some are more distinct...

[schungx]

I'm not saying that you absolutely have to follow this convention; People can just specify a specific patch version. But I think it's something to consider because most people will expect it.

Well, sometimes breaking changes are to very rarely used API's that do not have significant impacts to code outside. In such cases, I suppose it is OK for 0 versions to have breaking changes even on patch level, otherwise the minor version numbers will be shooting up very fast, as almost every new version will have some small breaking changes...

[SuperCuber]

API's that do not have significant impacts to code outside

I think 10 bullet points of breaking changes to rust apis (on module and ast related stuff, but still) and 3 rhai apis definitely is enough to mark as a breaking change, although I do agree in the general case (especially on 0.*)

[SuperCuber]

Also, seems like Module::eval_ast_as_new doesn't correctly default merge_namespaces=true, tested using this:

extern crate rhai;

use rhai::{Engine, Module, Scope};

const MODULE_TEXT: &str = r#"
fn run_function(function) {
    function.call()
}
"#;

const SCRIPT: &str = r#"
    import "test_module" as test;
    fn foo() {
        print("foo");
    }
    test::run_function(Fn("foo"));
"#;

fn main() {
    let mut engine = Engine::new();
    let module_ast = engine.compile(MODULE_TEXT).unwrap();
    let module = Module::eval_ast_as_new(Scope::new(), &module_ast, &engine).unwrap();
    let mut static_modules = rhai::module_resolvers::StaticModuleResolver::new();
    static_modules.insert("test_module", module);
    engine.set_module_resolver(Some(static_modules));

    engine.consume(SCRIPT).unwrap();
}

In 0.19.0 with true it works, in 0.19.1 without the argument it errors with function not found: foo ()

---

By the way, once I cargo builded with rhai="0.19.1", changing it to rhai = "0.19.0" didn't even pull the new depdendency since cargo assumes them to be compatible, I had to do rhai = "= 0.19.0", yikes

[schungx]

Hear you. I'll be more careful in the future about breaking changes. Probably bundle them up for the next minor version bump.

[schungx]

By the way, once I cargo builded with rhai="0.19.1", changing it to rhai = "0.19.0" didn't even pull the new depdendency since cargo assumes them to be compatible, I had to do rhai = "= 0.19.0", yikes

Yes, you'll have to remove Cargo.lock for this.

I'll check the eval_ast_as_new...

[schungx]

You're using StaticModuleResolver which only serves static modules... it should work with FileModuleResolver. Can you check?

I'll make StaticModuleResolver work as well...

[SuperCuber]

Originally I got that error in my own Resolver, which is strange since it's basically a copy of the latest FileModuleResolver...

To be fair, the only reason I have my own is because I explicitly want it to not cache so a flag to FileModuleResolver to disable that would solve my issue as well (or maybe a feature flag? that's probably a bad idea because you might want both)

Here's my implementation for reference:

struct FilesystemModuleResolver;
impl rhai::ModuleResolver for FilesystemModuleResolver {
    fn resolve(
        &self,
        engine: &rhai::Engine,
        path: &str,
        pos: Position,
    ) -> Result<Module, Box<EvalAltResult>> {
        let mut path = PathBuf::from(CONFIGURED).join(path);
        path.set_extension("rhai");
        if !path.exists() {
            return Err(Box::new(EvalAltResult::ErrorModuleNotFound(
                path.to_string_lossy().into(),
                pos,
            )));
        }

        let ast = engine.compile_file(path.into())?;

        Ok(Module::eval_ast_as_new(Scope::new(), &ast, true, engine)?)
    }
}

I'll check FileModuleResolver in a second.

[schungx]

I think there is a bug... tracking it down...

[SuperCuber]

confirming that FileModuleResolver fails on this as well

[schungx]

Yes, it is a bug! Exactly what you mentioned... poetic...

I had an !is_empty() which should be is_empty()...

Bummer

[SuperCuber]

Heh, time to open an issue with rust asking for an unless statement

[schungx]

I'll yank 0.19.1 and push 0.19.2...

[schungx]

@SuperCuber just in case I screw up again, can you pull from this repo to test, just in case? I've already landed the PR.

Thanks!

[SuperCuber]

Seems good, tested FileModuleResolver and my custom one (not static)

[schungx]

Thanks!