hide private classes and methods #47

mengxr · 2015-02-05T02:56:45Z

I'm not sure whether this is the correct way to hide private classes and methods from generated Java docs. It seems to work on Apache Spark.

CC: @rxin

viktorklang · 2015-03-13T19:28:37Z

Ping @rkuhn

rkuhn · 2015-03-13T19:35:31Z

Oh, thanks Viktor, it seems that I don’t get notifications anymore. Will have to defer deeper look until after my ScalaDays presentation is at least somewhat in shape.

dragos · 2015-03-13T21:27:27Z

I think this is a good improvement, but it currently fails tests, for example:

[error] /Users/dragos/workspace/git/genjavadoc/tests/target/java/akka/rk/buh/is/it/PTrait.java:5: error: modifier private not allowed here
[error] private  interface PTrait {
[error]          ^
[error] 5 errors

It would be good to also add some tests to illustrate how this is supposed to work. On top of my head, private[qualifier] does not translate to private members on the JVM (unless qualifier == this), so what is the correct thing to do for genjavadoc? Should it follow the scalac encoding (what it does right now), or it should follow the Scala semantics (and discourage usage from Java). I believe this PR wants to implement the second behavior, but it should probably have a flag to turn it on explicitly.

rkuhn · 2015-03-13T21:57:02Z

Without having looked at the details yet the intention is to follow Scala semantics: there should not be any docs for methods that should actually not be accessible (but are due to lack of byte-code expressiveness).

2m · 2015-03-14T00:28:53Z

Closing and reopening for PR validation to kick in.

mengxr · 2015-03-14T01:22:28Z

I didn't update the tests because we may want to discuss the semantics first. If the target here is to generate the JavaDoc, I think we should hide all package private classes/methods. Those are not supposed to be called by Scala users. Java users should not see them either. If we agree on this, I'll go and update the tests.

mengxr · 2015-03-14T01:39:59Z

I mapped Scala's package private to Java's empty modifier, so Java compiler won't complain on private class or private interface.

dragos · 2015-03-14T16:13:13Z

plugin/src/main/scala/com/typesafe/genjavadoc/AST.scala

@@ -97,14 +97,16 @@ trait AST { this: TransformCake ⇒
    if (m.isPublic) "public"
    else if (m.isProtected && !topLevel) "protected"
    else if (m.isPrivate && !topLevel) "private"
-    else "public" // this is the case for “private[xy]” and top level classes
+    else if (m.privateWithin != tpnme.EMPTY) ""


privateWithin may point to any enclosing definition (this, enclosing class or package, and packages nest). I think you'd need to consider the following mappings:

package org.example class Outer { class Inner { private[this] val x private[Inner] val y private[Outer] val z private[example] val t private[org] val u } }

private[this] --> private

private[Inner] --> private

private[Outer] --> ?? (I guess private again)

private[org|example] --> ""

What do you think?

private[org] val u under org.example has to be public in Java. Otherwise, if there exists references to u under org, the Java code won't compile. Is it correct?

I'm not very sure about how java doc works. Does it expect everything to type-check according to Java rules? If that's the case, I think this comment would apply everywhere Scala makes something public in classfiles that was private in source. So probably none of the changes here would be safe to do. AFAIK the compiler does not make anything public just for tha sake of it.

rkuhn · 2015-03-17T16:08:54Z

Sorry for keeping you waiting so long, but I’ll have to defer really looking into this until after ScalaDays.

dragos · 2015-06-15T15:05:09Z

@rkuhn could you have a look at this one now that ScalaDays is behind us?

rkuhn · 2015-06-18T06:12:11Z

Currently a bit difficult, how urgent is it?

rkuhn · 2015-06-25T15:19:42Z

Hmm, trying this out I find that Scalac’s behavior does not match my previous understanding: I thought that protected[<package>] should result in package protected Java byte-code, but that is not the case. Having the emitted Java code be stricter than the real byte-code will result in javadoc failures, especially with the extremely picky updates in JDK8, so I’m wary to tighten things up here.

Is this really needed?

dragos · 2015-07-03T10:43:05Z

Could we put this behind a flag? It seems to be useful at least for projects that don't use the fact that Scala packages nest (so no private[org] or the like). @mengxr what do you think? Would this remove the need for the Spark fork?

mengxr · 2015-07-05T23:21:33Z

@rkuhn @dragos Yes, based on the discussion I think using flags would be a reasonable solution. Then we can remove our fork for Spark. What flags do you recommend? For Spark use cases, we only need to hide package private traits/classes and methods from the generated JavaDoc. So maybe we can start with making these configurable first. If people ask for more, we could add others in the future. Does it sound good to you? I'm not familiar with the code base. It would be really helpful if you can show me an existing flag implementation in genjavadoc.

dragos · 2015-07-06T09:01:11Z

@mengxr I think this would be the first instance of a flag, and your suggestion makes sense to me. I'll let @rkuhn comment, this was only my take, but I'm not the author/maintainer.

rkuhn · 2015-07-06T15:28:04Z

Yes, providing this capability behind a flag would be a good compromise. There is one challenge, though, and that is how to test this feature then. I guess it needs another subproject next to tests because it needs to compile some files with the feature flag and then verify that the right Java sources drop out—we don’t need to check for classfile equivalence because that would be (intentionally) violated anyway.

@mengxr would you mind changing this PR towards such a scheme? Thanks a lot for contributing and for bearing with my slowness.

Concerning a release: my plan was to push out 1.0 last week but I discovered some more bugs that need fixing first :-( This week I’m at CurryOn so I might not get around to this.

dragos · 2015-07-07T09:31:04Z

Sorry to hijack this thread, but @mengxr can you please publish a 2.11.7 version of your fork? See my PR stalled for more than a week now: apache/spark#6903

mengxr · 2015-08-05T15:42:42Z

Sorry that I didn't have time to work on this. I will try to take a look next week.

mengxr · 2016-03-17T17:29:52Z

cc: @jodersky

jodersky · 2016-03-23T22:13:43Z

@mengxr I'm in the process of rebasing your changes and solving conflicts. Was the only issue stopping a merge the missing flag (and tests)?

mengxr · 2016-03-23T23:02:43Z

Yes, I guess so. We could make it more general, e.g., allow customizing scope mapping. But for Spark releases, we only need a boolean feature flag for hiding package private classes/members in the generated Java API doc.

jodersky · 2016-03-24T22:23:10Z

@dragos, what would be the best way to implement a flag for the features discussed in this PR?
I guess it's more of a general question, what's the convention for passing flags to compiler plugins?

jodersky · 2016-03-24T22:26:06Z

never mind, it was right under my nose

Private access visibility (follow-up to #47)

hide private classes and methods

ae6cd6a

2m closed this Mar 14, 2015

2m reopened this Mar 14, 2015

mengxr added 2 commits March 13, 2015 18:31

package private to no modifier

6625fb6

generate expected output

d11b052

dragos reviewed Mar 14, 2015
View reviewed changes

thunterdb mentioned this pull request Nov 18, 2015

Missing package private information in the AST with current master #62

Open

set version to 0.9-spark0 and cross build with 2.10.6 and 2.11.8

a5c52d5

jodersky mentioned this pull request Mar 24, 2016

Private access visibility (follow-up to #47) #70

Merged

mengxr closed this Mar 25, 2016

patriknw mentioned this pull request Mar 31, 2016

Try genjavadoc "hide private members" akka/akka#20185

Closed

2m added a commit that referenced this pull request Apr 15, 2016

Merge pull request #70 from jodersky/mengxr-private

4b410ab

Private access visibility (follow-up to #47)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hide private classes and methods #47

hide private classes and methods #47

mengxr commented Feb 5, 2015

viktorklang commented Mar 13, 2015

rkuhn commented Mar 13, 2015

dragos commented Mar 13, 2015

rkuhn commented Mar 13, 2015

2m commented Mar 14, 2015

mengxr commented Mar 14, 2015

mengxr commented Mar 14, 2015

dragos Mar 14, 2015

mengxr Mar 15, 2015

dragos Mar 17, 2015

rkuhn commented Mar 17, 2015

dragos commented Jun 15, 2015

rkuhn commented Jun 18, 2015

rkuhn commented Jun 25, 2015

dragos commented Jul 3, 2015

mengxr commented Jul 5, 2015

dragos commented Jul 6, 2015

rkuhn commented Jul 6, 2015

dragos commented Jul 7, 2015

mengxr commented Aug 5, 2015

mengxr commented Mar 17, 2016

jodersky commented Mar 23, 2016

mengxr commented Mar 23, 2016

jodersky commented Mar 24, 2016

jodersky commented Mar 24, 2016

hide private classes and methods #47

hide private classes and methods #47

Conversation

mengxr commented Feb 5, 2015

viktorklang commented Mar 13, 2015

rkuhn commented Mar 13, 2015

dragos commented Mar 13, 2015

rkuhn commented Mar 13, 2015

2m commented Mar 14, 2015

mengxr commented Mar 14, 2015

mengxr commented Mar 14, 2015

dragos Mar 14, 2015

Choose a reason for hiding this comment

mengxr Mar 15, 2015

Choose a reason for hiding this comment

dragos Mar 17, 2015

Choose a reason for hiding this comment

rkuhn commented Mar 17, 2015

dragos commented Jun 15, 2015

rkuhn commented Jun 18, 2015

rkuhn commented Jun 25, 2015

dragos commented Jul 3, 2015

mengxr commented Jul 5, 2015

dragos commented Jul 6, 2015

rkuhn commented Jul 6, 2015

dragos commented Jul 7, 2015

mengxr commented Aug 5, 2015

mengxr commented Mar 17, 2016

jodersky commented Mar 23, 2016

mengxr commented Mar 23, 2016

jodersky commented Mar 24, 2016

jodersky commented Mar 24, 2016