Extension Initialization

[colin-kiegel]

Ok, here is some complicated issue and the reason, why I think we need to have a "ExtensionsAreNotRegistered" error somewhere in our codebase.

I moved ExtensionRegistry to engine::Options.

pub struct Options {
    pub debug: bool,
   `// ...
    pub extensions: ExtensionRegistry,
}

Now Setup does this

        // init extensions
        try_chain!(self.options.extensions.init( /* foo */ );

The problem is with foo.

Lets look at ExtensionRegistry::init()

    pub fn init(&mut self, options: &/*mut?*/ Options) -> Result<(), ExtensionRegistryError> {
        if self.initialized {
            return err!(ExtensionRegistryErrorCode::AlreadyInitialized)
        }

        for (_, ext) in self.ext.iter() {
            ext.init(options);
   // ...

Notice the &mut self in init()

This loops over extensions implementing something like this

/// Extends the Twig Engine with new behaviour.
pub trait Extension : fmt::Debug {
    /// Initialize the engine.
    /// This is where you can load some file that contains filter functions for instance.
    fn init(&mut self, _options: &engine::Options) {} // TODO: add error handling ???
    // ...

>>> Uh oh. Now we have to fight the borrow checker. Why? (a) extensions are part of engine::Options (b) each extensions might want to initialize itself (via &mut self borrow). Due to (a) this implies a mutable borrow to engine::Options (c) It would be reasonable for extensions to read the options, but we can't have another borrow due to the mutable borrow of one extension.

How can we solve this? Safe or Unsafe?

• extensions are part of engine::Options - but nullable, i.e. wrapped inside Option<ExtensionRegistry>. During initializiation, we set this to None, so we have separate borrows. After initialization we move extensions inside options. The catch: All following code must deal with nullable Extensions inside Options... This is the "ExtensionsAreNotRegistered" error.

Are you ok with this approach, or do you have a different suggestion?

[Nercury]

Separate Options and Extensions. When passing Options to extensions, maybe pass only the objects containing data that is needed. However, when extensions modify options, those are no longer the same Options. Therefore immutable reference to original options would suffice, provided that extensions create next incarnation of Options after initialization.

[Nercury]

OK, I am thinking of this "pipeline":

• &TwigOptions -> consumed by registry and passed to extensions.
• Each extension added into registry is free to have its own options passed over its constructor new.
• Transform &Registry into UnicornOptions with { LexerOptions and ParserOptions } inside. Don't allow extensions to modify UnicornOptions directly, instead provide interface that requires them to pass ownership of all defined functions/filters/operators etc. At this transformation is known which bits of data are required for lexer and parser, pass only the things that are needed. For example, all that Lexer needs from extensions is a vector of operator names.
• Pass &LexerOptions to lexer when running it.
• Pass &ParserOptions to parser the same way.

Worked for me! :)

P.S. Using unicorn from now when I don't care about name.

[colin-kiegel]

Ok, cool - thx - I will take some inspiration from codebase, too. E.g. I like the Config::from_hashmap() :-)

[colin-kiegel]

PS: I noticed a different approach in your codebase.

impl Extension for CoreExtension {
    fn apply(env: &mut Environment) {
        env.push_operators(vec![
            Operator::new_unary("not", 50, |_| unimplemented!()),
            Operator::new_unary("-", 500, |_| unimplemented!()),
            Operator::new_unary("+", 500, |_| unimplemented!()),

vs. my twigphp-oriented approach

/// Extends the Twig Engine with new behaviour.
pub trait Extension : fmt::Debug {
    /// Get the token parser instances to register with the engine.
    fn token_parsers(&self) -> HashMap<String, Box<TokenParser>> {
        HashMap::new()
    }

    /// Get the node visitor instances to register with the engine.
    fn node_visitors(&self) -> Vec<Box<NodeVisitor>> {
        Vec::new()
    }

I like the explicit style (alias "tell, don't ask") - I will change my pull request accordingly. :-)

[Nercury]

Allright, but have in mind that fn apply and Extension were just most minimal things that worked (haven't spend much time thinking about names).

The more interesting part I think is the Operator implementation. If you stumble upon function/Callable, don't pay much attention to Callable::Static, it's probably a bad way to do it.

But one idea might be useful: I am transforming all operator closures into functions that take arbitrary number of arguments (this one: Box<for<'e> Fn(&'e [Value]) -> RuntimeResult<Value>>), but keep argument count check hidden from extension developers here.

In the end I wished to leave only one kind of callable for any filter, operator or closure, which would simplify the execution, and allow to parse callables from other kinds of tokens.