typelevel / simulacrum

First class syntax support for type classes in Scala
BSD 3-Clause "New" or "Revised" License
936 stars 61 forks source link

Ops doesn’t always import the typeclass instance when it needs to. #55

Open sellout opened 8 years ago

sellout commented 8 years ago

Right now, something like

@typeclass trait Super[A] {
  type T
}
@typeclass trait Sub[A] extends Super[A] {
  def foo(a: A): T
  def bar(a: A): T
}

Will fail to compile because the generated Ops class for Sub[A] doesn’t import the typeclass instance, so it can’t find T. The current implementation only imports the typeclass instance if it finds a type in the immediate trait, not a super-trait.

Which led me to a workaround …

@typeclass trait Super[A] {
  type T
}
@typeclass trait Sub[A] extends Super[A] {
  type T0 = T       // add an alias for the super-trait’s type
  def foo(a: A): T0 // and use it at least once
  def bar(a: A): T
}

Simulacrum now sees that the trait defines a type that is referenced, so it imports the typeclass instance giving access to the super-trait’s type as well.

yilinwei commented 8 years ago

Hmm. This one is a strange issue. I seem to remember that if you extend a trait it should inherit the type aliases - in fact I've made use of this fact in some of my older macros. I should have some time to look at this in the next week to see how simulacrum is doing the expansion with how it differs.

Jasper-M commented 7 years ago

It used to always import typeClassInstance._ into Ops. Why was this changed?

ariwaranosai commented 7 years ago

Perhaps they want to avoid some warning for useless import.

paulp commented 7 years ago

Yes, probably to avoid useless import warnings (aka errors under -Xfatal-warnings.) Unfortunately there is no correct way to import only when it's needed this because simulacrum acts on untypechecked trees. There's no way to know what identifiers you inherit from your parents without knowing who your parents are. It only works in the non-inheritance case by searching the untyped tree for type declarations which match a type reference elsewhere in the class, so even that one is sketchy.

Sad to say, the most straightforward approach which comes to mind is always importing the typeclasses and then using scalafix to elide the useless imports. But after examining scalacenter/scalafix#82, scalacenter/scalafix#83, scalacenter/scalafix#84, the madness of piling hacks upon hacks should be clear enough.

paulp commented 7 years ago

A more realistic possibility is to add a third optional argument to @typeclass which lets the programmer specify parents which need to be imported - there's already e.g. @typeclass(excludeParents = List("Show")) so add importParents.

sellout commented 7 years ago

Yes, too bad you can’t @SuppressWarnings on Scalac’s own warnings …

paulp commented 7 years ago

"Too" "bad"