TaskLog: IntoOwned for lightningcss

Oct 6, 2023

Recently, I created a clever trait that can be beneficial to all users of lightningcss. I don't want to brag about that fact, but I'm writing a post because I think it'll help many people who want to be good at programming. The key to programming is the way you think.

Problem

I want to use lightningcss for one of my projects. But AST nodes are not 'static, so I can't use them when I need 'static lifetime. So... I tried implementing them by recursively copying items.

But I don't want to do this. And it seems like some types have a method like .into_owned() , which can be used to get the lifetime I want.

So, my question was

Why don't all types have such a method?

and it seems like it's because CssRule is defined as

pub enum CssRule<'i, R = DefaultAtRule> {
  /// A `@media` rule.
  #[cfg_attr(feature = "serde", serde(borrow))]
  Media(MediaRule<'i, R>),
  /// An `@import` rule.
  Import(ImportRule<'i>),
  /// A style rule.
  Style(StyleRule<'i, R>),
  /// A `@keyframes` rule.
  Keyframes(KeyframesRule<'i>),
  /// A `@font-face` rule.
  FontFace(FontFaceRule<'i>),
  /// A `@font-palette-values` rule.
  FontPaletteValues(FontPaletteValuesRule<'i>),
  /// A `@page` rule.
  Page(PageRule<'i>),
  /// A `@supports` rule.
  Supports(SupportsRule<'i, R>),
  /// A `@counter-style` rule.
  CounterStyle(CounterStyleRule<'i>),
  /// A `@namespace` rule.
  Namespace(NamespaceRule<'i>),
  /// A `@-moz-document` rule.
  MozDocument(MozDocumentRule<'i, R>),
  /// A `@nest` rule.
  Nesting(NestingRule<'i, R>),
  /// A `@viewport` rule.
  Viewport(ViewportRule<'i>),
  /// A `@custom-media` rule.
  CustomMedia(CustomMediaRule<'i>),
  /// A `@layer` statement rule.
  LayerStatement(LayerStatementRule<'i>),
  /// A `@layer` block rule.
  LayerBlock(LayerBlockRule<'i, R>),
  /// A `@property` rule.
  Property(PropertyRule<'i>),
  /// A `@container` rule.
  Container(ContainerRule<'i, R>),
  /// A `@starting-style` rule.
  StartingStyle(StartingStyleRule<'i, R>),
  /// A placeholder for a rule that was removed.
  Ignored,
  /// An unknown at-rule.
  Unknown(UnknownAtRule<'i>),
  /// A custom at-rule.
  Custom(R),
}

I think R would be the problem. This is a guess, so I need to verify.

MediaRule contains R as a generic parameter, and does not have .into_owned() .
ImportRule does not have a generic parameter, and have .into_owned().
.into_owned() of ImportRule (see CssRule::Import(r.clone().into_owned()) above) is implemented as direct implementation instead of using a trait.

So, I can avoid writing code for a recursive copy if I can make a trait that is implemented for types regardless of generic parameters.

But lightingcss is not my project. I don't have permission to add such a trait. But this trait will benefit everyone! Let's ask the maintainer if they are open to a PR.

https://github.com/parcel-bundler/lightningcss/issues/596

IntoOwned as a trait, and implement it for nodes when R implements IntoOwned

Will you accept a PR for this?
If so, which crate should have IntoOwned?

And the response was

I think I originally tried to implement it as a trait and couldn't get it to work but I can't remember why. But if you get it working that sounds good to me. We can put the trait in here https://github.com/parcel-bundler/lightningcss/blob/master/src/traits.rs

Yeah, it makes sense. Lifetime is very tricky if it's mixed with a proc-macro and a trait. But I don't want to write recursive copy code. That's more important, at least to me.

Solutions

Of course, I didn't get to the answer from the start. Instead, I started with something super dumb. But who cares? I did it. And that's what matters. I'll describe my approaches in order. Don't swear at me even if I look too stupid.

#1: `Self` -> `Self`

I declared the trait like

/// A trait for things that can be cloned with a new lifetime.
pub trait IntoOwned {
  /// Make lifetime of `self` `'static`.
  fn into_owned(self) -> Self;
}

Umm... This is super stupid, but it's okay if you don't see the problem with this definition. I was the same.

I happily wrote a simple macro to implement this trait for already-owned types.


macro_rules! impl_into_owned {
  ($t: ty) => {
    impl<'i> IntoOwned for $t {
      #[inline]
      fn into_owned(self) -> Self {
        self
      }
    }
  };
  ($($t:ty),*) => {
    $(impl_into_owned!($t);)*
  };
}

impl_into_owned!(bool, f32, f64, u8, u16, u32, u64, u128, i8, i16, i32, i64, i128, usize, isize);

Thanks to GitHub Copilot, this was dead simple. I only typed macro_rul , ($($t and impl_into_owned!(bool, f32 . Actually, this kind of code is extremely common in the Rust world.

Time to patch derive macro to generate impl IntoOwned for Type {} instead of impl Type { } . The original definition was

    quote! {
      impl #impl_generics #self_name #ty_generics #where_clause {
        /// Consumes the value and returns an owned clone.
        pub fn into_owned<'x>(self) -> #self_name<'x, #(#params),*> {
          #res
        }
      }
    }

and umm... My trait is so dumb that it cannot return anything other than Self, which has exactly the same lifetime and generic parameters as self.

#2: `Self` -> `Self<'static>`

Okay. I agree that we need to return something other than Self. However, the return type is determined by the shape of the type. So the output should be an associated type of the trait.

So, I declared the trait like this.

pub trait IntoOwned {
  type Owned: 'static;

  /// Make lifetime of `self` `'static`.
  fn into_owned(self) -> Self::Owned;
}

This does not look stupid, at least. So I was satisfied. I patched the derive macro and existing implementations while thinking

This is too easy, considering the comment by the maintainer.

It seems to work, except for exactly one item.

https://github.com/kdy1/lightningcss/tree/abaddb600775364b0ecbea279b36fa2e5e23b19c

lifetime may not live long enough requirement occurs because of the type properties::Property<'_>, which makes the generic argument '_ invariant the enum properties::Property<'i> is invariant over the parameter 'i see https://doc.rust-lang.org/nomicon/subtyping.html for more information about variance

The lifetime was invariant, so a 'static lifetime was not enough, and rustc wants an exact match. But well... this error came from a code that worked previously.

The previous definition of Property::into_owned(self) should be generated from

pub fn into_owned<'x>(self) -> #self_name<'x, #(#params),*>

so it should be something like into_owned<'x>(self) -> Property<'x, ...> . It worked? Okay... I don't like the decision of rustc, but I decided to mimic the previous code anyway.

#3: `Self` -> `Self<'any>`

pub trait IntoOwned<'any> {
  type Owned: 'any;

  fn into_owned(self) -> Self::Owned;
}

Previously, 'x in into_owned<'x> is input, so I declared it as a generic lifetime parameter.
But I don't like the name 'x. I wanted it to be more explicit, so I chose 'any.
The return type can use 'any, just as the previous code could use 'x in the return type.

Direct implementations of such traits are quite trivial, but always the problem is proc-macro. I need to add 'any to various type definitions, and AST work is always error-prone.

But I know the most important thing. I need to use 'any in the derived definition of Owned. So I just wrote there. The previous code was

      impl #impl_generics lightningcss::traits::IntoOwned for #self_name #ty_generics #where_clause {
        type Owned = #self_name<'static, #(#params),*>;
        /// Consumes the value and returns an owned clone.
        fn into_owned(self) -> Self::Owned {
          use lightningcss::traits::IntoOwned;

          #res
        }
      }

so it became

      impl #impl_generics lightningcss::traits::IntoOwned<'any> for #self_name #ty_generics #where_clause {
        type Owned = #self_name<'any, #(#params),*>;
        /// Consumes the value and returns an owned clone.
        fn into_owned(self) -> Self::Owned {
          use lightningcss::traits::IntoOwned;

          #res
        }
      }

Now the problem collapses. I need to add 'any to #impl_generics because rustc complains that 'any is used, but not declared anywhere.

impl_generics is declared as

let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();

so I prepended 'any to generics. Of course, it was broken. it? Many things were broken. But I made progress, so I didn't care. As 'any is added to generics, ty_generics also has something new, which is a bomb for types without generics in the definition.

#[derive(IntoOwned)]
enum Foo {}

resulted in

impl IntoOwned<'any> for Foo<'any>

so it's clearly wrong.

struct Foo<'a>

resulted in

impl IntoOwned<'any> for Foo<'any, 'a>

so it's also clearly wrong. I fixed this one first. I made it replace instead of prepend if a lifetime parameter exists.

But this was also a stupid decision. If you spot the problem, you are a genius. The problem here is that

impl IntoOwned<'any> for Foo<'any> {
    type Owned = Foo<'any>;
}

is generated, which is... identical to my bad approach. I didn't notice this fact. I didn't have confidence from the start, so I thought #3 might be fundamentally wrong like the others, but I was going to try many things at least for 2 hours. And after some triage, I found that I was replacing 'a in for Foo<'a> to 'any, resulting in for Foo<'any>.

After some thinking, I found that

let orig_generics = generics.clone();

// Modify `generics`

let (impl_generics, _, where_clause) = generics.split_for_impl();
let (_, ty_generics, _) = orig_generics.split_for_impl();

is the way to go.

So after fixing small issues, I undrafted the PR, and it's good for review now. Many people, including my friends, ask me about the way to write code like me. I think the key is always the way you think.

The noticeable points in this task were

I classified enum variants based on characteristics to verify my guess about the reasoning behind no implementation.

When 'static, which should work according to my intuition, didn't work, I tried to find a definition that is a 1-to-1 mapping of something that worked before.

I'm sure that 'any is required in the output, so even if adding 'any to some places resulted in lots of errors, I considered it progress.

Also, trial and error is one of the most important things in programming. One of the best points of almost all programming is that you can try.

P.S.

Most of the people who asked me about coding are Koreans, so I don't know how many of them will read this, but I write in English because I think writing in English will help more people.