cvogt / cbt

CBT - fun, fast, intuitive, compositional, statically checked builds written in Scala
Other
488 stars 60 forks source link

cbt should only show public members of BaseBuild & co #522

Open megri opened 7 years ago

megri commented 7 years ago

Currently, protected members are shown as valid arguments when running cbt. The issue is most likely with this method: https://github.com/cvogt/cbt/blob/618711302b7ea29de651c1f771eb3160e236b339/stage2/Lib.scala#L56

The method above uses Java reflection to reflect on public members, but should be using scala.reflect for proper introspection

cvogt commented 7 years ago

Would be great to have more info with an example and the pain this is causing. Classifying it as backlog for now.

cvogt commented 7 years ago

we should evaluate how performance is affected if using scala reflect here

megri commented 7 years ago
Methods provided by Build(/Users/martin/dev/scala/cbt-test/splaintest)

All  Bounds  BoundsImplicits  BreakInfix  Color  Compact  FoundReq  Implicits  Infix  Off  On  Tree  TruncRefined  splainParams  splainVersion

That, basically. It makes sense (to me) to have these case classes readily available as soon as you extend a trait, but they only make sense in the scope of the trait: they shouldn't leak out as cbt arguments

cvogt commented 7 years ago

Why not use protected then? Java reflection picks up on that. Anyways, I'd opt for letting users import things explicitly and not using the OO-scopes too much to inject names. Supportive of structuring things for convenient wild card imports though.

megri commented 7 years ago

Why not use protected then? Java reflection picks up on that.

Either Scala is encoding things differently than java reflection expects, or I'm doing something wrong. On a side-note, as protected[this] is more restrictive than protected it would make sense if Scala actually encoded them as protected members in the bytecode.

object Foo {
  protected val bar = 1
  protected def baz = 2
}

Foo.getClass.getMethods.map {m =>
  val isProtected = java.lang.reflect.Modifier.isProtected(m.getModifiers)
  val isPublic = java.lang.reflect.Modifier.isPublic(m.getModifiers)

  s"${m.getName}: protected -> $isProtected, public -> $isPublic"
} // Array(bar: protected -> false :: public -> true, baz: protected -> false :: public -> true, wait: protected -> false :: public -> true, wait: protected -> false :: public -> true, …)
cvogt commented 7 years ago

@megri ah, that's entirely possible. does protected[this] or private[this] show up via reflection?

megri commented 7 years ago

protected[this] shows up as public. private and private[this] is private to java reflection.

cvogt commented 7 years ago

Mh maybe scala protected isn't encoded in Java then. Interesting. I thought it would be.

On June 16, 2017 6:03:52 PM EDT, megri notifications@github.com wrote:

protected[this] shows up as public. private and private[this] is private to java reflection.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/cvogt/cbt/issues/522#issuecomment-309144928

megri commented 7 years ago

I have a working prototype at home, I'll commit it so you can have a look when I'm back :) Performance may be worse than Java but I doubt it'll be noticeable from the user perspective

cvogt commented 7 years ago

cool :)!

megri commented 7 years ago

Sorry for not getting back sooner.

So what I have right now is a way to reflect on Scala type members a la carte. My issue is that tying this logic into the existing code is troublesome — there's a lot of loosely typed stuff floating around and I'm getting odd errors (nothing is found) when trying to retrofit my solution.

Anyway, here is what I've come up with so far

import scala.reflect.ClassTag
import scala.reflect.runtime.universe

def methodsOf[T: universe.TypeTag](cls: Class[T]) = {
  implicit val ct = ClassTag[T](cls)
  val mirror = universe.runtimeMirror(cls.getClassLoader)

  def declaredIn(tpe: universe.Type) = {
    tpe
      .decls
      .collect {
        case m if 
          !m.isConstructor && 
          m.isPublic && // comment me out to get exceptions!
          m.isMethod && 
          m.asMethod.paramLists.forall(_.isEmpty) =>

          val methodF: T => Any = instance => mirror.reflect(instance).reflectMethod(m.asMethod).apply()
          m.name.decodedName.toString -> methodF
      }
      .toMap
  }

  mirror
    .typeOf[T]
    .baseClasses
    .dropRight(2) // drop … <: Object <: Any
    .foldLeft(Map.empty[String, T => Any]) {
    case (map, symbol) =>
      map ++ declaredIn(symbol.typeSignature)
  }
}

// test
trait Foo {
  def takesParameters(x: Int) =
    throw new Exception("only arity-0 members should show up")
}

trait Bar {
  protected def protectedMember =
    throw new Exception("protected members should not be visible")
}

class Impl extends Foo with Bar {
  lazy val everything = "is ok!"
}

val instance = new Impl

methodsOf(classOf[Impl]).map { case (key, f) => key -> f(instance) }
megri commented 7 years ago

What do we make of this? :)

cvogt commented 7 years ago

you mean when trying to get it into CBT? Just create PRs, I am happy to look into the errors.

megri commented 7 years ago

Alright, I'll retrofit is as far as I can and create a branch!