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

Fix for type.getType(...) use on non-signature type names #221

Merged
merged 8 commits into from
Sep 15, 2023
Merged

Fix for type.getType(...) use on non-signature type names #221

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4816a0f
Fix a regression of PR#171 - Correct usage of Type.getType(...)
May 2, 2023
c25de87
Remove fall through
May 2, 2023
8fd3efd
added tests - do not throw
May 2, 2023
e8f89f0
fix coverage
May 2, 2023
8ad6cc4
fix: use StringUtils.isEmpty(internalTypeName)
May 2, 2023
19121c0
Fix name of method to camel case: internalTypeNameToSignature
May 2, 2023
c1e615d
Make Const.SHORT_TYPE_NAMES public for Type.internalTypeNameToSignature
Aug 29, 2023
7baad01
Checkstyle WhitespaceAround
garydgregory Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/apache/bcel/Const.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2834,7 +2834,7 @@ public final class Const {
/**
* The signature characters corresponding to primitive types, e.g., SHORT_TYPE_NAMES[T_INT] = "I"
*/
private static final String[] SHORT_TYPE_NAMES = {ILLEGAL_TYPE, ILLEGAL_TYPE, ILLEGAL_TYPE, ILLEGAL_TYPE, "Z", "C", "F", "D", "B", "S", "I", "J", "V",
public static final String[] SHORT_TYPE_NAMES = {ILLEGAL_TYPE, ILLEGAL_TYPE, ILLEGAL_TYPE, ILLEGAL_TYPE, "Z", "C", "F", "D", "B", "S", "I", "J", "V",
ILLEGAL_TYPE, ILLEGAL_TYPE, ILLEGAL_TYPE};

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/apache/bcel/generic/LDC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public Object getValue(final ConstantPoolGen cpg) {
case org.apache.bcel.Const.CONSTANT_Class:
final int nameIndex = ((org.apache.bcel.classfile.ConstantClass) c).getNameIndex();
c = cpg.getConstantPool().getConstant(nameIndex);
return Type.getType(((org.apache.bcel.classfile.ConstantUtf8) c).getBytes());
return Type.getType(Type.internalTypeNameToSignature(((org.apache.bcel.classfile.ConstantUtf8) c).getBytes()));
default: // Never reached
throw new IllegalArgumentException("Unknown or invalid constant type at " + super.getIndex());
}
Expand Down
21 changes: 20 additions & 1 deletion src/main/java/org/apache/bcel/generic/Type.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.bcel.classfile.ClassFormatException;
import org.apache.bcel.classfile.InvalidMethodSignatureException;
import org.apache.bcel.classfile.Utility;
import org.apache.commons.lang3.StringUtils;

/**
* Abstract super class for all possible java types, namely basic types such as int, object types like String and array
Expand Down Expand Up @@ -180,7 +181,7 @@ public static String getSignature(final java.lang.reflect.Method meth) {
public static Type getType(final Class<?> cls) {
Objects.requireNonNull(cls, "cls");
/*
* That's an amzingly easy case, because getName() returns the signature. That's what we would have liked anyway.
* That's an amazingly easy case, because getName() returns the signature. That's what we would have liked anyway.
Copy link
Member

Choose a reason for hiding this comment

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

Good one! :-)

*/
if (cls.isArray()) {
return getType(cls.getName());
Expand Down Expand Up @@ -365,6 +366,24 @@ public int hashCode() {
return type ^ signature.hashCode();
}

static String internalTypeNameToSignature(final String internalTypeName) {
if (StringUtils.isEmpty(internalTypeName) || StringUtils.equalsAny(internalTypeName, Const.SHORT_TYPE_NAMES)) {
return internalTypeName;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not crazy about this magic list of strings. Could we reuse or refactor org.apache.bcel.generic.InstructionFactory.SHORT_NAMES (by making it public)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The array has different values

Copy link

Choose a reason for hiding this comment

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

I couldn't find any array containing these exact short names. The mentioned SHORT_NAMES misses Z, and J, but contains L additionally. The SHORT_TYPE_NAMES in Consts and Constants both contain V and ILLEGAL_TYPE unnecessarily. There are constants for the short names individually in ElementValue, but I'm not sure using those would help a lot.

Copy link
Member

Choose a reason for hiding this comment

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

Hi all,
I'm looking for clarity when reading the code. A hard coded array of strings is bad. Having a good name for a constant would help, and SHORT_NAMES is not a good name when the class is Constants or Consts. Any improvement is appreciated.

Copy link

Choose a reason for hiding this comment

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

Since none of the existing arrays could be reused here because of the different values, the renaming should be in a separate PR, not in this one. In the JVM specification these short names are introduced as field descriptors, but that name may cause some confusion with the field and it rather refers to the type of the fields.
As far as I can see, this particular "array" contains the primitive or base types, but naming it is the privilege of the author.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

InstructionFactory.SHORT_NAMES wasn't a good fit but Const.SHORT_TYPE_NAMES looks better suited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garydgregory is it ok now ?

Copy link

Choose a reason for hiding this comment

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

@garydgregory any chance you can have a look at it?

Copy link
Member

Choose a reason for hiding this comment

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

Sometime this week...

Copy link

Choose a reason for hiding this comment

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

@garydgregory can you please look at this PR?

}
switch(internalTypeName.charAt(0)) {
case '[':
return internalTypeName;
case 'L':
case 'T':
if (internalTypeName.charAt(internalTypeName.length() - 1) == ';') {
return internalTypeName;
}
return 'L' + internalTypeName + ';';
default:
return 'L' + internalTypeName + ';';
}
}

/**
* boolean, short and char variable are considered as int in the stack or local variable area. Returns {@link Type#INT}
* for {@link Type#BOOLEAN}, {@link Type#SHORT} or {@link Type#CHAR}, otherwise returns the given type.
Expand Down
70 changes: 68 additions & 2 deletions src/test/java/org/apache/bcel/generic/TypeTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,16 @@
*/
package org.apache.bcel.generic;

import static org.junit.jupiter.api.Assertions.assertEquals;

import org.apache.bcel.Repository;
import org.apache.bcel.classfile.Code;
import org.apache.bcel.classfile.JavaClass;
import org.apache.bcel.classfile.Method;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

public class TypeTestCase {
@Test
Expand All @@ -32,4 +39,63 @@ public void testBCEL243() {
assertEquals(expectedValue, actualValue, "Type.getType");
}

@ParameterizedTest
@ValueSource(strings = {
// @formatter:off
"java/io/Externalizable",
"java/io/ObjectOutputStream",
"java/io/Serializable",
"java/lang/Cloneable",
"java/lang/RuntimeException",
"java/lang/String",
"java/lang/System",
"java/lang/Throwable",
"java/net/URI",
"java/sql/Statement",
"java/util/ArrayList",
"java/util/Calendar",
"java/util/EnumMap",
"java/util/HashSet",
"java/util/Iterator",
"java/util/LinkedList",
"java/util/List",
"java/util/Map",
"java/util/concurrent/ConcurrentMap",
"java/util/concurrent/ExecutorService",
"org/apache/bcel/classfile/JavaClass",
"org/apache/bcel/classfile/Method",
"org/apache/bcel/classfile/Synthetic",
"org/apache/bcel/generic/ConstantPoolGen",
"org/apache/bcel/generic/MethodGen"})
// @formatter:on
public void testLDC(final String className) throws Exception {
final JavaClass jc = Repository.lookupClass(className);
final ConstantPoolGen cpg = new ConstantPoolGen(jc.getConstantPool());
for (final Method method : jc.getMethods()) {
final Code code = method.getCode();
if (code != null) {
final InstructionList instructionList = new InstructionList(code.getCode());
for (final InstructionHandle instructionHandle : instructionList) {
instructionHandle.accept(new EmptyVisitor() {
@Override
public void visitLDC(LDC obj) {
assertNotNull(obj.getValue(cpg));
}
});
}
}
}
}

@Test
public void testInternalTypeNametoSignature() {
assertEquals(null, Type.internalTypeNameToSignature(null));
assertEquals("", Type.internalTypeNameToSignature(""));
assertEquals("TT;", Type.internalTypeNameToSignature("TT;"));
assertEquals("Ljava/lang/String;", Type.internalTypeNameToSignature("Ljava/lang/String;"));
assertEquals("[Ljava/lang/String;", Type.internalTypeNameToSignature("[Ljava/lang/String;"));
assertEquals("Ljava/lang/String;", Type.internalTypeNameToSignature("java/lang/String"));
assertEquals("I", Type.internalTypeNameToSignature("I"));
assertEquals("LT;", Type.internalTypeNameToSignature("T"));
}
}