spark-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pwendell <...@git.apache.org>
Subject [GitHub] spark pull request: [SPARK-2069] MIMA false positives
Date Wed, 11 Jun 2014 03:45:05 GMT
Github user pwendell commented on a diff in the pull request:

    https://github.com/apache/spark/pull/1021#discussion_r13632707
  
    --- Diff: tools/src/main/scala/org/apache/spark/tools/GenerateMIMAIgnore.scala ---
    @@ -40,74 +40,73 @@ object GenerateMIMAIgnore {
       private val classLoader = Thread.currentThread().getContextClassLoader
       private val mirror = runtimeMirror(classLoader)
     
    -  private def classesPrivateWithin(packageName: String): Set[String] = {
    +
    +  private def isDeveloperApi(sym: unv.Symbol) =
    +    sym.annotations.exists(_.tpe =:= unv.typeOf[org.apache.spark.annotation.DeveloperApi])
    +
    +  private def isExperimental(sym: unv.Symbol) =
    +    sym.annotations.exists(_.tpe =:= unv.typeOf[org.apache.spark.annotation.Experimental])
    +
    +
    +  private def isPackagePrivate(sym: unv.Symbol) =
    +    !sym.privateWithin.fullName.startsWith("<none>")
    +
    +  private def isPackagePrivateModule(moduleSymbol: unv.ModuleSymbol) =
    +    !moduleSymbol.privateWithin.fullName.startsWith("<none>")
    +
    +  private def classesPrivateWithin(packageName: String): (Set[String], Set[String]) =
{
     
         val classes = getClasses(packageName)
         val ignoredClasses = mutable.HashSet[String]()
    +    val ignoredMembers = mutable.HashSet[String]()
     
    -    def isPackagePrivate(className: String) = {
    +    for (className <- classes) {
           try {
    -        /* Couldn't figure out if it's possible to determine a-priori whether a given
symbol
    -           is a module or class. */
    -
    -        val privateAsClass = mirror
    -          .classSymbol(Class.forName(className, false, classLoader))
    -          .privateWithin
    -          .fullName
    -          .startsWith(packageName)
    -
    -        val privateAsModule = mirror
    -          .staticModule(className)
    -          .privateWithin
    -          .fullName
    -          .startsWith(packageName)
    -
    -        privateAsClass || privateAsModule
    -      } catch {
    -        case _: Throwable => {
    -          println("Error determining visibility: " + className)
    -          false
    +        val classSymbol = mirror.classSymbol(Class.forName(className, false, classLoader))
    +        val moduleSymbol = mirror.staticModule(className) // TODO: see if it is necessary.
    +        val directlyPrivateSpark =
    +          isPackagePrivate(classSymbol) || isPackagePrivateModule(moduleSymbol)
    +        val developerApi = isDeveloperApi(classSymbol)
    +        val experimental = isExperimental(classSymbol)
    +
    +        /* Inner classes defined within a private[spark] class or object are effectively
    +         invisible, so we account for them as package private. */
    +        lazy val indirectlyPrivateSpark = {
    +          val maybeOuter = className.toString.takeWhile(_ != '$')
    +          if (maybeOuter != className) {
    +            isPackagePrivate(mirror.classSymbol(Class.forName(maybeOuter, false, classLoader)))
||
    +              isPackagePrivateModule(mirror.staticModule(maybeOuter))
    +          } else {
    +            false
    +          }
    +        }
    +        if (directlyPrivateSpark || indirectlyPrivateSpark || developerApi || experimental)
{
    +          ignoredClasses += className
    +        } else {
    +          // check if this class has package-private/annotated members.
    +          ignoredMembers ++= getAnnotatedOrPackagePrivateMembers(classSymbol)
             }
    -      }
    -    }
     
    -    def isDeveloperApi(className: String) = {
    -      try {
    -        val clazz = mirror.classSymbol(Class.forName(className, false, classLoader))
    -        clazz.annotations.exists(_.tpe =:= unv.typeOf[org.apache.spark.annotation.DeveloperApi])
           } catch {
    -        case _: Throwable => {
    -          println("Error determining Annotations: " + className)
    -          false
    -        }
    +        case _: Throwable => println("Error instrumenting class::" + className)
           }
         }
    +    (ignoredClasses.flatMap(c => Seq(c, c.replace("$", "#"))).toSet, ignoredMembers.toSet)
    +  }
     
    -    for (className <- classes) {
    -      val directlyPrivateSpark = isPackagePrivate(className)
    -      val developerApi = isDeveloperApi(className)
    -
    -      /* Inner classes defined within a private[spark] class or object are effectively
    -         invisible, so we account for them as package private. */
    -      val indirectlyPrivateSpark = {
    -        val maybeOuter = className.toString.takeWhile(_ != '$')
    -        if (maybeOuter != className) {
    -          isPackagePrivate(maybeOuter)
    -        } else {
    -          false
    -        }
    -      }
    -      if (directlyPrivateSpark || indirectlyPrivateSpark || developerApi) {
    -        ignoredClasses += className
    -      }
    -    }
    -    ignoredClasses.flatMap(c => Seq(c, c.replace("$", "#"))).toSet
    +  private def getAnnotatedOrPackagePrivateMembers(classSymbol: unv.ClassSymbol) = {
    +    classSymbol.typeSignature.members
    +      .filter(x => isPackagePrivate(x) || isDeveloperApi(x) || isExperimental(x)).map(_.fullName)
       }
     
       def main(args: Array[String]) {
    -    scala.tools.nsc.io.File(".generated-mima-excludes").
    -      writeAll(classesPrivateWithin("org.apache.spark").mkString("\n"))
    -    println("Created : .generated-mima-excludes in current directory.")
    +    val privateWithin = classesPrivateWithin("org.apache.spark")
    --- End diff --
    
    might be a bit more clear to do:
    
    ```
    val (privateClasses, privateMembers) = classesPrivateWithin("org.apache.spark")
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message