Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
megri opened this issue Jun 15, 2017 · 14 comments
Open

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

megri opened this issue Jun 15, 2017 · 14 comments

Comments

@megri
Copy link
Contributor

megri commented Jun 15, 2017

Currently, protected members are shown as valid arguments when running cbt. The issue is most likely with this method:

def taskMethods(cls:Class[_]): Map[String, Method] =

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

@cvogt
Copy link
Owner

cvogt commented Jun 15, 2017

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

@cvogt cvogt added this to the backlog milestone Jun 15, 2017
@cvogt
Copy link
Owner

cvogt commented Jun 15, 2017

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

@megri
Copy link
Contributor Author

megri commented Jun 16, 2017

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
Copy link
Owner

cvogt commented Jun 16, 2017

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
Copy link
Contributor Author

megri commented Jun 16, 2017

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
Copy link
Owner

cvogt commented Jun 16, 2017

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

@megri
Copy link
Contributor Author

megri commented Jun 16, 2017

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

@cvogt
Copy link
Owner

cvogt commented Jun 16, 2017 via email

@megri
Copy link
Contributor Author

megri commented Jun 17, 2017

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
Copy link
Owner

cvogt commented Jun 17, 2017

cool :)!

@megri
Copy link
Contributor Author

megri commented Jun 26, 2017

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
Copy link
Contributor Author

megri commented Jun 26, 2017

What do we make of this? :)

@cvogt
Copy link
Owner

cvogt commented Jun 26, 2017

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

@megri
Copy link
Contributor Author

megri commented Jun 26, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants