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.

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

8284942: Proxy building can just iterate superinterfaces once #8281

Closed
wants to merge 8 commits into from
@@ -495,41 +495,17 @@ private static final class ProxyBuilder {
private static final ClassLoaderValue<Boolean> reverseProxyCache =
new ClassLoaderValue<>();

private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {
String proxyPkg = null; // package to define proxy class in
int accessFlags = Modifier.PUBLIC | Modifier.FINAL;
boolean nonExported = false;
private record ProxyClassContext(Module module, String pkg, int accessFlags) { }

/*
* Record the package of a non-public proxy interface so that the
* proxy class will be defined in the same package. Verify that
* all non-public proxy interfaces are in the same package.
*/
for (Class<?> intf : interfaces) {
int flags = intf.getModifiers();
if (!Modifier.isPublic(flags)) {
accessFlags = Modifier.FINAL; // non-public, final
String pkg = intf.getPackageName();
if (proxyPkg == null) {
proxyPkg = pkg;
} else if (!pkg.equals(proxyPkg)) {
throw new IllegalArgumentException(
"non-public interfaces from different packages");
}
} else {
if (!intf.getModule().isExported(intf.getPackageName())) {
// module-private types
nonExported = true;
}
}
}
private static Class<?> defineProxyClass(ProxyClassContext context, List<Class<?>> interfaces) {
String proxyPkg = context.pkg;
int accessFlags = context.accessFlags;
Module m = context.module;

if (proxyPkg == null) {
// all proxy interfaces are public and exported
if (Modifier.isPublic(accessFlags)) {
// all proxy interfaces are public
if (!m.isNamed())
throw new InternalError("unnamed module: " + m);
proxyPkg = nonExported ? PROXY_PACKAGE_PREFIX + "." + m.getName()
: m.getName();
} else if (proxyPkg.isEmpty() && m.isNamed()) {
Copy link
Member

@mlchung mlchung May 20, 2022

Choose a reason for hiding this comment

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

With the refactoring, it may be easier to understand to make this check for both public and non-public access (i.e. drop the else) for clarity - a named module can't have unnamed package.

Copy link
Contributor Author

@liach liach May 20, 2022

Choose a reason for hiding this comment

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

a named module can't have unnamed package

Since this is mandated by the Java Language Specification 7.4.2, I am tempted to change the thrown exception to InternalError, but I cannot find any restriction in the JVM Specification that prevents declaring a package-private superinterface in the unnamed package of a named module. If we can find a reference in the JVM Specification, I'm more than glad to refer to that in a comment and throw InternalError instead.

throw new IllegalArgumentException(
"Unnamed package cannot be added to " + m);
@@ -555,6 +531,7 @@ private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {
/*
* Generate the specified proxy class.
*/
accessFlags |= Modifier.FINAL;
Copy link
Member

@mlchung mlchung May 20, 2022

Choose a reason for hiding this comment

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

It can be inlined in the call to generateProxyClass:

            byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, proxyName, interfaces, accessFlags | Modifier.FINAL);

byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, proxyName, interfaces, accessFlags);
try {
Class<?> pc = JLA.defineClass(loader, proxyName, proxyClassFile,
@@ -575,7 +552,7 @@ private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {

/**
* Test if given class is a class defined by
* {@link #defineProxyClass(Module, List)}
* {@link #defineProxyClass(ProxyClassContext, List)}
*/
static boolean isProxyClass(Class<?> c) {
return Objects.equals(reverseProxyCache.sub(c).get(c.getClassLoader()),
@@ -631,7 +608,7 @@ private static boolean isDebug(String flag) {
// ProxyBuilder instance members start here....

private final List<Class<?>> interfaces;
private final Module module;
private final ProxyClassContext context;
ProxyBuilder(ClassLoader loader, List<Class<?>> interfaces) {
if (!VM.isModuleSystemInited()) {
throw new InternalError("Proxy is not supported until "
@@ -648,8 +625,8 @@ private static boolean isDebug(String flag) {
validateProxyInterfaces(loader, interfaces, refTypes);

this.interfaces = interfaces;
this.module = mapToModule(loader, interfaces, refTypes);
assert getLoader(module) == loader;
this.context = proxyClassContext(loader, interfaces, refTypes);
assert getLoader(context.module) == loader;
}

ProxyBuilder(ClassLoader loader, Class<?> intf) {
@@ -667,7 +644,8 @@ private static boolean isDebug(String flag) {
*/
@SuppressWarnings("removal")
Constructor<?> build() {
Class<?> proxyClass = defineProxyClass(module, interfaces);
Class<?> proxyClass = defineProxyClass(context, interfaces);
Module module = context.module;
assert !module.isNamed() || module.isOpen(proxyClass.getPackageName(), Proxy.class.getModule());

final Constructor<?> cons;
@@ -768,10 +746,11 @@ private static void addElementType(HashSet<Class<?>> types,
}

/**
* Returns the module that the generated proxy class belongs to.
* Returns the context for the generated proxy class, including the
* module and the package it belongs to and whether it is package-private.
*
* If any of proxy interface is package-private, then the proxy class
* is in the same module of the package-private interface.
* is in the same package and module as the package-private interface.
*
* If all proxy interfaces are public and in exported packages,
* then the proxy class is in a dynamic module in an unconditionally
@@ -785,14 +764,21 @@ private static void addElementType(HashSet<Class<?>> types,
*
* Reads edge and qualified exports are added for dynamic module to access.
*/
private static Module mapToModule(ClassLoader loader,
List<Class<?>> interfaces,
Set<Class<?>> refTypes) {
private static ProxyClassContext proxyClassContext(ClassLoader loader,
List<Class<?>> interfaces,
Set<Class<?>> refTypes) {
Map<Class<?>, Module> packagePrivateTypes = new HashMap<>();
boolean nonExported = false;

for (Class<?> intf : interfaces) {
Module m = intf.getModule();
if (!Modifier.isPublic(intf.getModifiers())) {
packagePrivateTypes.put(intf, m);
} else {
if (!intf.getModule().isExported(intf.getPackageName())) {
// module-private types
nonExported = true;
}
}
}

@@ -838,7 +824,7 @@ private static Module mapToModule(ClassLoader loader,
Modules.addOpens(targetModule, targetPackageName, Proxy.class.getModule());
}
// return the module of the package-private interface
return targetModule;
return new ProxyClassContext(targetModule, targetPackageName, 0);
}

// All proxy interfaces are public. So maps to a dynamic proxy module
@@ -852,7 +838,10 @@ private static Module mapToModule(ClassLoader loader,
for (Class<?> c : types) {
ensureAccess(targetModule, c);
}
return targetModule;

var pkgName = nonExported ? PROXY_PACKAGE_PREFIX + '.' + targetModule.getName()
: targetModule.getName();
return new ProxyClassContext(targetModule, pkgName, Modifier.PUBLIC);
}

/*