Copilot commented on code in PR #2467:
URL: https://github.com/apache/groovy/pull/2467#discussion_r3072638311
##########
src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java:
##########
@@ -365,7 +365,7 @@ private static void addGetterIfNeeded(final
DelegateDescription delegate, final
}
if (willHaveIsAccessor && !ownerWillHaveIsAccessor.get()
- && !shouldSkipPropertyMethod(name, getterName,
delegate.excludes, delegate.includes, allNames)) {
+ && !shouldSkipPropertyMethod(prop, getterName,
delegate.excludes, delegate.includes, allNames)) {
Review Comment:
The `willHaveIsAccessor` branch generates `isserName`, but the skip-check
passes `getterName` into `shouldSkipPropertyMethod`. This means
excludes/includes for the `isXxx` method name won't be honored. Pass
`isserName` instead so filtering applies to the actual generated method.
```suggestion
&& !shouldSkipPropertyMethod(prop, isserName,
delegate.excludes, delegate.includes, allNames)) {
```
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -2291,6 +2294,22 @@ private static boolean canAccessLegally(final MetaMethod
method) {
|| ((CachedMethod) method).canAccessLegally(MetaClassImpl.class);
}
+ private static boolean isMarkedInternal(final MetaBeanProperty mbp) {
+ CachedField field = mbp.getField();
+ if (field != null &&
field.getCachedField().isAnnotationPresent(Internal.class)) {
+ return true;
+ }
+ MetaMethod getter = mbp.getGetter();
+ if (getter instanceof CachedMethod cm &&
cm.getCachedMethod().isAnnotationPresent(Internal.class)) {
+ return true;
+ }
+ MetaMethod setter = mbp.getSetter();
+ if (setter instanceof CachedMethod cm &&
cm.getCachedMethod().isAnnotationPresent(Internal.class)) {
Review Comment:
`isMarkedInternal` uses `field.getCachedField()` / `cm.getCachedMethod()`
which both attempt to make members accessible. For methods you can avoid that
by using `CachedMethod#getAnnotation(Internal.class)` (doesn't call
`makeAccessible`); for fields consider a similar non-accessing annotation
lookup to keep `getProperties()` cheap and side-effect free.
```suggestion
if (field != null && field.getAnnotation(Internal.class) != null) {
return true;
}
MetaMethod getter = mbp.getGetter();
if (getter instanceof CachedMethod cm &&
cm.getAnnotation(Internal.class) != null) {
return true;
}
MetaMethod setter = mbp.getSetter();
if (setter instanceof CachedMethod cm &&
cm.getAnnotation(Internal.class) != null) {
```
##########
src/main/java/org/codehaus/groovy/transform/AbstractASTTransformation.java:
##########
@@ -276,6 +304,25 @@ public static boolean shouldSkip(String name, List<String>
excludes, List<String
(includes != null && !includes.isEmpty() &&
!includes.contains(name));
}
+ /**
+ * Variant that checks both the name and {@link groovy.transform.Internal
@Internal} annotation.
+ *
+ * @since 6.0.0
+ */
+ public static boolean shouldSkip(AnnotatedNode node, List<String>
excludes, List<String> includes, boolean allNames) {
+ String name = nodeName(node);
+ return (excludes != null && excludes.contains(name)) ||
+ (!allNames && deemedInternal(node)) ||
+ (includes != null && !includes.isEmpty() &&
!includes.contains(name));
+ }
+
+ private static String nodeName(AnnotatedNode node) {
+ if (node instanceof FieldNode fn) return fn.getName();
+ if (node instanceof PropertyNode pn) return pn.getName();
+ if (node instanceof MethodNode mn) return mn.getName();
+ return "";
Review Comment:
`nodeName` returns an empty string for `AnnotatedNode` types other than
`FieldNode`/`PropertyNode`/`MethodNode`, which can silently change skip
behavior (e.g., `includes`/`excludes` checks on ""). Since
`shouldSkip*(AnnotatedNode, ...)` are public APIs, consider throwing an
`IllegalArgumentException` for unsupported node types (or explicitly handling
the additional node types you intend to support).
```suggestion
throw new IllegalArgumentException("Unsupported AnnotatedNode type:
" + (node == null ? "null" : node.getClass().getName()));
```
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -2253,13 +2254,15 @@ public List<MetaProperty> getProperties() {
// simply return the values of the metaproperty map as a List
List<MetaProperty> ret = new ArrayList<>(propertyMap.size());
for (MetaProperty mp : propertyMap.values()) {
- if (mp instanceof CachedField) {
- if (mp.isSynthetic()
+ if (mp instanceof CachedField cf) {
+ if (cf.isSynthetic()
+ ||
cf.getCachedField().isAnnotationPresent(Internal.class)
// GROOVY-5169, GROOVY-9081, GROOVY-9103,
GROOVY-10438, GROOVY-10555, et al.
- || (!permissivePropertyAccess &&
!checkAccessible(getClass(), ((CachedField) mp).getDeclaringClass(),
mp.getModifiers(), false))) {
+ || (!permissivePropertyAccess &&
!checkAccessible(getClass(), cf.getDeclaringClass(), cf.getModifiers(),
false))) {
Review Comment:
`cf.getCachedField().isAnnotationPresent(Internal.class)` forces
`CachedField#getCachedField()` which calls
`ReflectionUtils.makeAccessibleInPrivilegedAction(field)` (side-effect +
overhead) even though we only need to read the annotation. Consider
adding/using an annotation accessor on `CachedField` that does *not* call
`makeAccessible`, and use that here to avoid unnecessary accessibility changes
during property enumeration.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
