Bump-Scoped rendering and SVG class attribute fix

[awestlake87]

Changed the Render trait to be generic across a lifetime instead of containing a generic render function. This allows components that implement Render to contain bump-allocated fields as well, which can make it easier to separate the rendering logic without resorting to raw functions or duplicate allocations. More details in #46.

This also includes a fix for #125 that was already in my fork. I can revert it if we want to separate these two issues.

[awestlake87]

Hmm, looks like the checks failed, but I think it failed before it started testing code. Looks like it's failing to download some tools.

[awestlake87]

Here's where the SVG change begins and ends. I believe it's just this one if statement.

[awestlake87]

In #46 @fitzgen mentioned this is the same as 'static, but I'm not sure it is in this case. It would be simpler without higher-order lifetimes, so I was trying to change it to Render<'static> or Render + 'static. The compiler didn't like either of those, which kinda makes sense to me.

The way I understand it, this trait bound says RootRender requires its types to also implement Any and Render for all lifetimes 'a. 'static is just the longest possible lifetime, so it wouldn't fulfill that requirement.

Am I missing something?

[fitzgen]

If something is valid for all lifetimes then it must be 'static which is the lifetime that is always valid. If it were not 'static then it would not be valid for all lifetimes, leading to a contradiction.

The only time that for<'a> is useful is when you are taking some generic function and calling it with parameters that contain "unknown" lifetimes or when you wish to call it multiple times with arguments of different lifetimes.

For this case, I think Render<'static> + 'static should do the trick here.

[awestlake87]

So I think we both might be right.

Even though Render is a generic trait, it is object safe when used with higher-order lifetimes. That indicates to me that you are correct about for<'a> Render<'a> essentially being 'static from the perspective of the implementation of this trait. The compiler is effectively able to assume that the lifetime of the bump arena in RenderContext is static since it will always be inconsequential to the functionality inside of render.

That being said, the bound for<'a> Render<'a> does seem to be required, maybe not for the callee, but the caller. Here's an example that I've boiled down:

use dodrio::{bumpalo::{self, Bump}};

struct Context<'a> {
    bump: &'a Bump
}

#[derive(Debug, PartialEq, Eq)]
struct Node<'a> {
    name: &'a str
}

trait Render<'a> {
    fn render(&self, cx: &mut Context<'a>) -> Node<'a>;
}

struct Test;

impl<'a> Render<'a> for Test {
    fn render(&self, cx: &mut Context<'a>) -> Node<'a> {
        Node { name: bumpalo::format!(in cx.bump, "Test{}", 1).into_bump_str() }
    }
}

#[cfg(test)]
mod tests {
    use super::*;


    #[test]
    fn it_works() {
        let bump = Bump::default();
        let mut cx = Context { bump: &bump  };
        let comp: Box<dyn for<'a> Render<'a>> = Box::new(Test);

        assert_eq!(Node { name: "Test1" }, comp.render(&mut cx));
    }
}

This test compiles and runs as expected. It's emulating the situation I currently have in my fork of dodrio.

Now, when I switch the type of comp to Box<dyn Render<'static> + 'static>, the compiler doesn't like this example anymore.

use dodrio::{bumpalo::{self, Bump}};

struct Context<'a> {
    bump: &'a Bump
}

#[derive(Debug, PartialEq, Eq)]
struct Node<'a> {
    name: &'a str
}

trait Render<'a> {
    fn render(&self, cx: &mut Context<'a>) -> Node<'a>;
}

struct Test;

impl<'a> Render<'a> for Test {
    fn render(&self, cx: &mut Context<'a>) -> Node<'a> {
        Node { name: bumpalo::format!(in cx.bump, "Test{}", 1).into_bump_str() }
    }
}

#[cfg(test)]
mod tests {
    use super::*;


    #[test]
    fn it_works() {
        let bump = Bump::default();
        let mut cx = Context { bump: &bump  };
        let comp: Box<dyn Render<'static> + 'static> = Box::new(Test);

        assert_eq!(Node { name: "Test1" }, comp.render(&mut cx));
    }
}

Compiler output:

   Compiling dodrio-test v0.1.0 (dodrio-test)
error[E0597]: `bump` does not live long enough
  --> src/lib.rs:32:38
   |
32 |         let mut cx = Context { bump: &bump  };
   |                                      ^^^^^ borrowed value does not live long enough
33 |         let comp: Box<dyn Render<'static> + 'static> = Box::new(Test);
   |                   ---------------------------------- type annotation requires that `bump` is borrowed for `'static`
...
36 |     }
   |     - `bump` dropped here while still borrowed

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
error: could not compile `dodrio-test`.
warning: build failed, waiting for other jobs to finish...
error: build failed

This is because while the Test component may treat its Context as 'static, the it_works function knows that cx has a lifetime that is shorter than 'static. Therefore, the trait bound of Render<'static> cannot be satisfied.

If I'm missing something, could you show me what you mean with this example? Maybe we're talking about different things.

[fitzgen]

Ah ok, this makes sense. Thanks for breaking it down!

[fitzgen]

Looks like not everything from the tests/ subdirectory was adapted for the changes in this PR:

error[E0726]: implicit elided lifetime not allowed here
  --> tests/web/cached.rs:21:6
   |
21 | impl Render for CountRenders {
   |      ^^^^^^- help: indicate the anonymous lifetime: `<'_>`

https://travis-ci.com/fitzgen/dodrio/jobs/287099243#L321

[fitzgen]

Using className when it is valid to do so is a significant speedup.

I haven't read closely enough to the original issue yet to have a strong grasp on what the ways that SVG and class name setting were interacting poorly are, but I think the best fix is probably to only use the fast path when name == "class" && namespace.is_none(). This will require a little more plumbing to pass the namespace in, if any, but shouldn't be too difficult.

[awestlake87]

Ah, yep. I wasn't using the build.sh script. I'll get that sorted out soon.

[awestlake87]

Ok, I think most everything is fixed now. js-sys is having trouble compiling on nightly, though. Not sure why...

[fitzgen]

The nightly error is a rustc regression: rust-lang/rust#69315

[fitzgen]

Can you add a regression test for the svg namespaces and setAttribute bug and also a smoke test for actually rendering a child component that borrows from the bump? With that, then we can merge this.

Thanks!

[awestlake87]

This should act as a smoke test for bump-scoped lifetimes in child components

[fitzgen]

Sorry I should have been more clear. This does ensure that code with bump-scoped child components compiles, but I was hoping for a rendering test that ensures that we also get the expected physical dom. Should be fine to put it in the render.rs test file.

[awestlake87]

Ah, ok. I'll get that taken care of sometime tonight.

[awestlake87]

Here's the regression test. I wanted to test the failure case too, but web_sys doesn't mark set_class_name as catch, so I wasn't able to figure out how to do it.

You can verify that this works by running the following:

$ wasm-pack test --firefox --headless -- --features xxx-unstable-internal-use-only test_svg_set_class

Then, verify that the test fails when the new change is disabled:

// src/change_list/mod.rs - comment out !is_namespaced
    pub fn set_attribute(&mut self, name: &str, value: &str, is_namespaced: bool) {
        debug_assert!(self.traversal_is_committed());
        if name == "class" /* && !is_namespaced*/ {
            let class_id = self.ensure_string(value);
            debug!("emit: set_class({:?})", value);
            self.state.emitter.set_class(class_id.into());
        } else {
            let name_id = self.ensure_string(name);
            let value_id = self.ensure_string(value);
            debug!("emit: set_attribute({:?}, {:?})", name, value);
            self.state
                .emitter
                .set_attribute(name_id.into(), value_id.into());
        }
    }

[awestlake87]

BTW, I wasn't sure where to put the test, so let me know if you want to move it somewhere else.

[fitzgen]

this is a fine location -- thanks!

[awestlake87]

Here's the smoke test. It's pretty similar to the other one, but I used the assert_rendered afterward

[fitzgen]

Thanks for sticking through with this PR! Looks great :)