private fields and private methods are not private

A serious ramification of this is that "text" is considered a javaBean property and so it can be found in x.properties . I'm using grails and partially because of this problem I've found that grails will end up calling my private getters because it thinks it's a javabean property when I really don't think it should.

It should be noted it is just the private methods and not private fields that are exposed.

class Test {
   private textField = 'a'
   private def getText() { textField }
   private void setText( text ) { textField = text * 10 }
   int getFoo() { 0 }
}

def t = new Test()

// The following ought to fail (but currently succeed)...

assert (t.properties.keySet() == ['foo', 'metaClass', 'text', 'class'] as Set)
t.text = 'Z'
println t.text

// This should be what works...

assert (t.properties.keySet() == ['metaClass', 'foo', 'class'] as Set)

This is even more insidious and serious than I thought.

While the previous example shows that a private field is not included in a class's property list, the those private fields can still be accessed (both get and set). So this should fail but does not:

class X {
private String z;
}

def x = new X()

x.z = '123'

For an even more compelling case, consider java.lang.String:

def s = '12'
s.value = 'ABCD'
println s
==>
AB

Yikes!

I vote we close this hole in 1.6. If it turns out to be a too big an incompatiblity for some users, then we can have a flag to enable the old behavior for those folks.

That means that final fields are no longer final!

That means that final fields are no longer final!

That is not the case as final works by an entirely different mechanism.

Fortunately there is a simple fix for the issue with private. As Jochen points out, this problem wasn't fixed in Groovy before because it was thought to be necessary for compilation of closures. In is instructive to notice though that Java has the same issues with private members and inner classes.

While dealing with all of the finer points of that will have to wait for <http://jira.codehaus.org/browse/GROOVY-69>, we can take some of their tricks such as providing package scope access to private members when needed.

Which in the current compiler probably means always, so rather than creating hidden methods as Java does, I'd say just have private compiled as package.

Groovy already doesn't respect private, so there is no loss by compiling them to package, and there is great gain in restoring the meaning of private.

But the 'value' field of java.lang.String is final.
And the above code changes the content of a final field, ain't it?

Java has the same problems with private members and inner classes, but the difference is that Java resolves the name at compile time, while inside the closure we resolve the name at runtime. Because of this Java can provide an access method that will be used by the inner class only. We can provide such a method too, but only for the closure? Only in what cases? The case is just not as trivial as it seems. Jim... I just don't get why changing private to package access will solve anything. I told you that on the mailing list already.

Victor... true the field is final and it should not have been changed. But that is a different issue. Don't mix multiple cases in the same issue, that leads to confusion and makes the tracking problematic. Not respecting final here is a bug

Jochen, you are right. Sorry. Just tried to find an existing bug now, but didn't succeed. I therefore created GROOVY-2774.
Perhaps GROOVY-1628 or GROOVY-1475 are caused by the same root cause. But I am not sure. And I think it is quite critical, that an immutable class' contract is violated

We can provide such a method too, but only for the closure? Only in what cases? The case is just not as trivial as it seems.

It is quite simple, because as I said the thing to do given the current Groovy compiler is to compile all 'private' members as 'package'.

If in the future Groovy gets a smarter compiler, like one that can also handle Java inner classes, then members that are known to never be referenced by a closure can be compiled as private again.

true the field is final and it should not have been changed

The final field is not changed. The value in the referenced array was changed. So I repeat, that aspect is completely valid and is not bug. If Groovy had actually assigned a new value to the 'value' field then that would be something else entirely (and would be the bug Victor thinks).

Jim... why don't you explain what we get from compiling "private" as "package private"? As it is atm, the class is at last correct from a Java view.

As for the final char[]... I can changes elements of the array in Java, but I can not set a new array. The code here sets a new array. The only reason that only 2 characters is a special ability of the String class. To safe memory a String can be a substring of another String. This is done by referencing the other char[]. But since there is a new offset and length for the substring String uses these values additionally to the char[]. To understand what I mean you may want to have a look at this script:

def s = "12"
def chars = s.value
s.value = "ABCD"
assert !chars.is(s.value)
assert s == "AB"
s.count=3
assert s == "ABC"
s.offset = 1
assert s == "BCD"

def s2 = "0123"
s = s2.substring(0,2)
assert s.value.is(s2.value)
s2.value = "ABCD"
assert s2 == "ABCD"
assert s == "01"

as you can take from "assert Unable to render embedded object: File (chars.is(s.value)" a new value is really assigned here) not found.

Btw, inner classes are planed for 1.8 already.

why don't you explain what we get from compiling "private" as "package private"? As it is atm, the class is at last correct from a Java view.

Groovy currently destroys the Java class model by ignoring private scope. It is a big enough leap for many enterprises to accept dynamic typing as a change from standard Java static typing. But by making it impossible to have safely operating API implementations this issue alone will make Groovy a non-starter for many of them.

Since there is a simple fix, and IMHO should have been done this way from the start rather than the hack against privacy, you need to defend why this should not be fixed and ASAP.

It is interesting to look at Jira stats. Top (9 votes) and equal 2nd (8 votes) issues:

   	9  	    	 Bug   	GROOVY-1875  	 private fields and private methods are not private
   	8 	   	Improvement 	GROOVY-1591 	Private Fields Are Accessible From Other Classes
   	8 	   	Bug 	GROOVY-1063 	No access protection for private static fields

Jim.... you quoted my question, yet you don't answer it. Why is this? And I don't see your easy fix. Ok, let us assume we make private fields in Groovy classes package private, then why does this not violate the Java class model? You can still access the fields, nothing has changed. What could be done then is to avoid access to private fields in general, which would solve the access problem for String for example.

But there are two drawbacks here.

For example I always thought that "package private" excludes the access from subclasses, but they can access if they are in the same package. This means if we replace private with package private, then the field becomes visible where it was not visible before. This is incompatible to both Java and Groovy. Is see that much more as a danger for the class model than being able to access private. Because the ability of private as not being inherited remains untouched in current Groovy.

making private package private means also that any Java class in the same package can access the fields with pure Java.By this you can bypass properties, even if it was not your intention. Of course you can do the same in Groovy, even today, but at last for Groovy you have to enforce it if you want to bypass the getter/setter method calls. I see this as a much bigger problem from the Java side, than the Groovy side, because Java is the language that tries to enforce these things.

And there is one more thing to say to a possible fix. "package private"as well as protected are handled like public atm. That is not only by the compiler, but also by the runtime the case. Making the private fields "package private" means a major change to the way the runtime handles inheritance of fields for one, but also you would need to discover such an access and forbid it. While not a problem for private, any check to "package private" is doomed to be not done unless private is implemented "correctly". Such changes are affected the semantics in this area in extreme ways. I am pretty sure I won't do such a change in the 1.x series, because it is simply too much. Doing such a change now means to make a 2.x line and ones it has been fixed it would be a 3.x line.

Also, as for testing purposes, meaning writing unit test for Java or Groovy code, it makes sense to allow access to private.

For me Closures accessing private fields and testing are the main reasons why this issue was not yet solved. A complete fix for this issue needs several incompatible changes to the MOP and that means it won't be done before 2.0. And that is the reason why I set the fix target to 2.0 here. A complete fix does not only include field access, but also method acceess. I know many partial solutions to the problem, but a partial solution will not do.

I think a false but consistent solution is much better than inconsistent changes and also not completely correct ones (there would be no need for further changes if they were completely correct), that do change the way Groovy behaves with probably each version. If you think your solution does do the job, then I suggest you do a fix and we discuss the implications of it. Sometimes a case seems rather different if there is prominent evidence.

The Java-Groovy Privacy Manifesto

1) Groovy must respect 'private' scope. To do otherwise is in total conflict with it's groovy nature of being the best scripting language for Java.

2) The issue that some thought was preventing #1, and is reflected in the current implementation, is the compilation of closures.

3) The easy and correct fix (and the solution used by Java for inner classes) for #2 is to make private members of Groovy classes that need to be accessed by it's closures accessible using package scope.

4) The current Groovy compiler implementation probably makes it difficult to know exactly which private members are accessed by the relevant closures.

5) The conclusion of #4 and #5 is that the correct, easy, and reliable fix is to compile all private members of Groovy classes as package scoped in the bytecode.

6) The change in member visibility from a Groovy perspective for Groovy classes is unchanged.

7) The change in member visibility from a Java perspective for Groovy classes is unchanged, except for Java classes in the same package as Groovy classes. The only difference being that such Java code has access to Groovy 'private' members (which are already only private from Java code, never from Groovy code).

8) #7 is a huge improvement in the class integrity situation.

9) The Java-code-in-same-package-access-to-Groovy-private-members exception of #7 is only even true for Java code that is linked to precompiled Groovy classes. For those that use the unified Java-Groovy compiler, private members need not be visible.

10) Respecting private scope of all Java bytecode and compiling Groovy 'private' members as package scope in the bytecode is giant step forward in making Groovy consistent with the Java platform and should be done in 1.6x.

Then let me say a few things to some of the points...

Historically there where other reasons as well, but they got resolved over time. On of these changes is that this.foo will be compiled as direct access to the field foo if the field foo is defined in the current class. The problem with closures is that this is not done for them, because it breaks a few things here and there.

Then for the implementation of private field access from inner classes in Java. Given the following class in Java:

public class X {
   private int foo;
   class Y {
     Y(){foo=1;}
  }
}

javac will produce bytecode looking like this:

 // class version 50.0 (50)
// access flags 33
public class X {

  // compiled from: X.java
  // access flags 0
  INNERCLASS X$Y X Y

  // access flags 2
  private I foo

  // access flags 1
  public <init>()V
   L0
    LINENUMBER 1 L0
    ALOAD 0
    INVOKESPECIAL java/lang/Object.<init> ()V
   L1
    LINENUMBER 3 L1
    RETURN
    MAXSTACK = 1
    MAXLOCALS = 1   void testFinalProperty() {

  // access flags 4104
  static synthetic access$002(LX;I)I
   L0
    LINENUMBER 1 L0
    ALOAD 0
    ILOAD 1
    DUP_X1
    PUTFIELD X.foo : I
    IRETURN
    MAXSTACK = 3
    MAXLOCALS = 2
}

As you can see Java does not make the field "package private", instead it creates a static synthetic accessor method that is "package private". This is a major difference, because such a method might be callable from other classes in the same package, but at last the method is not inherited, because it is static. A "package private" field would be inherited, leading to complications with other fields of the same name.

Surely we could create such accessor methods and use them instead of accessing the field directly, but this misses another major point, we can already access the field, accessing the field is not the problem at all (well it would be faster this way, but that is another story). The major point here is in knowing when or when not it is allowed to access the field. Either you do this by the compiler, or through the runtime. for a runtime check we lack information in the MOP and for a compile time check we would need a breaking change to closures.

Java compiles the access to the private field foo in Y as a method call to access$002. A Closure would have to duplicate that technique, but it can't use a dynamic method call, and currently the field access goes through getProperty, meaning there is no way to emulate this without a breaking change.

So in short form why we don't fix this right away:

1. it is a breaking change for closures (a property access becomes a method call outside the MOP)
2. package private fields are visible in subclasses in the same package, and will possibly produce name conflicts with already existing fields in both Java and Groovy classes
3. package private for fields is an not intended relaxation of the visibility
4. a correct and 1-time breaking change is preferred over a temporary solution that will break things as well
5. testing private members from Groovy is handy

I realize there are many places to apply a solution, some with more ramifications than others. Coming from a user of Groovy's point of view (not knowing the internals), it seems one place of exposure of this information that should be hidden with little or no detrimental ramifications is what is exposed via foo.properties. Even if the methods / data is still actually accessible, it is better than the current state which is complete exposure. So I am proposing a first step, a baby step (but not the last step) towards eventual resolution to this issue. Perhaps there are other baby steps beyond this have ramifications either. I understand that in the short term we don't want to break things.

As far as I understand the code, to really solve this issue, you would have to differentiate between property access from "inside a class" (and a closure is inside the class where it is defined) and access from outside a class. Interestingly the getProperty-method in MetaClassImpl has a parameter "fromInsideClass" that is never used and in the comments for this parameter there is only a "??".

This might even mean that the MOP has to be changed, adding a get/setPropertyFromInsideClass (and a get/setAttributeFromInsideClass?).

If that is correct I understand that you want to delay the implementation to a 2.x release.
But still this is – for me – the biggest issue with Groovy. As long as this ain't fixed, I will oppose any usage of Groovy in business critical applications in my organization.

true, there is a "fromInside" on MetaClass. I added that a long while ago. But later I did see that this does not solve the issue at all. In a class if you have "this.foo", then it is compiled as direct field access if the field exists. If you do only "foo", then due to the implicit "this", it is compiled in the same way as "this.foo". That means that in current Groovy (1.0, 1.5.x, 1.6) the MetaClass method won't be used for this and a direct field access, like Java would do that is done instead. We can do this for classes, because classes have a clear context when it comes to the meaning of the implicit "this". Much later than adding this parameter we decided on two things, first the direct access to the field in classes and second the mutable name resolving for closures. That all opens a bug question for when then implicit this means "this" as in a class or the delegate.. not to forget that there is the problem that a closure should ask the owner for the field, not the class directly in the nested case. And now it comes down to the MOP. If we don't do a direct field access, then we need to go through the MOP. And that unfortunately means to do a property based access, because the current MOP does not have a special case for fields only. We also have the convention, that "this.foo" becomes a property access if there is no field foo. As a conclusion every field/property access becomes a MOP based property access inside a closure, because we don't know the real meaning of the "implicit this" until runtime.

Now the next element... doing a property based MOP access means that you have to go through getProperty first. And if you look at that method, then you will see, that getProperty doesnot provide this "fromInside" flag. And since that information is mssing, it can't be given to MetaClass and there used to restrict access.

Solutions? one might be to extend getProperty -> breaking change to the MOP. Another to resolve fields in Closures directly while assuming the "implicit this" means the class the closure is contained in -> breaking change to how names are resolved in Closures and the MOP probably as well.

Since there goes no solution without a breaking change I do not want to do this before 2.0

Fix Version/s: None?

it is now a subtask of GROOVY-3010, which is scheduled for 2.0. GROOVY-3010 is a task and has several sub tasks, to collect the issues which are more or less the same.

It seems that groovy++ has resolved the issue. Maybe Alex Tkachman can give a hand to port the code.