`EffectivelyPrivate` false positives?

[facboy]

A couple of scenarios:

public methods on inner private classes

class Outside {

  private class Inside {
    public void doSomething() {
      // ...
    }
  }
}

I'm prepared to be told that it's worthwhile warning on doSomething(), but at the moment it seems a bit like noise. It appears to me it's not just 'effectively private', it is private because it's defined in a private inner class?

It will also warn on public (and protected) constructors of private inner classes.

public methods on private inner classes that are extended by public inner classes

class Outside {

  private abstract class InsideParent {
    public void doSomething() {
      // ...
    }
  }

  public OutsideChild extends InsideParent {
    // ... other stuff
  }
}

doSomething() is supposed to be visible and accessible on OutsideChild. This is a fairly common pattern, isn't it?

[cpovirk]

Thanks. Taking your examples in reverse order:

That second case is one that we knew about when writing and reviewing the check, but we have hoped that it comes up rarely enough for us to ignore it. (That said, if I had a little more free time at the moment, I would propose a code change to handle it on principle :)) I'd be interested to know roughly how common it's been for you. I have certainly seen a similar pattern with package-private classes, but for private classes, I at least don't remember an example offhand.

The first case, if I'm understanding it right, is basically the case that this check is designed for: It has a method that is declared as public, which might encourage extra attention to its API or trigger style-guide Javadoc requirements, but it turns out that the method is accessible only within the class, so it would make more sense to treat it like a private method.

I'm interested in whether you have an example in mind of where you'd envision that this check would produce a warning. I ask for two reasons:

• It can help us clarify the docs for this check.
• It might suggest another pattern that we could cover with this check or with a separate check.

[facboy]

thx the prompt response.

Re: the second case, it encountered it in the very first of our projects I upgraded, but that's just anecdotal. I guess I'd make the point that it seems to me that it could be fairly common, and arguably 'better' practice in line with 'keep things private that can be kept private' (i.e. the private abstract parent class). errorprone warning about something that seems perfectly valid, arguably best-practice, and not really a source of errors seems a bit...counter to the purpose of errorprone?

Re: the first case, is the suggestion that it be package-private instead? When I have private inner classes I like to make the distinction on its fields/methods between actually private (only accessed by the inner class) and accessed by other code within the containing class. i usually do this by marking the 'available to other code' methods as public. i think if the argument that the reason for the check is, among other things, 'avoid cognitive load/documentation tasks of treating it as public when it actually isn't' then that could go in the doc for the check? it seems a good justification.

i'm still in two minds about it for this case though, even with the doc/style point...any method on a private inner class is 'effectively private', so on some level this just seems like noise.

not sure what you mean by an example where it would produce a warning? i can't think of one as it relates to private inner classes, or did you mean 'any example at all?' obviously the canonical

public class Foo {

  public void notCalledAnywhereExceptInThisClassBar() {
  }
}

should produce a warning. or did you mean something else?

[JarvisCraft]

I would say that

public class Outer {

    // `Inner` itself is only visible from `Outer`
    private class Inner {
        // `public` implies that I want it to be callable from `Outer`
        public void bar() { ... }

        // `private` implies that I do not want this to be called from `Outer`
        private void baz() { ... }
    }
}

is quite a valid use-case for public which kind of makes this inspection to noisy for a warn-by-default

[cpovirk]

Thanks, and sorry for not being nearly so prompt this time.

I'll still hope to add handling for the second case at some point. (Opinions differ on how good a practice it is, but I will say that I don't think we went into writing this check with the intent of discouraging it.)

That leaves a couple other things:

First, there's the question of using public on members of private classes (well, ones that are not extended by public classes :)) to express intent: The rule could be "Only public members are accessed outside the class." Alternatively, you could use private on the "opposite" set of members. Then the rule could be "private members are not accessed outside the class."

Still, we're generally reluctant to have Error Prone "reinterpret" code in such ways (example), despite some internal advocates for using keywords in a similar way. If you do want to use that approach, though, what are your thoughts on using an explicit private when you don't a member to be used outside the class and then using no explicit modifier when you do? That would quiet EffectivelyPrivate, which cares only about public and protected. And it would mesh well with style rules that require Javadoc on public and protected methods, which could make exceptions for "effectively private" members but wouldn't need to. Similarly, humans who read the code would know what's publicly exposed without needing the context of whether the enclosing class is private or not.

Finally, there's your other example:

public class Foo {

  public void notCalledAnywhereExceptInThisClassBar() {
  }
}

Restricting the visibility of notCalledAnywhereExceptInThisClassBar is a bit outside what Error Prone supports today—and, in some scenarios, what it could ever support.

If Error Prone were building a codebase and all its users together (meaning, say, a self-contained app and its tests, all in one javac invocation), then it could definitively determine when we can safely reduce the visibility of a public member. However, I think this is not the common case. Still, such a check might work reasonably well even if Error Prone sees the prod code alone: There probably aren't too many members that have increased visibility just for tests (though it happens :)). Unfortunately, for technical/implementation reasons, I think we're currently in a world in which we have to decide whether to make changes to file X.java while we're looking at X.java, which is too early if we need to know about calls to methods in X from Y.java.

And of course, if Error Prone is analyzing a common library, then a member might need to be public in order to make it available to library users, even if it's never used outside the package within the library itself.

These cases could still be made to work if we were to provide a way to Error Prone in multiple "rounds." (And the simpler case could perhaps be solved by reworking the implementation.) My guess, unfortunately, is that none of that will become a priority soon, much as I'd like to be able to run the all-purpose visibility restrictor and finalizer on my various projects.

[facboy]

Hmm...it seems I was missing some context.

So my 'other example' wasn't great because as you say it would mess badly with pure 'library' codebases. Possibly it could be used for package-private top-level classes, but you could fall foul of libs that for some reason expect to support 'external' (as in outside the lib) classes with access to its package — that sounds like terrible practice though.

Yes, what you've suggested about just making those public methods package-private works for me, I'm not wedded to having them be public for 'implying intention' reasons, package-private works too. That is largely what i've been doing to fix the warnings. Though for the purposes of scoring debating points, in some ways the check is 'reinterpreting' (or inferring semantics) from keywords isn't it? It's inferring intention from the public keyword in a private inner class, even though the keyword can have no effect precisely because it's used in a private inner class.

I guess I find this specific check slightly jarring, because in my experience all previous errorprone warnings that our code has fallen foul of have been genuine 'you should check this because it can be a source of actual errors.' Whereas this doesn't seem like anything that is actually going to be a source of errors at all, it's almost more of a 'style' preference.

I'll ask a clarifying question though: if the check only works on a single file at a time, it sounds as if the only things it catches at the moment are public/protected methods on private inner classes, as it doesn't check package-private methods. Is that right? So in effect, warning for my 'first case' is the entire purpose of the check? If so, I can just disable it entirely with params if that's the path I want to go down.

[cpovirk]

I tend to think of the purpose of this check as being about removing a small section of code that is unnecessary or potentially misleading. Of course, when I say "potentially misleading," I'm granting you your debating point about inferring intention :) In fairness, I think that it's reasonable to infer the intention "I want people to be able to use this outside the package and outside subclasses" from the keyword public. But then, as you point out, the author "couldn't" have meant that in this situation, and that's why there's some interest in using keywords to express intent here.

I do agree that this is very much a style issue. I'll leave it to people more involved with Error Prone to decide whether this check would be better off demoted to opt-in. (Maybe we'll also get more feedback.) And yes, all that this check flags is protected and public, as in your first example. That sounds like enough reason for you to disable it in your case.

Beyond that, I will still try to address the "second case" discussed above at some point, as well as to flesh out the docs. It might take me a bit, though....

[facboy]

Ok thanks for the feedback. I'll see how I go and whether it really does create a lot of noise across our projects before I decide if it warrants a blanket disable. I don't feel strongly enough about the 'style' one way or another, but equally I don't want to be fixing hundreds of warnings across our codebase either for what seems to be a stylistic preference at the moment.

Feel free to close/leave this open as you see fit.

[commonquail]

I have only a cursory sense of what this check is going to mean for me presently but I definitely consider the example

public class Outer {
  private class Inner {
    // `private` implies that I do not want this to be called from `Outer`
    private void baz() { }
  }
}

to be stylistically poor form, on the basis that that simply is not how Java works. If baz() should not be called from Outer, Inner may not be an inner class. Consequently, the role of baz()'s access modifier is predominantly noisy variance liable to induce confusion about the author's intentions or the language itself, so I welcome the idea of a constraint.

I don't recall off-hand whether Error Prone also rejects

public class Outer { }
class Inner {
  private void baz() { }
}

but that meets the stated objective as long as Inner is not otherwise required to be an inner class.

[graememorgan]

Thanks for the discussion here. Definitely agree with Chris's point that this is very much a stylistic complaint, not a bug. We have a stronger distinction between those two categories of checks inside Google which is a bit lost in the OSS release.

"Visibility is widened by a non-private subclass in the same file" is a nice edge case that I'm happy to take a stab at. I just send a PR for a possibly even more surprising issue that I didn't know about: private members are not accessible to subclasses in the same file:

class Outer {
  private class A {
    protected void foo();
  }
  private class B extends A {
    public void bar() {
      foo(); // `A.foo` cannot be private
    }
  }
}

[cpovirk]

Thanks, @graememorgan! I have had this thread starred for weeks now but haven't gotten any closer to doing anything about it.

@kluever and I came across the same wrinkle around subclasses in CL 783812775 (a.k.a. google/guava#7875). But since our check hasn't been making things private, I'm not sure I understand what you're looking to change: Making A.foo package-private, as we do now, should be fine, right? (I snooped on your CL 808766517 but was not instantaneously enlightened.)

[facboy]

ha, slightly confusing, my actual name is chris too lol.

it hasn't been hugely burdensome so far 'just fixing it' when necessary, i haven't yet given up and disabled it on any thing.

[JarvisCraft]

Chrises in this thread be like: