You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2022/01/07 20:45:21 UTC

[GitHub] [groovy] eric-milles opened a new pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

eric-milles opened a new pull request #1674:
URL: https://github.com/apache/groovy/pull/1674


   There is still some asymmetry between read and write for protected fields.  But I wasn't sure if I should add a protected check to read or remove the protected check from write (see `MetaClassImpl#setProperty` circa GROOVY-5985 comment).
   
   https://issues.apache.org/jira/browse/GROOVY-5001
   https://issues.apache.org/jira/browse/GROOVY-5491
   
   see also:
   https://issues.apache.org/jira/browse/GROOVY-662
   https://issues.apache.org/jira/browse/GROOVY-8065
   https://issues.apache.org/jira/browse/GROOVY-8074


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles commented on pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#issuecomment-1013925732


   Added test cases for "metaClass" and "MetaClass" in dynamic groovy.  "class", "Class", "empty", "Empty", "metaClass" and "MetaClass" probably need tests under static compilation.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] danielsun1106 commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r780601820



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -2085,73 +2089,65 @@ public void setProperty(Object object, Object newValue) {
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return new MetaProperty(name, Object.class) {
-                @Override
-                public Object getProperty(Object object) {
-                    return ((Map) object).get(name);
-                }
-
-                @Override
-                public void setProperty(Object object, Object newValue) {
-                    throw new UnsupportedOperationException();
-                }
-            };
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, theClass, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || "class".equals(name) || (isMap && "empty".equals(name))) {

Review comment:
       It's better to extract the common checking logic, here is same code at line 1981:
   `if (method == null || "class".equals(name) || (isMap && "empty".equals(name)))`




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles edited a comment on pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles edited a comment on pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#issuecomment-1013925732


   Added test cases for "metaClass" and "MetaClass" in dynamic groovy.  "metaClass", "MetaClass", "Class" and "Empty" probably need tests under static compilation.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles edited a comment on pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles edited a comment on pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#issuecomment-1013940781


   GROOVY-6151 is a good illustration of user expectations (comments added):
   ```groovy
   class TestError extends HashMap {
       String id
       boolean equals(Object o) {
           if (this.is(o)) return true
           if (getClass() != o.class) return false // "o.class" may not be "getClass()" for some types
           if (!super.equals(o)) return false
           TestError testError = (TestError) o
           if (id != testError.id) return false // "id" and "this.id" refer to the property; "testError.id" does not refer to "getId()"
           return true
       }
   }
   ```
   
   With this PR, all references to "id" will refer to the declared property and not a Map property.  "class" vs "getClass()" and "empty" vs "isEmpty()" remain unchanged.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles merged pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles merged pull request #1674:
URL: https://github.com/apache/groovy/pull/1674


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r780607867



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -2085,73 +2089,65 @@ public void setProperty(Object object, Object newValue) {
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return new MetaProperty(name, Object.class) {
-                @Override
-                public Object getProperty(Object object) {
-                    return ((Map) object).get(name);
-                }
-
-                @Override
-                public void setProperty(Object object, Object newValue) {
-                    throw new UnsupportedOperationException();
-                }
-            };
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, theClass, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || "class".equals(name) || (isMap && "empty".equals(name))) {

Review comment:
       Why is it better to extract?  `getProperty` and `getEffectiveMetaProperty` are essentially the same general structure, only differing in a few details.  I don't really have the energy to extract the true repeated structure(s).  You can follow the two methods and see the parallel, so I think it is decent in this state.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] sonatype-lift[bot] commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r785487229



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -1959,100 +1959,105 @@ public void setProperties(Object bean, Map map) {
      * @return the given property's value on the object
      */
     @Override
-    public Object getProperty(Class sender, Object object, String name, boolean useSuper, boolean fromInsideClass) {
+    public Object getProperty(final Class sender, final Object object, final String name, final boolean useSuper, final boolean fromInsideClass) {
 
         //----------------------------------------------------------------------
         // handling of static
         //----------------------------------------------------------------------
-        boolean isStatic = theClass != Class.class && object instanceof Class;
+        boolean isStatic = (theClass != Class.class && object instanceof Class);
         if (isStatic && object != theClass) {
-            MetaClass mc = registry.getMetaClass((Class) object);
+            MetaClass mc = registry.getMetaClass((Class<?>) object);
             return mc.getProperty(sender, object, name, useSuper, false);
         }
 
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return ((Map) object).get(name);
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, sender, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || isSpecialProperty(name)) {
+            //------------------------------------------------------------------
+            // public field
+            //------------------------------------------------------------------
+            MetaProperty mp = methodAndProperty.getV2();
+            if (mp != null && Modifier.isPublic(mp.getModifiers())) {
+                try {
+                    return mp.getProperty(object);
+                } catch (IllegalArgumentException | CacheAccessControlException e) {
+                    // can't access the field directly but there may be a getter
+                    mp = null;
+                }
+            }
 
-        //----------------------------------------------------------------------
-        // field
-        //----------------------------------------------------------------------
-        if (method == null && mp != null) {
-            try {
-                return mp.getProperty(object);
-            } catch (IllegalArgumentException | CacheAccessControlException e) {
-                // can't access the field directly but there may be a getter
-                mp = null;
+            //------------------------------------------------------------------
+            // java.util.Map get method
+            //------------------------------------------------------------------
+            if (isMap && !isStatic) {
+                return ((Map<?,?>) object).get(name);
             }
-        }
 
-        // check for propertyMissing provided through a category
-        Object[] arguments = EMPTY_ARGUMENTS;
-        if (method == null && !useSuper && !isStatic && GroovyCategorySupport.hasCategoryInCurrentThread()) {
-            method = getCategoryMethodGetter(sender, PROPERTY_MISSING, true);
-            if (method != null) arguments = new Object[]{name};
+            //------------------------------------------------------------------
+            // non-public field
+            //------------------------------------------------------------------
+            if (mp != null) {
+                try {
+                    return mp.getProperty(object);
+                } catch (IllegalArgumentException | CacheAccessControlException e) {

Review comment:
       *EmptyCatch:*  Caught exceptions should not be ignored [(details)](https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions)
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r780607867



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -2085,73 +2089,65 @@ public void setProperty(Object object, Object newValue) {
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return new MetaProperty(name, Object.class) {
-                @Override
-                public Object getProperty(Object object) {
-                    return ((Map) object).get(name);
-                }
-
-                @Override
-                public void setProperty(Object object, Object newValue) {
-                    throw new UnsupportedOperationException();
-                }
-            };
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, theClass, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || "class".equals(name) || (isMap && "empty".equals(name))) {

Review comment:
       Why is it better to extract?  `getProperty` and `getEffectiveMetaProperty` are essentially the same general structure, only differing in a few details.  I don't really have the energy to extract the true repeated structure(s).  You can follow the two methods and see the parallel, so I think it is decent in this state.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] danielsun1106 commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r780601820



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -2085,73 +2089,65 @@ public void setProperty(Object object, Object newValue) {
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return new MetaProperty(name, Object.class) {
-                @Override
-                public Object getProperty(Object object) {
-                    return ((Map) object).get(name);
-                }
-
-                @Override
-                public void setProperty(Object object, Object newValue) {
-                    throw new UnsupportedOperationException();
-                }
-            };
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, theClass, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || "class".equals(name) || (isMap && "empty".equals(name))) {

Review comment:
       It's better to extract the common checking logic, here is same code at line 1981:
   `if (method == null || "class".equals(name) || (isMap && "empty".equals(name)))`




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles commented on pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#issuecomment-1013940781


   GROOVY-6151 is a good illustration of user expectations (comments added):
   {code:groovy}
   class TestError extends HashMap {
       String id
       boolean equals(Object o) {
           if (this.is(o)) return true
           if (getClass() != o.class) return false // "o.class" may not be "getClass()" for some types
           if (!super.equals(o)) return false
           TestError testError = (TestError) o
           if (id != testError.id) return false // "id" and "this.id" refer to the property; "testError.id" does not refer to "getId()"
           return true
       }
   }
   {code}
   
   With this PR, all references to "id" will refer to the declared property and not a Map property.  "class" vs "getClass()" and "empty" vs "isEmpty()" remain unchanged.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] paulk-asert commented on pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#issuecomment-1009908404


   I agree it would be good for the get and set behaviors to be aligned. The suggested external behavior seems okay and I would add 'metaClass' as an additional case to test for.
   
   What worries me is the change in behavior for closures. The following currently passes:
   ```
   class Foo {
       @Delegate Map m = [:]
       def x = 4
       protected y = 5
       private z = 6
       void method() {
           def w = 3
           assert 3 == { -> w }()
           assert null == { -> x }()
           assert null == { -> y }()
           assert null == { -> z }()
       }
   }
   
   class Bar {
       def x = 4
       protected y = 5
       private z = 6
       void method() {
           def w = 3
           assert 3 == { -> w }()
           assert 4 == { -> x }()
           assert 5 == { -> y }()
           assert 6 == { -> z }()
       }
   }
   
   new Foo().method()
   new Bar().method()
   ```
   This exhibits the "potentially strange" map takes precedence behavior but is at least consistent across `x`, `y`, and `z`.
   After the change, just the `x` behavior has altered precedence.
   
   My suspicion is that what you have is close but needs some more work. I am also thinking we might need to define a special Map metaclass that would provide back the old behavior - again it would be good to align get/set even for that case. Users relying on the old behavior could manually apply that metaclass. I would regard this as a legacy compatibility mechanism rather than something we would recommend is used for new development.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles commented on pull request #1674: GROOVY-5001, GROOVY-5491: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#issuecomment-1010106711


   It was intended to make this read the property and not from the map:
   ```groovy
   class C {
       @Delegate Map m = [:]
       def x = 4
       public y = 5
       void method() {
           def v = 3
           assert 3 == { -> v }()
           assert 4 == { -> x }() // previously preferred map lookup
           assert 5 == { -> y }() // previously preferred map lookup
       }
   }
   ```
   
   I thought the first step would be to have read of `public` field or accessor gain precedence over map lookup.  For write it already includes `public` and `protected`.  I took the cautions approach by dealing with `public` first and leaving `protected` as-is.
   
   The idea here is that if you created a type that implements `Map` but then added public API, that API should take precedence over the general map property mechanism.
   
   For legacy support, any code may replace `name` with `get('name')` to stick with the map.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] sonatype-lift[bot] commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r785488576



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -1959,100 +1959,105 @@ public void setProperties(Object bean, Map map) {
      * @return the given property's value on the object
      */
     @Override
-    public Object getProperty(Class sender, Object object, String name, boolean useSuper, boolean fromInsideClass) {
+    public Object getProperty(final Class sender, final Object object, final String name, final boolean useSuper, final boolean fromInsideClass) {
 
         //----------------------------------------------------------------------
         // handling of static
         //----------------------------------------------------------------------
-        boolean isStatic = theClass != Class.class && object instanceof Class;
+        boolean isStatic = (theClass != Class.class && object instanceof Class);
         if (isStatic && object != theClass) {
-            MetaClass mc = registry.getMetaClass((Class) object);
+            MetaClass mc = registry.getMetaClass((Class<?>) object);
             return mc.getProperty(sender, object, name, useSuper, false);
         }
 
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return ((Map) object).get(name);
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, sender, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || isSpecialProperty(name)) {
+            //------------------------------------------------------------------
+            // public field
+            //------------------------------------------------------------------
+            MetaProperty mp = methodAndProperty.getV2();
+            if (mp != null && Modifier.isPublic(mp.getModifiers())) {
+                try {
+                    return mp.getProperty(object);
+                } catch (IllegalArgumentException | CacheAccessControlException e) {
+                    // can't access the field directly but there may be a getter
+                    mp = null;
+                }
+            }
 
-        //----------------------------------------------------------------------
-        // field
-        //----------------------------------------------------------------------
-        if (method == null && mp != null) {
-            try {
-                return mp.getProperty(object);
-            } catch (IllegalArgumentException | CacheAccessControlException e) {
-                // can't access the field directly but there may be a getter
-                mp = null;
+            //------------------------------------------------------------------
+            // java.util.Map get method
+            //------------------------------------------------------------------
+            if (isMap && !isStatic) {
+                return ((Map<?,?>) object).get(name);
             }
-        }
 
-        // check for propertyMissing provided through a category
-        Object[] arguments = EMPTY_ARGUMENTS;
-        if (method == null && !useSuper && !isStatic && GroovyCategorySupport.hasCategoryInCurrentThread()) {
-            method = getCategoryMethodGetter(sender, PROPERTY_MISSING, true);
-            if (method != null) arguments = new Object[]{name};
+            //------------------------------------------------------------------
+            // non-public field
+            //------------------------------------------------------------------
+            if (mp != null) {
+                try {
+                    return mp.getProperty(object);
+                } catch (IllegalArgumentException | CacheAccessControlException e) {

Review comment:
       I've recorded this as ignored for this pull request. If you change your mind, just comment `@sonatype-lift unignore`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [groovy] eric-milles commented on a change in pull request #1674: GROOVY-5001, GROOVY-5491, GROOVY-6144: map property precedence

Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1674:
URL: https://github.com/apache/groovy/pull/1674#discussion_r785488573



##########
File path: src/main/java/groovy/lang/MetaClassImpl.java
##########
@@ -1959,100 +1959,105 @@ public void setProperties(Object bean, Map map) {
      * @return the given property's value on the object
      */
     @Override
-    public Object getProperty(Class sender, Object object, String name, boolean useSuper, boolean fromInsideClass) {
+    public Object getProperty(final Class sender, final Object object, final String name, final boolean useSuper, final boolean fromInsideClass) {
 
         //----------------------------------------------------------------------
         // handling of static
         //----------------------------------------------------------------------
-        boolean isStatic = theClass != Class.class && object instanceof Class;
+        boolean isStatic = (theClass != Class.class && object instanceof Class);
         if (isStatic && object != theClass) {
-            MetaClass mc = registry.getMetaClass((Class) object);
+            MetaClass mc = registry.getMetaClass((Class<?>) object);
             return mc.getProperty(sender, object, name, useSuper, false);
         }
 
         checkInitalised();
 
         //----------------------------------------------------------------------
-        // turn getProperty on a Map to get on the Map itself
+        // getter
         //----------------------------------------------------------------------
-        if (!isStatic && this.isMap) {
-            return ((Map) object).get(name);
-        }
-
         Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, sender, name, useSuper, isStatic);
         MetaMethod method = methodAndProperty.getV1();
 
-        //----------------------------------------------------------------------
-        // getter
-        //----------------------------------------------------------------------
-        MetaProperty mp = methodAndProperty.getV2();
+        if (method == null || isSpecialProperty(name)) {
+            //------------------------------------------------------------------
+            // public field
+            //------------------------------------------------------------------
+            MetaProperty mp = methodAndProperty.getV2();
+            if (mp != null && Modifier.isPublic(mp.getModifiers())) {
+                try {
+                    return mp.getProperty(object);
+                } catch (IllegalArgumentException | CacheAccessControlException e) {
+                    // can't access the field directly but there may be a getter
+                    mp = null;
+                }
+            }
 
-        //----------------------------------------------------------------------
-        // field
-        //----------------------------------------------------------------------
-        if (method == null && mp != null) {
-            try {
-                return mp.getProperty(object);
-            } catch (IllegalArgumentException | CacheAccessControlException e) {
-                // can't access the field directly but there may be a getter
-                mp = null;
+            //------------------------------------------------------------------
+            // java.util.Map get method
+            //------------------------------------------------------------------
+            if (isMap && !isStatic) {
+                return ((Map<?,?>) object).get(name);
             }
-        }
 
-        // check for propertyMissing provided through a category
-        Object[] arguments = EMPTY_ARGUMENTS;
-        if (method == null && !useSuper && !isStatic && GroovyCategorySupport.hasCategoryInCurrentThread()) {
-            method = getCategoryMethodGetter(sender, PROPERTY_MISSING, true);
-            if (method != null) arguments = new Object[]{name};
+            //------------------------------------------------------------------
+            // non-public field
+            //------------------------------------------------------------------
+            if (mp != null) {
+                try {
+                    return mp.getProperty(object);
+                } catch (IllegalArgumentException | CacheAccessControlException e) {

Review comment:
       @sonatype-lift ignore




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org