ByteBuddyMockFactory adds @internal annotation to Groovy MOP methods

The Groovy 3 & 4 runtime expects to have the @internal annotation on the MOP methods from GroovyObject.
That AbstractCallSite.createGroovyObjectGetPropertySite() processes it as GroovyObject.
So the ByteBuddyMockFactory now adds the @internal annotation to the intercepted MOP methods.

After that the "Unable to access protected constant when spying instances" problems are gone,
because we take the normal Groovy route.

Also some strange inconsistencies for Groovy  2 <=> 3,4 are now gone,
but a new inconsistency appeared Groovy 4 prefers is over get for boolean.
But this is not a Spock issue, because in Groovy only code the same thing happens.
See tests in JavaMocksForGroovyClasses.

This fixes #1501

spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java

@@ -1,7 +1,11 @@
 package org.spockframework.mock.runtime;
 
+import groovy.lang.GroovyObject;
+import groovy.transform.Internal;
 import net.bytebuddy.ByteBuddy;
 import net.bytebuddy.TypeCache;
+import net.bytebuddy.description.annotation.AnnotationDescription;
+import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.modifier.SynchronizationState;
 import net.bytebuddy.description.modifier.SyntheticState;
 import net.bytebuddy.description.modifier.Visibility;
@@ -16,12 +20,12 @@
 import org.spockframework.mock.ISpockMockObject;
 import org.spockframework.mock.codegen.Target;
 import org.spockframework.util.Nullable;
+import org.spockframework.util.ReflectionUtil;
 
 import java.lang.reflect.Method;
 import java.util.List;
 import java.util.concurrent.ThreadLocalRandom;
 
-import static net.bytebuddy.matcher.ElementMatchers.any;
 import static net.bytebuddy.matcher.ElementMatchers.none;
 
 class ByteBuddyMockFactory {
@@ -30,6 +34,10 @@ class ByteBuddyMockFactory {
     new TypeCache.WithInlineExpunction<>(TypeCache.Sort.SOFT);
   private static final Class<?> CODEGEN_TARGET_CLASS = Target.class;
   private static final String CODEGEN_PACKAGE = CODEGEN_TARGET_CLASS.getPackage().getName();
+  @Nullable
+  private static final AnnotationDescription INTERNAL_ANNOTATION = ReflectionUtil.isClassAvailable("groovy.transform.Internal") ?
+    AnnotationDescription.Builder.ofType(Internal.class).build() : null;
+
 
   static Object createMock(final Class<?> type,
                            final List<Class<?>> additionalInterfaces,
@@ -57,11 +65,13 @@ static Object createMock(final Class<?> type,
           .name(name)
           .implement(additionalInterfaces)
           .implement(ISpockMockObject.class)
-          .method(any())
-          .intercept(MethodDelegation.withDefaultConfiguration()
-            .withBinders(Morph.Binder.install(ByteBuddyInvoker.class))
-            .to(ByteBuddyInterceptorAdapter.class))
-          .transform(Transformer.ForMethod.withModifiers(SynchronizationState.PLAIN, Visibility.PUBLIC)) // Overridden methods should be public and non-synchronized.
+          .method(m -> isGroovyMOPMethod(type, m))
+          .intercept(mockInterceptor())
+          .transform(mockTransformer())
+          .annotateMethod(INTERNAL_ANNOTATION) //Annotate the Groovy MOP methods with @Internal
+          .method(m -> !isGroovyMOPMethod(type, m))
+          .intercept(mockInterceptor())
+          .transform(mockTransformer())
           .implement(ByteBuddyInterceptorAdapter.InterceptorAccess.class)
           .intercept(FieldAccessor.ofField("$spock_interceptor"))
           .defineField("$spock_interceptor", IProxyBasedMockInterceptor.class, Visibility.PRIVATE, SyntheticState.SYNTHETIC)
@@ -75,6 +85,39 @@ static Object createMock(final Class<?> type,
     return proxy;
   }
 
+  private static Transformer<MethodDescription> mockTransformer() {
+    return Transformer.ForMethod.withModifiers(SynchronizationState.PLAIN, Visibility.PUBLIC); //Overridden methods should be public and non-synchronized.
+  }
+
+  private static MethodDelegation mockInterceptor() {
+    return MethodDelegation.withDefaultConfiguration()
+      .withBinders(Morph.Binder.install(ByteBuddyInvoker.class))
+      .to(ByteBuddyInterceptorAdapter.class);
+  }
+
+  /**
+   * Checks if the passed method, is a Groovy MOP method from {@link GroovyObject}.
+   *
+   * <p>GroovyObject defined MOP methods getProperty(), setProperty() and invokeMethod(),
+   * because these methods are handled in a special way in the {@link org.codehaus.groovy.runtime.callsite.AbstractCallSite} when marked with {@link Internal}.
+   * See also {@link GroovyObject} comments.
+   * So we need to mark the method with {@link Internal} annotation, if we intercept it.
+   * </p>
+   *
+   * @param type   the type the mock
+   * @param method the method to intercept
+   * @return <code>true</code>, if the method is a Groovy MOP method
+   */
+  private static boolean isGroovyMOPMethod(Class<?> type, MethodDescription method) {
+    if (INTERNAL_ANNOTATION != null &&
+      GroovyObject.class.isAssignableFrom(type) &&
+      method.getDeclaredAnnotations().isAnnotationPresent(Internal.class) &&
+      method.isDefaultMethod()) {
+      return true;
+    }
+    return false;
+  }
+
   // This methods and the ones it calls are inspired by similar code in Mockito's SubclassBytecodeGenerator
   private static boolean shouldLoadIntoCodegenPackage(Class<?> type) {
     return isComingFromJDK(type) || isComingFromSignedJar(type) || isComingFromSealedPackage(type);

spock-core/src/main/java/org/spockframework/runtime/GroovyRuntimeUtil.java

@@ -311,4 +311,15 @@ public static boolean isGroovy3orNewer() {
     //Having isGroovy3() with just "startsWith("3.") there could be (in the future) no tests executed at all for Groovy 4
     return !isGroovy2();
   }
+
+  @Internal
+  public static boolean isGroovy3orOlder() {
+    return isGroovy2() || GroovySystem.getVersion().startsWith("3.");
+  }
+
+  @Internal
+  public static boolean isGroovy4orNewer() {
+    //Having isGroovy4() with just "startsWith("4.") there could be (in the future) no tests executed at all for Groovy 5
+    return !isGroovy3orOlder();
+  }
 }

spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedJavaBaseClass.java

@@ -0,0 +1,11 @@
+package org.spockframework.mock;
+
+class AccessProtectedJavaBaseClass {
+  protected boolean accessible = true;
+}
+
+class AccessProtectedJavaSubClass extends AccessProtectedJavaBaseClass {
+  boolean doesThisWork() {
+    return accessible;
+  }
+}

spock-specs/src/test/groovy/org/spockframework/mock/AccessProtectedPropsSpec.groovy

@@ -0,0 +1,49 @@
+package org.spockframework.mock
+
+import org.spockframework.runtime.model.parallel.ExecutionMode
+import spock.lang.Execution
+import spock.lang.Specification
+
+@Execution(ExecutionMode.SAME_THREAD)
+class AccessProtectedPropsSpec extends Specification {
+
+  /**
+   * The problem with https://github.com/spockframework/spock/issues/1501 is that the ByteBuddyMockFactory overrides
+   * the <code>getProperty(String)</code> method from GroovyObject without having the <code>groovy.transform.Internal</code>
+   * annotation. Therefore <code>org.codehaus.groovy.runtime.callsite.AbstractCallSite.createGroovyObjectGetPropertySite(java.lang.Object)</code>
+   * takes another code path.
+   */
+  def "Access protected const should be accessible in Groovy 3&4 Issue #1501"() {
+    when:
+    AccessProtectedSubClass mySpy = Spy(AccessProtectedSubClass)
+    then:
+    mySpy.doesThisWork()
+  }
+
+  /**
+   * Strangely the test above, would not fail if this test has run before it.
+   */
+  def "Access protected const without spy"() {
+    when:
+    AccessProtectedSubClass mySpy = new AccessProtectedSubClass()
+    then:
+    mySpy.doesThisWork()
+  }
+
+  def "Access protected const should be accessible in Groovy 3&4 with Java class Issue #1501"() {
+    when:
+    AccessProtectedJavaSubClass mySpy = Spy(AccessProtectedJavaSubClass)
+    then:
+    mySpy.doesThisWork()
+  }
+}
+
+class AccessProtectedBaseClass {
+  protected boolean accessible = true
+}
+
+class AccessProtectedSubClass extends AccessProtectedBaseClass {
+  boolean doesThisWork() {
+    return accessible
+  }
+}

spock-specs/src/test/groovy/org/spockframework/runtime/GroovyRuntimeUtilSpec.groovy

@@ -86,5 +86,21 @@ class GroovyRuntimeUtilSpec extends Specification {
     expect:
     GroovyRuntimeUtil.instantiateClosure(cl.getClass(), owner, thisObject).call(3) == 6
   }
+
+  def "getAttribute - error case"() {
+    when:
+    GroovyRuntimeUtil.getAttribute(null, null)
+    then:
+    thrown(MissingFieldException)
+  }
+
+  def "getAttribute"() {
+    given:
+    def list = ["A"]
+    when:
+    Object[] res = GroovyRuntimeUtil.getAttribute(list, "elementData")
+    then:
+    res[0] == "A"
+  }
 }
 

spock-specs/src/test/groovy/org/spockframework/smoke/mock/JavaMocksForGroovyClasses.groovy

@@ -155,19 +155,7 @@ class JavaMocksForGroovyClasses extends Specification {
   }
 
   // TODO: swallowed when mocking static inner class because the latter implements methodMissing/propertyMissing
-  @Requires({ GroovyRuntimeUtil.isGroovy2() }) //different exception in Groovy 2 and 3
   @FailsWith(MissingPropertyException)
-  def "dynamic properties are considered to not exist (Groovy 2)"() {
-    when:
-    mockMe.someProperty
-
-    then:
-    1 * mockMe.someProperty
-  }
-
-  // TODO: swallowed when mocking static inner class because the latter implements methodMissing/propertyMissing
-  @Requires({ GroovyRuntimeUtil.isGroovy3orNewer() })
-  @FailsWith(MissingMethodException)
   def "dynamic properties are considered to not exist"() {
     when:
     mockMe.someProperty
@@ -271,21 +259,59 @@ class JavaMocksForGroovyClasses extends Specification {
   }
 
   @Issue("https://github.com/spockframework/spock/issues/1256")
-  def "Mock object boolean (get + is) accessor via dot-notation" () {
+  @Requires({ GroovyRuntimeUtil.isGroovy3orOlder() })
+  def "Mock object boolean (get + is) accessor via dot-notation (Groovy 2&3)"() {
     given:
     ExampleData mockData = Mock(ExampleData)
 
     when: "query via property syntax"
     def result = mockData.active ? "Data is current" : "Data is not current"
 
-    then: "calls mock, preferring 'get' to 'is' for boolean getters (surprise!)"
+    then: "calls mock, preferring 'get' to 'is' for boolean getters (surprise!) in Groovy <=3"
     1 * mockData.getActive() >> true
     0 * mockData.isActive()
 
     and:
     result == "Data is current"
   }
 
+  /**
+   * The resolution of properties changed in Groovy 4 for the ExampleData.active Groovy 4 resolves the is
+   * whereas Groovy 2 & 3 resolved the get.
+   */
+  @Issue("https://github.com/spockframework/spock/issues/1256")
+  @Requires({ GroovyRuntimeUtil.isGroovy4orNewer() })
+  def "Mock object boolean (get + is) accessor via dot-notation (Groovy 4)"() {
+    given:
+    ExampleData mockData = Mock(ExampleData)
+
+    when: "query via property syntax"
+    def result = mockData.active ? "Data is current" : "Data is not current"
+
+    then: "calls mock, preferring 'is' for boolean getters in Groovy >=4"
+    1 * mockData.isActive() >> true
+    0 * mockData.getActive()
+
+    and:
+    result == "Data is current"
+  }
+
+  @Requires({ GroovyRuntimeUtil.isGroovy4orNewer() })
+  def "Real object boolean (get + is) accessor via dot-notation (Groovy 4)"() {
+    given:
+    def data = new ExampleData()
+    expect:
+    !data.active //The isActive() method returns false
+  }
+
+  @Requires({ GroovyRuntimeUtil.isGroovy3orOlder() })
+  def "Real object boolean (get + is) accessor via dot-notation (Groovy 2&3)"() {
+    given:
+    def data = new ExampleData()
+    expect:
+    data.active //The getActive() method returns true
+  }
+
   @Issue("https://github.com/spockframework/spock/issues/1256")
   def "Mock object non-boolean (get + is) accessor via dot-notation" () {
     given:
@@ -371,7 +397,7 @@ class ExampleData {
   }
 
   boolean getActive() {
-    false
+    true
   }
 
   String isName() {