Fix issues #4 and #5

[jmg-duarte]

This PR addresses both issues submitted @tyranron

It adds support for:

• Several traits per module
• Generic parameters
• Foreign types
• Blanket impls
  It also adds tests and examples based on the issues.

@tyranron I'd like that you took a look at this before I merge it to ensure it fully addresses your concerns.

[danielhenrymantilla]

Nice job, LGTM ✅

• Nit: given that trait names are good old ASCII, pulling the full heck dependency may be a bit overkill; indeed, consider:

  fn to_snake_case (s: &'_ str)
    -> String
  {
      let mut ret = String::with_capacity(s.len());
      let mut first = true;
      s.bytes().for_each(|c| {
          if c.is_ascii_uppercase() {
              if !first {
                  ret.push('_');
              }
              ret.push(c.to_ascii_lowercase() as char);
          } else {
              ret.push(c as char);
          }
          first = false;
      });
      ret
  }

[tyranron]

@jmg-duarte the overall implementations seems to be legit enough.

I've described some possible minor improvements.

[tyranron]

Also, an important thing is to use super::* in the generate module. Since, well, we have trait bounds:

Playground

[danielhenrymantilla]

Indeed! That being said, use super::*; breaks https://rust-lang.github.io/api-guidelines/macros.html#item-macros-work-anywhere-that-items-are-allowed-c-anywhere, so if it can be avoided it will be a win. In this instance, it can be avoided by removing the bounds, so it's mostly a matter of adding:

// Only keep the introduced params (no bounds), since
// the bounds may break in the `#seal` submodule.
let trait_generics = trait_generics.split_for_impl().1;

before the quote! invocation

[tyranron]

@jmg-duarte @danielhenrymantilla if so, we need to consider implicit Sized. So, something like this would work:

#[sealed]
pub MyTrait<T: ?Sized>: MyBounds {}

// As described above will desugar as:
pub MyTrait<T: ?Sized>: MyBounds + private::Sealed<T> {}
mod private {
    pub Sealed<T> {}
}

// Which breaks the original trait:
impl private::Sealed<dyn Error> for MyType {} // compiler error

// If we remove bounds, we should allways apply `?Sized` to type params,
// to be transparent asap:
pub MyTrait<T: ?Sized>: MyBounds + private::Sealed<T> {}
mod private {
    pub Sealed<T: ?Sized> {}
}

[danielhenrymantilla]

Ah, true! The Sealed trait could then be made generic over ?Sized types to avoid this; although I do admit that at this point using use super::*; (so as to forward all the bounds) at the cost of not supporting being applied inside a function's body may be the simpler way to go. I don't have strong preferences anymore.

[tyranron]

@jmg-duarte @danielhenrymantilla I'd prefer to start with a simpler use super::* solution and mess around only if there will be any real cases working badly this way.

[jmg-duarte]

I think there are no outstanding items to take care of. If you don't mind, please take a look at the current code @tyranron @danielhenrymantilla

[danielhenrymantilla]

I think that leaving the where clauses in the impl is mandatory, actually, so let's try to tackle that before merging 🙂

[danielhenrymantilla]

I am curious about this, what problem were the where clauses causing?

Basically the example I have in mind is:

use ::core::future::Future;

#[sealed]
trait GeneratedBy<F> {}

#[sealed]
impl<F> GeneratedBy<F> for F::Output
where
    F : Future,
{}

That is, the #where_clauses may need to be present for #self_type to be well-defined.

• Another maybe less contrived example:

  #[sealed]
  trait Trait {}
  
  struct MyWrapper<T : Copy>(T);
  
  #[sealed]
  impl<T> Trait for MyWrapper<T>
  where
      T : Copy,
  {}

• And more generally, a less constrained impl will allow downstream users to circumvent the whole seal:

  pub trait Is { type EqTo : ?Sized; }
  impl<T : ?Sized> Is for T { type EqTo = Self; }
  
  #[sealed]
  pub trait Trait {}
  
  #[sealed]
  impl<T> Trait for T
  where
      T : Is<EqTo = ()>, // where T = ()
  {}

  without the where bounds, we'd end up with impl<T> Sealed for T {}

• This makes me also realize the case of unbounds:

  #[sealed]
  trait Trait {}
  
  #[sealed]
  impl<T> Trait for T where T : ?Sized {}

[jmg-duarte]

I remember there was a reason you pointed out. But maybe I just misunderstood and took them out.
I'll put them back in, my bad!

[jmg-duarte]

It was this comment
#6 (comment)

[danielhenrymantilla]

Yeah I think a distinction must be made between "necessary bounds to ever impl Sealed" (the bounds on the trait definition), which, in that comment, I claimed they are not necessary (those will already be enforced by such bounds on the annotated trait) –with the exception of the ?Sized "unbound", which is the only bound that actually loosens genericity. So, technically, the Sealed definition ought to carry ?Sized unbounds on all its generic type parameters, and does not need anything more.

Each impl Sealed, however, must have the same degree of generality as the annotated impl, so it has to carry / forward all the bounds that the original impl had. This is less problematic than the trait definition, because we no longer need to be in a submodule and can thus just copy-paste, verbatim, such bounds without issues (whereas for the trait def, we cannot forward the clauses from without a submodule without a use super::*;, and such import breaks when it happens inside a function's body).

The situation is a bit subtle, granted but by thinking about this in advance we will be dodging many potential bugs / issues 🙂

[jmg-duarte]

I think the latest commit might appease to both sides.

I've defaulted to use super and, when bound erasure is necessary one can add the erase argument.