TaskLog: Improvement of IntoOwned

Oct 11, 2023

I'm back with lightingcss again! But it's just an example I selected, and what I want to say is:

You need to repeat to learn how to think. Thinking is passing by, and there are so many that you shouldn't think you can use the same way next time just because you've used it once.

Let's see the PR for today.

PR: https://github.com/parcel-bundler/lightningcss/pull/599

This is super complex PR, of course. That's why it deserves the example of these blog posts. If a task is very complex, it's quite easy to analyze my approaches after finishing the work.

Problem

While trying to use the result of the previous work, I found that I forgot to add derives for IntoOwned.

Oops.

But well, I'm very used to doing things like this. This should not be a big deal. So I just opened lightningcss in vscode and added

#[cfg_attr(feature = "into_owned", derive(lightningcss_derive::IntoOwned))]

to various types, including StyleRule. However due to the design decisions in previous work, Selector, which is used as a field of a variant of the StyleRule, cannot implement IntoOwned easily. Fundamentally, Selector is parcel_selectors::Selector and StyleRule is lightningcss::StyleRule where lightningcss depends on parcel_selectors .

So... I thought I needed a separate crate for this, and asked the maintainer. This time, I didn't file an issue. Instead, I just opened a PR.

PR: https://github.com/parcel-bundler/lightningcss/pull/599

One of the suggested solutions by the maintainer was creating a new crate, which is I wanted to do. So I decided to go with the path.

Steps

This was a long journey. It took much longer than initially expected, but I think it was worth it.

Add `derive`, ignoring errors

This step is done before creating a PR.

I know what I want to do. I want to make lightningcss::CssRule , and related types implement IntoOwned. So I just added derive to them.

As a result, I could get a useful error message. I have to implement them for parcel_selector::Selectorand parcel_selector::SelectorList , but I couldn't because of the reason I described above.

So I asked the maintainer and decided the way to go.

New crate, move codes

I needed to create a new package, and naming a project is the hardest thing in the world of programming. But I don't like using lots of time for naming things, so I just selected static-self, which is quite intuitive.

I created two new crates. The first one is the actual package, and the second one is a proc-macro, which makes implementing IntoOwned semi-automatic.

After that, I just copy-pasted codes for IntoOwned. Both the derive macro and the runtime library. There were many red lines in IDE, but I don't care.

I cleaned up the code so I could compile them. These cleanups were about adding relevant dependencies or adding a proper mod into_owned; statement.

Manual impl for various types

IntoOwned is now a separate project, so it deserves the effort. I implemented IntoOwned for Option<T> and Box<T> as the maintainer recommended.

Btw, since it's now a trait, I think a bunch of the special cases (e.g. Option and Box) can also be implemented with generics instead of in the derive macro so it might get a lot simpler.

impl<'any, T> IntoOwned<'any> for Option<T>
where
  T: for<'aa> IntoOwned<'aa>,
{
  type Owned = Option<<T as IntoOwned<'any>>::Owned>;

  fn into_owned(self) -> Self::Owned {
    self.map(|v| v.into_owned())
  }
}
impl<'any, T> IntoOwned<'any> for Box<T>
where
  T: for<'aa> IntoOwned<'aa>,
{
  type Owned = Box<<T as IntoOwned<'any>>::Owned>;

  fn into_owned(self) -> Self::Owned {
    Box::new((*self).into_owned())
  }
}

Those implementations are quite straightforward. I could remove special-case codes for Option<T> and Box<T> in the proc-macro.

Triage to use derive for selectors

As derive is cleaned up, I wanted to use it everywhere.

#[cfg_attr(feature = "into_owned", derive(static_self::IntoOwned))]
pub struct SelectorList<'i, Impl: SelectorImpl<'i>>(
  #[cfg_attr(feature = "serde", serde(borrow))] pub SmallVec<[Selector<'i, Impl>; 1]>,
);

I added it to one of the problematic types. But if we do this, we have more problems.

Impl is a group for actual type parameters, and it's not used as a generic type parameter of a field. However, detecting this from a procedural macro is not a good idea. So, I decided to go with manual implementations for type with Impl: SelectorImpl.

Manual implementation for `Selector`

First, I tried

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>> static_self::IntoOwned<'any> for Selector<'i, Impl>
where
  Impl: static_self::IntoOwned<'any>,
  <Impl as static_self::IntoOwned<'any>>::Owned: SelectorImpl<'any>,
{
  type Owned = Selector<'any, <Impl as static_self::IntoOwned<'any>>::Owned>;

  fn into_owned(self) -> Self::Owned {
    Selector(self.0, self.1.into_owned())
  }
}

to know what it should look like.

Because of the bound, I needed one more implementation of IntoOwned. I did it for Component.

I aliased <Impl as static_self::IntoOwned<'any>>::Owned as Sel, for convenience.

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>, Sel> static_self::IntoOwned<'any> for Selector<'i, Impl>
where
  Impl: static_self::IntoOwned<'any, Owned = Sel>,
  Sel: SelectorImpl<'any>,
{
  type Owned = Selector<'any, Sel>;

  fn into_owned(self) -> Self::Owned {
    Selector(self.0, self.1.into_owned())
  }
}

Also, it became much more intuitive. This time, I also made some stupid mistakes.

Sel is the output type, but I was confused and I tried something like

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>, Sel> static_self::IntoOwned<'any> for Selector<'i, Impl>
where
  Impl: static_self::IntoOwned<'any, Owned = Sel>,
  Sel: SelectorImpl<'any>,
  Sel::NamespaceUrl: for<'aa> IntoOwned<'aa>, // this one
{
  type Owned = Selector<'any, Sel>;

  fn into_owned(self) -> Self::Owned {
    Selector(self.0, self.1.into_owned())
  }
}

This bound is about requesting the IntoOwned::Owned to implement IntoOwned, which is pretty strange.

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>, Sel> static_self::IntoOwned<'any> for Selector<'i, Impl>
where
  Impl: static_self::IntoOwned<'any, Owned = Sel>,
  Sel: SelectorImpl<'any>,
  Impl::NamespaceUrl: for<'aa> static_self::IntoOwned<'aa, Owned = Sel::NamespaceUrl>,
  Impl::NamespacePrefix: for<'aa> static_self::IntoOwned<'aa, Owned = Sel::NamespacePrefix>,
{
  type Owned = Selector<'any, Sel>;

  fn into_owned(self) -> Self::Owned {
    Selector(self.0, self.1.into_owned())
  }
}

After some time, I found that these are the correct bounds. The old NamespaceUrl maps to the new NamespaceUrl, and so on.

But some of the types had problems. I had to write bounds like

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>, NewSel> static_self::IntoOwned<'any>
  for AttrSelectorWithOptionalNamespace<'i, Impl>
where
  Impl: static_self::IntoOwned<'any, Owned = NewSel>,
  NewSel: SelectorImpl<'any>,
  Impl::LocalName: static_self::IntoOwned<'any, Owned = NewSel::LocalName>,
  NamespaceConstraint<(Impl::NamespacePrefix, Impl::NamespaceUrl)>:
    static_self::IntoOwned<'any, Owned = NamespaceConstraint<(NewSel::NamespacePrefix, NewSel::NamespaceUrl)>>,
  ParsedAttrSelectorOperation<Impl::AttrValue>:
    static_self::IntoOwned<'any, Owned = ParsedAttrSelectorOperation<NewSel::AttrValue>>,
{
  type Owned = AttrSelectorWithOptionalNamespace<'any, NewSel>;

  fn into_owned(self) -> Self::Owned {
    AttrSelectorWithOptionalNamespace {
      namespace: self.namespace.into_owned(),
      local_name: self.local_name.into_owned(),
      local_name_lower: self.local_name_lower.into_owned(),
      operation: self.operation.into_owned(),
      never_matches: self.never_matches,
    }
  }
}

Relaxed bounds of generic implementations

I found that for<'aa> IntoOwned<'aa> is too much for bounds of type parameters, because we only need Self<'any>. So, I modified the bound of generic implementations first.

impl<'any, T> IntoOwned<'any> for Vec<T>
where
  T: IntoOwned<'any>,
{
  type Owned = Vec<<T as IntoOwned<'any>>::Owned>;

  fn into_owned(self) -> Self::Owned {
    self.into_iter().map(|v| v.into_owned()).collect()
  }
}

So now if T implements IntoOwned<'a>, Vec<T> implements IntoOwned<'a>. Previously, additional bounds were required because the bound was T: for<'aa> x<'aa> .

Generic impl for `Box<[T]>`

I found that now the problem is Box<[T]>, because [T] is ?Sized and our previous implementation has *self, which cannot be used with T: ?Sized.

After trying to remove the Sized bound, I found that Box<T> where T: Sized and Box<[T]> does not overlap. So I implemented IntoOwnedfor `[

impl<'any, T> IntoOwned<'any> for Box<[T]>
where
  T: IntoOwned<'any>,
{
  type Owned = Box<[<T as IntoOwned<'any>>::Owned]>;

  fn into_owned(self) -> Self::Owned {
    self.into_vec().into_owned().into_boxed_slice()
  }
}

I wrote this, and it worked!

Relax bounds derives

The procedural macro was doing

  for type_param in type_param_names {
    generics.make_where_clause().predicates.push_value(parse_quote! {
      #type_param: 'static + for<'aa> ::static_self::IntoOwned<'aa>
    })
  }

but as we saw above, this is too much. I modified this to

  for type_param in type_param_names {
    generics.make_where_clause().predicates.push_value(parse_quote! {
      #type_param: 'static + ::static_self::IntoOwned<'any>
    })
  }

and it worked like a charm! I thought the PR was done.

`extern crate`

rustc complained about missing extern crate, so I added one.

#[cfg(feature = "into_owned")]
extern crate static_self;

After that, I found that, still, there are many problems.

I did

#[cfg(feature = "into_owned")]
impl<'any> static_self::IntoOwned<'any> for DefaultAtRule {
  type Owned = Self;
  fn into_owned(self) -> Self {
    self
  }
}

and

  #[cfg(feature = "into_owned")]
  impl<'any> ::static_self::IntoOwned<'any> for Selectors {
    type Owned = Self;

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

but these were not enough.

Implement for tuples

For first, there were bounds like

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>, NewSel> static_self::IntoOwned<'any>
  for AttrSelectorWithOptionalNamespace<'i, Impl>
where
  Impl: static_self::IntoOwned<'any, Owned = NewSel>,
  NewSel: SelectorImpl<'any>,
  Impl::LocalName: static_self::IntoOwned<'any, Owned = NewSel::LocalName>,
  NamespaceConstraint<(Impl::NamespacePrefix, Impl::NamespaceUrl)>:
    static_self::IntoOwned<'any, Owned = NamespaceConstraint<(NewSel::NamespacePrefix, NewSel::NamespaceUrl)>>,
  ParsedAttrSelectorOperation<Impl::AttrValue>:
    static_self::IntoOwned<'any, Owned = ParsedAttrSelectorOperation<NewSel::AttrValue>>,
{
  type Owned = AttrSelectorWithOptionalNamespace<'any, NewSel>;

  fn into_owned(self) -> Self::Owned {
    AttrSelectorWithOptionalNamespace {
      namespace: self.namespace.into_owned(),
      local_name: self.local_name.into_owned(),
      local_name_lower: self.local_name_lower.into_owned(),
      operation: self.operation.into_owned(),
      never_matches: self.never_matches,
    }
  }
}

I was sure that (Impl::NamespacePrefix, Impl::NamespaceUrl) was causing at least one problem because I didn't implement IntoOwned for tuple types. So I implemented it, and it was easy.

Commit: https://github.com/parcel-bundler/lightningcss/pull/599/commits/3edcb0e8cd95a322f3b65c228f1568c8bf5f263b

Use `::Owned` for generics

As I had implemented IntoOwned for tuples, I modified the bounds to

#[cfg(feature = "into_owned")]
impl<'any, 'i, Impl: SelectorImpl<'i>, NewSel> static_self::IntoOwned<'any>
  for AttrSelectorWithOptionalNamespace<'i, Impl>
where
  Impl: static_self::IntoOwned<'any, Owned = NewSel>,
  NewSel: SelectorImpl<'any>,
  Impl::LocalName: static_self::IntoOwned<'any, Owned = NewSel::LocalName>,
  Impl::NamespacePrefix: static_self::IntoOwned<'any, Owned = NewSel::NamespacePrefix>,
  Impl::NamespaceUrl: static_self::IntoOwned<'any, Owned = NewSel::NamespaceUrl>,
{
  type Owned = AttrSelectorWithOptionalNamespace<'any, NewSel>;

  fn into_owned(self) -> Self::Owned {
    AttrSelectorWithOptionalNamespace {
      namespace: self.namespace.into_owned(),
      local_name: self.local_name.into_owned(),
      local_name_lower: self.local_name_lower.into_owned(),
      operation: self.operation.into_owned(),
      never_matches: self.never_matches,
    }
  }
}

But there were some more problems, and I used cargo-expand to see the generated code. The problem was that derived IntoOwned implementation was stupid.

#[cfg_attr(feature = "into_owned", derive(static_self::IntoOwned))]
pub enum NamespaceConstraint<NamespaceUrl> {
  Any,

  /// Empty string for no namespace
  Specific(NamespaceUrl),
}

became

    impl<'any, NamespaceUrl> ::static_self::IntoOwned<'any>
    for NamespaceConstraint<NamespaceUrl>
    where
        NamespaceUrl: 'static + ::static_self::IntoOwned<'any>,
    {
        type Owned = Self;
        #[inline]
        fn into_owned(self) -> Self {
            self
        }
    }

meaning IntoOwned cannot change anything about the type. The output type should be <NamespaceUrl as IntoOwned>::Owned , so I patched the procedural macro.

Remove `'static` bound from derive

I faced another error. rustc was complaining about 'static lifetime, so I tried removing 'static from the derive bound.

  for type_param in type_param_names {
    generics.make_where_clause().predicates.push_value(parse_quote! {
      #type_param: 'static + ::static_self::IntoOwned<'any>
    })
  }

is modified to


  for type_param in type_param_names {
    generics.make_where_clause().predicates.push_value(parse_quote! {
      #type_param: ::static_self::IntoOwned<'any>
    })
  }

and it was the answer.

`for<'aa>` => `'any` , in manual impl

I concluded that for<'aa> IntoOwned<'aa> is way too much, so I modified all bounds in manual implementation to use 'any instead.

Then... another problem! This time, the error message is even cryptic.

I didn't think about the associated types of SelectorImpl. rustc insists

Associated types of the SelectorImpl implementation for Selectors do not implement IntoOwned, so Selector<'_, Selectors> does not implement IntoOwned

in a very cryptic language. It's right, but how can I know the cause?

Debugging & Add one assertion

There's a way to go.

#[cfg(feature = "into_owned")]
fn _assert_into_owned() {
  // Ensure that we provide into_owned

  use static_self::IntoOwned;
  fn _assert<'any, T: IntoOwned<'any>>() {}

  _assert::<SelectorList>();
}

With this assertion, rustc shows the real problem. The problem was that types like CSSString<'i> , which are associated types of SelectorImpl, didn't implement IntoOwned.

Such assertion makes many types explicit and rustc kindly shows good error messages for explicit types.

Ah... I forgot to implement IntoOwned for types in SelectorImpl.

More derive

/// A pseudo class.
#[derive(Clone, PartialEq, Eq, Hash)]
#[cfg_attr(
  feature = "serde",
  derive(serde::Serialize, serde::Deserialize),
  serde(tag = "kind", rename_all = "kebab-case")
)]
#[cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema))]
#[cfg_attr(feature = "into_owned", derive(static_self::IntoOwned))]
pub enum PseudoClass<'i> {
  // Lots of variants...
}

I just derived them.

Wrapping up

Finally, it's done! Let's see what I should highlight here.

Notable approaches

Implement at least one item manually before writing a derive macro

I wanted to use derive for Selector and friends, too. But I could conclude it's a waste of time before actually wasting lots of time because I implemented it to Selector and found that it's impractical.

Analyze requirements of generics

Note: In many cases, it's fine to postpone this.

Generic bounds work if you have bounds that are too much. But it's not ideal, and it can cause problems, just like in this case. If such an error happens, don't trust your generic implementation because "it does not error."

Making types explicit to control `rustc`

Typically, the types generated from type inference is useless for human, and many build tools hide them from the user. But sometimes, you want to validate the type. If so, be explicit to the compiler. If you are using a good language, the compiler will show detailed diagnostics for your explicit types.

Conclusion

I always think that it would have been better to do something and how. This is the key to changing the way you think.

P.S.

I'm sure I have a bad approach I wanted to write about, but I forgot what it was.