Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce animal sniffer #2006

Merged
merged 2 commits into from Aug 18, 2020
Merged

Introduce animal sniffer #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d42221c
Add signature checks to assure Android and Java 8 compatibility. Excl…
raphw Aug 17, 2020
ec659f4
Do not use invoke/invokeExact directly but from generated code to avo…
raphw Aug 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions build.gradle
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ plugins {
id 'eclipse'
id 'com.github.ben-manes.versions' version '0.28.0'
id 'biz.aQute.bnd.builder' version '5.1.0'
id 'ru.vyarus.animalsniffer' version '1.5.1'
}

description = 'Mockito mock objects library core API and implementation'
Expand Down Expand Up @@ -98,6 +99,14 @@ dependencies {
testRuntime configurations.compileOnly

testUtil sourceSets.test.output

signature 'org.codehaus.mojo.signature:java18:1.0@signature'
signature 'net.sf.androidscents.signature:android-api-level-24:7.0_r2@signature'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for my understanding, this says that we are supporting Android API levels 24 and over? If so, we should document that somewhere as well. In the Twitter thread, they proposed level 21 and over. If we set this to 21, do we need to make a lot of changes? Iirc, Mockito mostly builds in Google so I think we would be able to do so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should, indeed. Android level 21 does not support types such as Optional or Function. We can go that far but it's already part of the public API. I guess 3.5.0 is not super-wide spread yet but I'd like to avoid releasing 4.0 just for that.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, iirc Mockito 3.4.0 builds just fine internally and also includes Optional. I can take a look tomorrow as to why that works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks though, that Android can easily desugar these few types: https://developer.android.com/studio/write/java8-support - it only does not work with method handle types which are not exposed. I just asked for a validation of this branch of the user who reported this. By requiring API level 24, we do not leak API that cannot be desugared at least. But yes, we should document this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, with code shrinking, it should already work with 3.5.0: https://developer.android.com/studio/build/shrink-code#shrink-code

}

animalsniffer {
sourceSets = [sourceSets.main]
annotation = 'org.mockito.internal.SuppressSignatureCheck'
}

wrapper {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/mockito/internal/MockedStaticImpl.java
View file
Open in desktop
Expand Up @@ -172,6 +172,6 @@ private void assertNotClosed() {

@Override
public String toString() {
return "static mock for " + control.getType().getTypeName();
return "static mock for " + control.getType().getName();
}
}
11 changes: 11 additions & 0 deletions src/main/java/org/mockito/internal/SuppressSignatureCheck.java
View file
Open in desktop
@@ -0,0 +1,11 @@
/*
* Copyright (c) 2020 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.internal;

import java.lang.annotation.*;

@Retention(RetentionPolicy.CLASS)
@Documented
public @interface SuppressSignatureCheck {}
28 changes: 15 additions & 13 deletions src/main/java/org/mockito/internal/creation/bytebuddy/InlineByteBuddyMockMaker.java
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@
import org.mockito.exceptions.base.MockitoException;
import org.mockito.exceptions.base.MockitoInitializationException;
import org.mockito.exceptions.misusing.MockitoConfigurationException;
import org.mockito.internal.SuppressSignatureCheck;
import org.mockito.internal.configuration.plugins.Plugins;
import org.mockito.internal.util.Platform;
import org.mockito.internal.util.concurrent.DetachedThreadLocal;
Expand Down Expand Up @@ -99,6 +100,7 @@
* support this feature.
*/
@Incubating
@SuppressSignatureCheck
public class InlineByteBuddyMockMaker
implements ClassCreatingMockMaker, InlineMockMaker, Instantiator {

Expand Down Expand Up @@ -506,7 +508,7 @@ public <T> StaticMockControl<T> createStaticMock(
|| ClassLoader.class.isAssignableFrom(type)) {
throw new MockitoException(
"It is not possible to mock static methods of "
+ type.getTypeName()
+ type.getName()
+ " to avoid interfering with class loading what leads to infinite loops");
}

Expand Down Expand Up @@ -534,7 +536,7 @@ public <T> ConstructionMockControl<T> createConstructionMock(
} else if (type.isPrimitive() || Modifier.isAbstract(type.getModifiers())) {
throw new MockitoException(
"It is not possible to construct primitive types or abstract types: "
+ type.getTypeName());
+ type.getName());
}

bytecodeGenerator.mockClassConstruction(type);
Expand All @@ -555,7 +557,7 @@ public <T> ConstructionMockControl<T> createConstructionMock(
public <T> T newInstance(Class<T> cls) throws InstantiationException {
Constructor<?>[] constructors = cls.getDeclaredConstructors();
if (constructors.length == 0) {
throw new InstantiationException(cls.getTypeName() + " does not define a constructor");
throw new InstantiationException(cls.getName() + " does not define a constructor");
}
Constructor<?> selected = constructors[0];
for (Constructor<?> constructor : constructors) {
Expand All @@ -579,7 +581,7 @@ public <T> T newInstance(Class<T> cls) throws InstantiationException {
mockitoConstruction.set(false);
}
} catch (Exception e) {
throw new InstantiationException("Could not instantiate " + cls.getTypeName(), e);
throw new InstantiationException("Could not instantiate " + cls.getName(), e);
}
}

Expand Down Expand Up @@ -754,14 +756,14 @@ private static class InlineConstructionMockContext implements MockedConstruction
private static final Map<String, Class<?>> PRIMITIVES = new HashMap<>();

static {
PRIMITIVES.put(boolean.class.getTypeName(), boolean.class);
PRIMITIVES.put(byte.class.getTypeName(), byte.class);
PRIMITIVES.put(short.class.getTypeName(), short.class);
PRIMITIVES.put(char.class.getTypeName(), char.class);
PRIMITIVES.put(int.class.getTypeName(), int.class);
PRIMITIVES.put(long.class.getTypeName(), long.class);
PRIMITIVES.put(float.class.getTypeName(), float.class);
PRIMITIVES.put(double.class.getTypeName(), double.class);
PRIMITIVES.put(boolean.class.getName(), boolean.class);
PRIMITIVES.put(byte.class.getName(), byte.class);
PRIMITIVES.put(short.class.getName(), short.class);
PRIMITIVES.put(char.class.getName(), char.class);
PRIMITIVES.put(int.class.getName(), int.class);
PRIMITIVES.put(long.class.getName(), long.class);
PRIMITIVES.put(float.class.getName(), float.class);
PRIMITIVES.put(double.class.getName(), double.class);
}

private int count;
Expand Down Expand Up @@ -810,7 +812,7 @@ public Constructor<?> constructor() {
join(
"Could not resolve constructor of type",
"",
type.getTypeName(),
type.getName(),
"",
"with arguments of types",
Arrays.toString(parameterTypes)),
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/mockito/internal/creation/bytebuddy/InlineBytecodeGenerator.java
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@
import net.bytebuddy.utility.OpenedClassReader;
import net.bytebuddy.utility.RandomString;
import org.mockito.exceptions.base.MockitoException;
import org.mockito.internal.SuppressSignatureCheck;
import org.mockito.internal.creation.bytebuddy.inject.MockMethodDispatcher;
import org.mockito.internal.util.concurrent.DetachedThreadLocal;
import org.mockito.internal.util.concurrent.WeakConcurrentMap;
Expand All @@ -48,6 +49,7 @@
import static net.bytebuddy.matcher.ElementMatchers.*;
import static org.mockito.internal.util.StringUtil.*;

@SuppressSignatureCheck
public class InlineBytecodeGenerator implements BytecodeGenerator, ClassFileTransformer {

private static final String PRELOAD = "org.mockito.inline.preload";
Expand Down
79 changes: 53 additions & 26 deletions src/main/java/org/mockito/internal/util/reflection/InstrumentationMemberAccessor.java
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@
import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
import net.bytebuddy.implementation.MethodCall;
import org.mockito.exceptions.base.MockitoInitializationException;
import org.mockito.internal.SuppressSignatureCheck;
import org.mockito.plugins.MemberAccessor;

import java.lang.instrument.Instrumentation;
Expand All @@ -21,6 +22,7 @@
import static net.bytebuddy.matcher.ElementMatchers.named;
import static org.mockito.internal.util.StringUtil.join;

@SuppressSignatureCheck
class InstrumentationMemberAccessor implements MemberAccessor {

private static final Map<Class<?>, Class<?>> WRAPPERS = new HashMap<>();
Expand Down Expand Up @@ -67,6 +69,13 @@ class InstrumentationMemberAccessor implements MemberAccessor {
"setAccessible", boolean.class))
.onArgument(0)
.withArgument(1))
.method(named("invokeWithArguments"))
.intercept(
MethodCall.invoke(
MethodHandle.class.getMethod(
"invokeWithArguments", Object[].class))
.onArgument(0)
.withArgument(1))
.make()
.load(
InstrumentationMemberAccessor.class.getClassLoader(),
Expand Down Expand Up @@ -144,17 +153,20 @@ public Object newInstance(Constructor<?> constructor, Object... arguments)
}
assureArguments(constructor, null, null, arguments, constructor.getParameterTypes());
try {
Object module = getModule.bindTo(constructor.getDeclaringClass()).invokeWithArguments();
Object module =
DISPATCHER.invokeWithArguments(
getModule.bindTo(constructor.getDeclaringClass()));
String packageName = constructor.getDeclaringClass().getPackage().getName();
assureOpen(module, packageName);
MethodHandle handle =
((MethodHandles.Lookup)
privateLookupIn.invokeExact(
DISPATCHER.invokeWithArguments(
privateLookupIn,
constructor.getDeclaringClass(),
DISPATCHER.getLookup()))
.unreflectConstructor(constructor);
try {
return handle.invokeWithArguments(arguments);
return DISPATCHER.invokeWithArguments(handle, arguments);
} catch (Throwable t) {
throw new InvocationTargetException(t);
}
Expand All @@ -180,19 +192,22 @@ public Object invoke(Method method, Object target, Object... arguments)
arguments,
method.getParameterTypes());
try {
Object module = getModule.bindTo(method.getDeclaringClass()).invokeWithArguments();
Object module =
DISPATCHER.invokeWithArguments(getModule.bindTo(method.getDeclaringClass()));
String packageName = method.getDeclaringClass().getPackage().getName();
assureOpen(module, packageName);
MethodHandle handle =
((MethodHandles.Lookup)
privateLookupIn.invokeExact(
method.getDeclaringClass(), DISPATCHER.getLookup()))
DISPATCHER.invokeWithArguments(
privateLookupIn,
method.getDeclaringClass(),
DISPATCHER.getLookup()))
.unreflect(method);
if (!Modifier.isStatic(method.getModifiers())) {
handle = handle.bindTo(target);
}
try {
return handle.invokeWithArguments(arguments);
return DISPATCHER.invokeWithArguments(handle, arguments);
} catch (Throwable t) {
throw new InvocationTargetException(t);
}
Expand All @@ -219,18 +234,21 @@ public Object get(Field field, Object target) {
new Object[0],
new Class<?>[0]);
try {
Object module = getModule.bindTo(field.getDeclaringClass()).invokeWithArguments();
Object module =
DISPATCHER.invokeWithArguments(getModule.bindTo(field.getDeclaringClass()));
String packageName = field.getDeclaringClass().getPackage().getName();
assureOpen(module, packageName);
MethodHandle handle =
((MethodHandles.Lookup)
privateLookupIn.invokeExact(
field.getDeclaringClass(), DISPATCHER.getLookup()))
DISPATCHER.invokeWithArguments(
privateLookupIn,
field.getDeclaringClass(),
DISPATCHER.getLookup()))
.unreflectGetter(field);
if (!Modifier.isStatic(field.getModifiers())) {
handle = handle.bindTo(target);
}
return handle.invokeWithArguments();
return DISPATCHER.invokeWithArguments(handle);
} catch (Throwable t) {
throw new IllegalStateException("Could not read " + field + " on " + target, t);
}
Expand All @@ -246,7 +264,8 @@ public void set(Field field, Object target, Object value) throws IllegalAccessEx
new Class<?>[] {field.getType()});
boolean illegalAccess = false;
try {
Object module = getModule.bindTo(field.getDeclaringClass()).invokeWithArguments();
Object module =
DISPATCHER.invokeWithArguments(getModule.bindTo(field.getDeclaringClass()));
String packageName = field.getDeclaringClass().getPackage().getName();
assureOpen(module, packageName);
// Method handles do not allow setting final fields where setAccessible(true)
Expand All @@ -268,13 +287,15 @@ public void set(Field field, Object target, Object value) throws IllegalAccessEx
try {
MethodHandle handle =
((MethodHandles.Lookup)
privateLookupIn.invokeExact(
field.getDeclaringClass(), DISPATCHER.getLookup()))
DISPATCHER.invokeWithArguments(
privateLookupIn,
field.getDeclaringClass(),
DISPATCHER.getLookup()))
.unreflectSetter(field);
if (!Modifier.isStatic(field.getModifiers())) {
handle = handle.bindTo(target);
}
handle.invokeWithArguments(value);
DISPATCHER.invokeWithArguments(handle, value);
} finally {
if (isFinal) {
DISPATCHER.setAccessible(field, false);
Expand All @@ -290,17 +311,18 @@ public void set(Field field, Object target, Object value) throws IllegalAccessEx
}

private void assureOpen(Object module, String packageName) throws Throwable {
if (!(Boolean) isOpen.invokeWithArguments(module, packageName, DISPATCHER.getModule())) {
redefineModule
.bindTo(INSTRUMENTATION)
.invokeWithArguments(
module,
Collections.emptySet(),
Collections.emptyMap(),
Collections.singletonMap(
packageName, Collections.singleton(DISPATCHER.getModule())),
Collections.emptySet(),
Collections.emptyMap());
if (!(Boolean)
DISPATCHER.invokeWithArguments(
isOpen, module, packageName, DISPATCHER.getModule())) {
DISPATCHER.invokeWithArguments(
redefineModule.bindTo(INSTRUMENTATION),
module,
Collections.emptySet(),
Collections.emptyMap(),
Collections.singletonMap(
packageName, Collections.singleton(DISPATCHER.getModule())),
Collections.emptySet(),
Collections.emptyMap());
}
}

Expand Down Expand Up @@ -359,5 +381,10 @@ public interface Dispatcher {
Object getModule();

void setAccessible(AccessibleObject target, boolean value);

// Used to avoid invoke/invokeExact being exposed to Android where this class should
// never be loaded. Since the invocation happens from the generated code, the Android
// build pipeline does not fail.
Object invokeWithArguments(MethodHandle handle, Object... arguments) throws Throwable;
}
}