Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cargo features additively. #182

Closed
vi opened this issue Jul 10, 2020 · 32 comments
Closed

Use cargo features additively. #182

vi opened this issue Jul 10, 2020 · 32 comments
Labels

Comments

@vi
Copy link

vi commented Jul 10, 2020

Adding more Cargo features should bring in more code and features, not less. Also adding a Cargo feature should almost never break existing scripts.

With a major semver bump, I suggest to make the following changes:

no_std

Reverse it to std.

plugins

I don't know what it is, probably leave it as is.

unchecked

Reverse to checked.

sync

Leave as is.

no_optimize

Reverse to optimize. Maybe rhai::OptimizationLevel::None should be available even without it.

no_float

Reverse to float

only_i32
only_i64

This one is tricky.
There should probably be integer feature that enables all integer types and also i32 and i64 features that selectively enable those types.

integer should imply (but not be limited to) i32 + i64.

A compile error should be emitted if none of three features (i32, i64, integers) are activated, unless Rhai can work without integers at all.

It should be OK to activate both i32 and i64 features without integer feature.

no_index

Reverse to index.

no_object

Reverse to object

no_function

Reverse to function

no_module

Reverse to module.

internals

Leave as is.

default = []

default=["std","module","function","object","index","integer","float","optimize","checked","plugins?"]

Additionally, I think keywords from missing features should still be reserved in scripts (unless explicitly configured otherwise from the Rust side), to avoid typical Rhai scripts breaking when a feature suddenly gets turned on from afar.

@vi
Copy link
Author

vi commented Jul 10, 2020

(answering to #100 (comment))

On the other hand, it conforms to "standard".

Not only it conforms to a standard, it also allows using Rhai multiple times in dependecy tree without cooperation between those dependencies.

I think I'll let the community ponder it more before deciding to change it for 1.0...

Community involvement is encouraged, even if just to inform about pending API breakage long before it actually happens.

You can announce that, for example, rhai v0.22.0 would have the new scheme (and write a warning in the book and crate doccomment near that features table), so users can upgrade gradually.


I think Cargo feature revamp should definitely happen before the "v1.0" release, but there is no reason to rush it.

In current form it looks like Rhai is indended to be used only by apps, not as a transitive, private dependency inside libraries.

@alvinhochun
Copy link
Contributor

It's something that I agree should be fixed. Since it will be a hard breaking change, users will have to fix their end when they upgrade. It doesn't matter whether you inform of the change early in advance, the change can still only be made the moment a new version with the change is available. It's not something that users can slowly migrate to like a deprecated function. Therefore, I'd rather see it fixed as early as possible, so that new users won't have to make the changes down the road. Just make it clear in the release notes and changelog.

@schungx schungx added the bug label Jul 11, 2020
@schungx
Copy link
Collaborator

schungx commented Jul 11, 2020

Additionally, I think keywords from missing features should still be reserved in scripts (unless explicitly configured otherwise from the Rust side), to avoid typical Rhai scripts breaking when a feature suddenly gets turned on from afar.

There is a problem with this.

When a feature is turned off, the current design is that it is as if the feature doesn't even exist.

Therefore, if no_function is on, then fn is a valid variable name.

You can further disable keywords., If you disable if, then if is a valid variable name.

The lack of a feature maybe dependent by script writers. Therefore, features are actually not additive. That is, a script that runs fine on a system without some language feature may not run in a system with that language feature, simply because the script writer assumes the lack of such language feature.

The idea is that there should only be one scripting language in an application. Therefore, the feature set of the Rhai language should be fixed.

However, you bring up a good scenario in which, for example, the application has its own embedded scripting language, but then also pull in crates such as casbin-rs which also has its own scripts.

In such a scenario, even if features are additive, you'll have trouble. That's because casbin assumes a restricted language; if you turn those features on, then casbin may not be able to handle it.

In other words, it might actually be better to turn off language features using configuration instead of features... But then, of course, we can never make a "minimal" build because you end up pulling everything in, even though it is not needed.

Another alternative is to keep the features set, but then also add a configuration API to turn off certain elements even when the features itself are there.

@schungx
Copy link
Collaborator

schungx commented Jul 11, 2020

Proposal for pre-1.0:

  1. Make features additive. The default should have almost everything. For example, baseline will have no modules, and "modules" feature adds it. Default will have "modules".

  2. Make API: Engine::disable_feature that selectively disables a feature even if it is there. If it isn't there, it does nothing.

  3. Any app who wants to restrict the language needs to omit the feature from the crate, and also call Engine::disable_feature on each Engine just in case something upstream includes that feature.

Does this make sense?

@alvinhochun
Copy link
Contributor

However, you bring up a good scenario in which, for example, the application has its own embedded scripting language, but then also pull in crates such as casbin-rs which also has its own scripts.

In such a scenario, even if features are additive, you'll have trouble. That's because casbin assumes a restricted language; if you turn those features on, then casbin may not be able to handle it.

I believe the best solution to this would be for Cargo to allow private instances of dependencies that are not shared between crates (there is an RFC for this), but we probably don't want to wait until it is implemented.

  • Make API: Engine::disable_feature that selectively disables a feature even if it is there. If it isn't there, it does nothing.

  • Any app who wants to restrict the language needs to omit the feature from the crate, and also call Engine::disable_feature on each Engine just in case something upstream includes that feature.

How about using marker type parameters so that language features can be disabled during compilation? But obviously this will take more effort to implement, will increase compile times, and users will probably have to make a type alias to refer to the Engine.

@vi
Copy link
Author

vi commented Jul 11, 2020

Engine::disable_feature is negative thinking again, not protected against future new features. Primary and recommended way should be enable_feature instead.

I.e. Engine::new() automatically enables all features present in Cargo (i.e. all features by default) and is recommended for applications. And EngineBuilder::no_features().add_feature(Feature::Functions).add_feature(Feature::Modules).....build() enables only selected features and is recommended for libraries.

@vi
Copy link
Author

vi commented Jul 11, 2020

Therefore, if no_function is on, then fn is a valid variable name.

fn being valid variable name splits Rhai scripts ecosystem into incompatible parts, also interferes with naturally upgrading Rhai environment from initially simple expression evaluator to more involved scripting environment as time passes and project scope gets bigger. It is like using class in C code, thinking "We'll never need to interface C++, so why not?".

So a function like EngineBuilder::reserve_all_keywords function can be nice to make scripts more restricted and prevent future upgrade problems. It can just Engine::disable_symbol all the unused keywords. It should be recommended in most environments, except of maybe if memory is so constrained that even holding a list of banned names is wasteful, or when Rhai is used just as an expression evaluator.

If Rhai has some pending, not-yet-developed features that also require keywords, they should also be disabled ahead of time.

@schungx
Copy link
Collaborator

schungx commented Jul 11, 2020

EngineBuilder::no_features().add_feature(Feature::Functions).add_feature(Feature::Modules).....build()

This looks feasible...

@schungx
Copy link
Collaborator

schungx commented Jul 12, 2020

I ended up adding a -> &mut Self to most API methods.

A builder style does not allow subsequent modification of the Engine. It is actually more convenient to just be able to chain API calls.

@schungx
Copy link
Collaborator

schungx commented Jul 23, 2020

I have been giving this some more thought and done a bunch of reading.

Such as this: https://stephencoakley.com/2019/04/24/how-rust-solved-dependency-hell

Seems like Rust is quite smart. For crate invocations that are clearly different, it pulls in both versions under different mangled names.

Now, the question is what it'll do when confronted with the same version of the crate, but with different features enabled.

Does it:

  1. Keep only one version and have all the features?
  2. Keep two versions each having their own features set?

From https://stackoverflow.com/questions/56921098/cargo-build-package-with-conflicting-features-from-the-same-git-repository it looks like (1).

Seems that to get (2) you need to clone the repo into another location. Then Cargo will treat it as a separate dependency.

In Rhai, features are really not very composable. They really turn on/off certain language features. Therefore, having a feature turned on when an app expects it to be off is not a mere wastage of more code. It may be incorrect - the app expects the lack of certain language features. The non-existence of a feature is as much as differentiator as a feature.

So it is really not as simple as making all feature additive, because the lack of a feature is a feature by itself.

@vi
Copy link
Author

vi commented Jul 23, 2020

the app expects the lack of certain language features
lack of a feature is a feature by itself.

Isn't it a design flaw? Imagine an ecosystem of user-contributed Rhai scripts that can be downloaded and executed. Without additional restrictions (such as that Rhai scripts should not break if extra features get enabled) it would be tricky to enhance script features (or even support both old-style and new-style scripts).

This is why I also propose to make it easy to reserve keywords for all possible features (maybe even some future unimplemented ones)

Additionally, I think keywords from missing features should still be reserved in scripts ...


What things in Rhai, apart from keywods being available or not available for use as identifiers, do require absense of particular features?

@schungx
Copy link
Collaborator

schungx commented Jul 24, 2020

This is why I also propose to make it easy to reserve keywords for all possible features (maybe even some future unimplemented ones)

Yes, reserved keywords are already merged, thx to your idea. The keywords are already reserved.

What things in Rhai, apart from keywods being available or not available for use as identifiers, do require absense of particular features?

Probably not. I guess there is nothing that cannot be done by simply disabling certain keywords and be done with it. There is even disable_symbol to do exactly that.

However, that also means that somebody writing a library that uses Rhai in a restricted environment must go thru the hassle of disabling all the features that he/she doesn't need, just in case the upstream app also uses Rhai and starts enabling some things.

And also there are features that are mutually exclusive and not additional. For example: sync. This is a feature that is additive, however, its behavior is not additive.

With sync: Engine must be Send + Sync and supports multi-threading. Without sync, Engine can handle any data type but not multi-threaded.

Say a library writer uses Rhai. He/she obviously doesn't want to restrict the library to sync only, because apps using the lib may not be multi-threaded.

However, if an app using the lib needs a scripting feature which needs to be thread-safe, if it pulls in sync, it affects the version used by the lib itself.

I believe right now the only solution is to have marker traits and then have type-def's that refer to common Engine invocations...

@schungx schungx removed the bug label Aug 20, 2020
@schungx
Copy link
Collaborator

schungx commented Sep 6, 2020

Closing this for the time being. It is not a simple matter to make Rhai features additive.

See: https://schungx.github.io/rhai/patterns/multiple.html

@vi
Copy link
Author

vi commented Sep 6, 2020

Should Rhai be explicitly documented against usage as a private (internal) dependency in libraries then?

Something like "Don't use Rhai in libraries. Rhai as a language is sensitive to the chosen Cargo feature set and is expected to appear only once in a project's dependency tree. Internal Rhai dependency may introduce unwanted Rhai feature switches or be confused by activating unwanted switches from a neighbouring Rhai dependency inclusion".

@schungx
Copy link
Collaborator

schungx commented Sep 6, 2020

Unfortunately many libraries use Rhai especially as an expression evaluator... so it is probably not easy to completely ban it.

@vi
Copy link
Author

vi commented Sep 9, 2020

Do you plan leaving the "Rhai v1.0" with cargo features usage still unfixed?

What does mean "for the time being" and how does it interact with the "wontfix" label?

Or maybe adjusting feature completeness vs compactness will be moved away from Cargo features to e.g. plugins? (can it handle sync and !sync engines within the same project?)

@schungx
Copy link
Collaborator

schungx commented Sep 10, 2020

can it handle sync and !sync engines within the same project

Unfortunately no. Not without finding a way to keep two separate copies. That's why Rhai features are not additive. They are more like toggle switches.

Another example will be no_float. It is simple: If the target does not have floating point support, it should exclude floating-point math types in its code. However, say a library does not specify no_float and may already register floating-point functions. In other words, the library actually requires floating-point support; therefore technically speaking it should not be used in any project that requires no_float. In other words, the feature, similar to no-std, is not additive; adding or removing features break compatibility altogether instead of adjusting behavior.

The current "features" feature in Cargo attempts to model additive behavior. It is technically not intended to be used to model pre-requisits. But the only way to model pre-requisits right now is to break up the project into multiple foundation crates, one implementing a single feature, then use runtime code to pull them all in. That's why you get the wide range of sub-crates in many Rust projects.

TL;DR So in conclusion, it is not a simple solution to make features additive. The "right" solution is to break up Rhai into multiple sub-crates.

@vi
Copy link
Author

vi commented Sep 10, 2020

Libraries may want to exclude feature to minimize code bloat even if otherwise it is no problem to leave feature enabled.

I expect sync and no-sync to be the biggest problem. Some libraries publish even two crates because of that. Example: im im-rc.

@vi
Copy link
Author

vi commented Sep 10, 2020

It is technically not intended to be used to model pre-requisits.

I don't understand this phrase.

Do you consider Tokio's usage of feature flags an intended use case of Cargo features?

@schungx
Copy link
Collaborator

schungx commented Sep 10, 2020

Libraries may want to exclude feature to minimize code bloat even if otherwise it is no problem to leave feature enabled.

Yes, that's the main problem and why features are this way. Embedded targets (even WASM) will want minimal builds to exclude unnecessary code. If hidden behind API switches, all those code will remain. The only way is to exclude those code with compile-time config switches.

That's why many of Rhai's language features are tagged under feature flags instead of a "configuration" API.

@vi
Copy link
Author

vi commented Sep 10, 2020

The only way is to exclude those code with compile-time config switches.

Or usage of compile-time evaluation (const functions) and optimisation (including whole-program fat LTO). String formatting and panicking code is often excluded this way when doing embedded.

@schungx
Copy link
Collaborator

schungx commented Sep 10, 2020

I don't understand this phrase.

Probably not very scientific, but I'm at a loss to find the right phrase.

Basically, unlike tokio as you pointed out, Rhai features cannot be used this way.

In tokio, for example, if you turn on tcp, then it pulls in all the tcp types. Fine. If another library doesn't want it, it doesn't harm. It'll just sit there occupying code space and nothing else.

The problem with "it doesn't harm" doesn't work with Rhai. Image that, for some wierd reason, we add a "while-loop" feature to Rhai, enabling/disabling the while statement.

A library pulling in Rhai with while-loop set will conflict with other code that doesn't require it. In other words, it does harm. The other code that doesn't set while-loop will expect the while statement to throw a syntax error; it will not expect a while statement to magically work.

@schungx
Copy link
Collaborator

schungx commented Sep 10, 2020

Or usage of compile-time evaluation (const functions) and optimisation (including whole-program fat LTO). String formatting and panicking code is often excluded this way when doing embedded.

Not easily. String formatting and panicking code is excluded via feature gates.

@vi
Copy link
Author

vi commented Sep 10, 2020

A library pulling in Rhai with while-loop set will conflict with other code that doesn't require it. In other words, it does harm. The other code that doesn't set while-loop will expect the while statement to throw a syntax error.

It may be solved by double check: a library which wants to fine tune which parts of Rhai it wants does two things:

  1. Includes appropriate Cargo feature
  2. Enables the keywords using in-code configuration (runtime or compiletime)

This way two Rhai engines: the one with while identifier and the other one with while keyword can coexist.


will expect the while statement to throw a syntax error

I think relying on things being syntax errors is not future-proof anyway. It is like relying on optimisation removing bad code.

@schungx
Copy link
Collaborator

schungx commented Sep 10, 2020

I think relying on things being syntax errors is not future-proof anyway.

That was just an example. There are many language features that fall into such category. Having it and not having it is not a simple matter of: If I have it but don't use it, it's fine. Many such cases are more like: Oh heck, I don't need it and suddenly it shows up and it is screwing me up.

@schungx
Copy link
Collaborator

schungx commented Sep 10, 2020

It may be solved by double check: a library which wants to fine tune which parts of Rhai it wants does two things:

1. Includes appropriate Cargo feature

2. Enables the keywords using in-code configuration (runtime or compiletime)

This way two Rhai engines: the one with while identifier and the other one with while keyword can coexist

You are absolutely right. This is the correct way to add/remove features, which is in-code configuration.

But this way it is very hard to remove the code if you don't need it.

There is one way that works, which is to have both additive features plus runtime configuration switches. But then the user must remember to do both.

@vi
Copy link
Author

vi commented Sep 11, 2020

But this way it is very hard to remove the code if you don't need it.

What about things like

pub trait RhaiFeatures {
    const ENABLE_FLOATS : bool = true;
    const ENABLE_OPTIMIZE: bool = true;
    ...
}

impl<F:RhaiFeatures> Something<F> {
    pub fn do_something() {
         if F::ENABLE_FLOATS {
             heavyweight_function();
         }
    }
}

Does it provide enough assurance that heavyweight_function is really not inclued?


Maybe a fork/branch of Rhai with experimental feature selection revamp should be created?

@schungx
Copy link
Collaborator

schungx commented Sep 12, 2020

Maybe a fork/branch of Rhai with experimental feature selection revamp should be created?

I wouldn't mind this at all! I tried but it wasn't easy. If somebody would like to take a shot at it, it'll be good!

@schungx
Copy link
Collaborator

schungx commented Sep 12, 2020

Does it provide enough assurance that heavyweight_function is really not inclued?

It does, but then the Engine is going to have a generic parameter tagged on. And there will need to be different traits with different combinations of true and false based on the features set. Essentially this is equivalent to in-code configuration.

@vi
Copy link
Author

vi commented Sep 12, 2020

I tried but it wasn't easy.

Maybe it can be done gradually, starting from some one feature?

Is there some statistics of how Rhai is used (especially in libraries), which features are typically enabled and where absense of features are really required?

You told that Rhai is sometimes used as math expression evaulator in some libraries (as far as I understood). Which of reverse dependencies are worth revieing? Are there known projects that are not on crates.io?

@schungx
Copy link
Collaborator

schungx commented Sep 12, 2020

Is there some statistics of how Rhai is used (especially in libraries), which features are typically enabled and where absense of features are really required?

I'm not sure if there is somewhere where these telementaries are kept... crates.io doesn't seem to capture it...

@schungx
Copy link
Collaborator

schungx commented Sep 12, 2020

You told that Rhai is sometimes used as math expression evaulator in some libraries (as far as I understood). Which of reverse dependencies are worth revieing? Are there known projects that are not on crates.io?

Not sure about non-crates.io projects. There is an outstanding issue about projects using Rhai but it is not updated.

If you scan thru the issues list, you'll find a lot of questions about using Rhai, probably in projects no on crates.io.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants