-
Notifications
You must be signed in to change notification settings - Fork 37
8379503: Intern constant expression of type String #978
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
Changes from 21 commits
6fde7bd
cf20247
c71e110
943fa35
204fcae
e72bdb3
3cfd1d8
b21de78
188e19e
dfd6325
dd21836
50d5b82
d6b839f
5796b78
bb86b12
def145e
66f5243
f7b488b
5465171
6cdb7b5
e73916c
6d2ac0b
fb04923
60dd65c
feb5d6d
4e74ebb
247c773
e149a14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package jdk.incubator.code.dialect.java; | ||
|
|
||
| import jdk.incubator.code.Block; | ||
| import jdk.incubator.code.CodeTransformer; | ||
| import jdk.incubator.code.Op; | ||
|
|
||
| import java.lang.invoke.MethodHandles; | ||
| import java.util.Optional; | ||
|
|
||
| import static jdk.incubator.code.dialect.core.CoreOp.constant; | ||
|
|
||
| public class ConstantFolder implements CodeTransformer { | ||
|
mabbay marked this conversation as resolved.
Outdated
|
||
| private final JavaOp.JavaExpression.Evaluator evaluator; | ||
|
|
||
| private ConstantFolder(JavaOp.JavaExpression.Evaluator evaluator) { | ||
| this.evaluator = evaluator; | ||
| } | ||
|
|
||
| public static ConstantFolder getInstance(MethodHandles.Lookup l) { | ||
|
mabbay marked this conversation as resolved.
Outdated
|
||
| return new ConstantFolder(new JavaOp.JavaExpression.Evaluator(l)); | ||
| } | ||
|
|
||
| @Override | ||
| public Block.Builder acceptOp(Block.Builder b, Op op) { | ||
| Optional<Object> v = evaluator.evaluate(op.result()); | ||
| if (v.isPresent()) { | ||
| Op.Result c = b.op(constant(op.resultType(), v.get())); | ||
| b.context().mapValue(op.result(), c); | ||
| } else { | ||
| b.op(op); | ||
| } | ||
| return b; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,12 +143,7 @@ public sealed interface JavaExpression permits | |
| *} | ||
| */ | ||
| static Optional<Object> evaluate(MethodHandles.Lookup l, Value v) { | ||
| try { | ||
| Object o = eval(l, v); | ||
| return Optional.of(o); | ||
| } catch (NonConstantExpression e) { | ||
| return Optional.empty(); | ||
| } | ||
| return new Evaluator(l).evaluate(v); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -180,151 +175,177 @@ static Optional<Object> evaluate(MethodHandles.Lookup l, Value v) { | |
| * @jls 15.29 Constant Expressions | ||
| */ | ||
| static <T extends Op & JavaExpression> Optional<Object> evaluate(MethodHandles.Lookup l, T op) { | ||
| try { | ||
| Object v = eval(l, op); | ||
| return Optional.of(v); | ||
| } catch (NonConstantExpression e) { | ||
| return Optional.empty(); | ||
| } | ||
| return new Evaluator(l).evaluate(op); | ||
| } | ||
|
|
||
| private static Object eval(MethodHandles.Lookup l, Op op) { | ||
| return switch (op) { | ||
| case CoreOp.ConstantOp cop when isConstant(cop) -> { | ||
| Object v = cop.value(); | ||
| yield v instanceof String s ? s.intern() : v; | ||
| class Evaluator { | ||
|
mabbay marked this conversation as resolved.
Outdated
|
||
| private final MethodHandles.Lookup l; | ||
| private final Map<Value, Object> m = new HashMap<>(); | ||
|
|
||
| Evaluator(MethodHandles.Lookup l) { | ||
| this.l = l; | ||
| } | ||
|
|
||
| <T extends Op & JavaExpression> Optional<Object> evaluate(T op) { | ||
| try { | ||
| Object v = this.eval(op); | ||
| return Optional.of(v); | ||
| } catch (NonConstantExpression e) { | ||
| return Optional.empty(); | ||
| } | ||
| case CoreOp.VarAccessOp.VarLoadOp varLoadOp when isConstant(varLoadOp.varOp()) -> | ||
| eval(l, varLoadOp.varOp().initOperand()); | ||
| case JavaOp.ConvOp _ -> { | ||
| // we expect cast to primitive type | ||
| var v = eval(l, op.operands().getFirst()); | ||
| yield ArithmeticAndConvOpImpls.evaluate(op, List.of(v)); | ||
| } | ||
|
|
||
| Optional<Object> evaluate(Value v) { | ||
| try { | ||
| Object o = this.eval(v); | ||
| return Optional.of(o); | ||
| } catch (NonConstantExpression e) { | ||
| return Optional.empty(); | ||
| } | ||
| case CastOp castOp -> { | ||
| // we expect cast to String | ||
| Value operand = castOp.operands().getFirst(); | ||
| if (!castOp.resultType().equals(J_L_STRING) || !operand.type().equals(J_L_STRING)) { | ||
| throw new NonConstantExpression(); | ||
| } | ||
|
|
||
| private Object eval(Op op) { | ||
| if (m.containsKey(op.result())) { | ||
| return m.get(op.result()); | ||
| } | ||
| Object r = switch (op) { | ||
| case ConstantOp cop when isConstant(cop) -> { | ||
| Object v = cop.value(); | ||
| yield v instanceof String s ? s.intern() : v; | ||
| } | ||
| Object v = eval(l, operand); | ||
| if (!(v instanceof String s)) { | ||
| throw new NonConstantExpression(); | ||
| case VarAccessOp.VarLoadOp varLoadOp when varLoadOp.operands().getFirst() instanceof Result && | ||
|
mabbay marked this conversation as resolved.
|
||
| isConstant(varLoadOp.varOp()) -> eval(varLoadOp.varOp().initOperand()); | ||
| case ConvOp _ -> { | ||
| // we expect cast to primitive type | ||
| var v = eval(op.operands().getFirst()); | ||
| yield ArithmeticAndConvOpImpls.evaluate(op, List.of(v)); | ||
| } | ||
| yield s; | ||
| } | ||
| case ConcatOp concatOp -> { | ||
| Object first = eval(l, concatOp.operands().getFirst()); | ||
| Object second = eval(l, concatOp.operands().getLast()); | ||
| yield (first.toString() + second).intern(); | ||
| } | ||
| case JavaOp.FieldAccessOp.FieldLoadOp fieldLoadOp -> { | ||
| Field field; | ||
| VarHandle vh; | ||
| try { | ||
| field = fieldLoadOp.fieldReference().resolveToField(l); | ||
| vh = fieldLoadOp.fieldReference().resolveToHandle(l); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new IllegalArgumentException(e); | ||
| case CastOp castOp -> { | ||
| // we expect cast to String | ||
| Value operand = castOp.operands().getFirst(); | ||
| if (!castOp.resultType().equals(J_L_STRING) || !operand.type().equals(J_L_STRING)) { | ||
| throw new NonConstantExpression(); | ||
| } | ||
| Object v = eval(operand); | ||
| if (!(v instanceof String s)) { | ||
| throw new NonConstantExpression(); | ||
| } | ||
| yield s; | ||
| } | ||
| // Requirement: the field must be a constant variable. | ||
| // Current checks: | ||
| // 1) The field is declared final. | ||
| // 2) The field type is a primitive or String. | ||
| // Missing check: | ||
| // 3) Verify the field is initialized and the initializer is a constant expression. | ||
| if ((field.getModifiers() & Modifier.FINAL) == 0 || | ||
| !isConstantType(fieldLoadOp.fieldReference().type())) { | ||
| throw new NonConstantExpression(); | ||
| case ConcatOp concatOp -> { | ||
| Object first = eval(concatOp.operands().getFirst()); | ||
| Object second = eval(concatOp.operands().getLast()); | ||
| yield (first.toString() + second).intern(); | ||
| } | ||
| Object v; | ||
| if ((field.getModifiers() & Modifier.STATIC) != 0) { | ||
| // @@@ why using field.get fails ? | ||
| v = vh.get(); | ||
| } else { | ||
| // we can't get the value of an instance field from the model | ||
| // we need the value of the receiver | ||
| throw new NonConstantExpression(); | ||
| case FieldAccessOp.FieldLoadOp fieldLoadOp -> { | ||
| Field field; | ||
| VarHandle vh; | ||
| try { | ||
| field = fieldLoadOp.fieldReference().resolveToField(l); | ||
| vh = fieldLoadOp.fieldReference().resolveToHandle(l); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new IllegalArgumentException(e); | ||
| } | ||
| // Requirement: the field must be a constant variable. | ||
| // Current checks: | ||
| // 1) The field is declared final. | ||
| // 2) The field type is a primitive or String. | ||
| // Missing check: | ||
| // 3) Verify the field is initialized and the initializer is a constant expression. | ||
| if ((field.getModifiers() & Modifier.FINAL) == 0 || | ||
| !isConstantType(fieldLoadOp.fieldReference().type())) { | ||
| throw new NonConstantExpression(); | ||
| } | ||
| Object v; | ||
| if ((field.getModifiers() & Modifier.STATIC) != 0) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you check for STATIC in the above -- e.g. when you also check for FINAL ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first checks are for constant variables (as per JLS). The second check is due to the limitation we have in the model, as we can't get the value of an instance field. |
||
| // @@@ why using field.get fails ? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you try field::get(null) ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it throws
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed this offline -- this failure is due to the fact that |
||
| v = vh.get(); | ||
| } else { | ||
| // we can't get the value of an instance field from the model | ||
| // we need the value of the receiver | ||
| throw new NonConstantExpression(); | ||
| } | ||
| yield v instanceof String s ? s.intern() : v; | ||
| } | ||
| yield v instanceof String s ? s.intern() : v; | ||
| } | ||
| case ArithmeticOperation _ -> { | ||
| List<Object> values = op.operands().stream().map(v -> eval(l, v)).toList(); | ||
| yield ArithmeticAndConvOpImpls.evaluate(op, values); | ||
| } | ||
| case JavaOp.ConditionalExpressionOp _ -> { | ||
| boolean p = evalBoolean(l, op.bodies().get(0)); | ||
| Object t = eval(l, op.bodies().get(1)); | ||
| Object f = eval(l, op.bodies().get(2)); | ||
| yield p ? t : f; | ||
| } | ||
| case JavaOp.ConditionalAndOp _ -> { | ||
| boolean left = evalBoolean(l, op.bodies().get(0)); | ||
| boolean right = evalBoolean(l, op.bodies().get(1)); | ||
| yield left && right; | ||
| } | ||
| case JavaOp.ConditionalOrOp _ -> { | ||
| boolean left = evalBoolean(l, op.bodies().get(0)); | ||
| boolean right = evalBoolean(l, op.bodies().get(1)); | ||
| yield left || right; | ||
| } | ||
| default -> throw new NonConstantExpression(); | ||
| }; | ||
| } | ||
|
|
||
| private static Object eval(MethodHandles.Lookup l, Value v) { | ||
| if (v.declaringElement() instanceof JavaExpression e) { | ||
| return eval(l, (Op & JavaExpression) e); | ||
| case ArithmeticOperation _ -> { | ||
| List<Object> values = op.operands().stream().map(this::eval).toList(); | ||
| yield ArithmeticAndConvOpImpls.evaluate(op, values); | ||
| } | ||
| case ConditionalExpressionOp _ -> { | ||
| boolean p = evalBoolean(op.bodies().get(0)); | ||
| Object t = eval(op.bodies().get(1)); // I need to have a version that accept operand evaluator, for every eval* method | ||
|
mabbay marked this conversation as resolved.
Outdated
|
||
| Object f = eval(op.bodies().get(2)); | ||
| yield p ? t : f; | ||
| } | ||
| case ConditionalAndOp _ -> { | ||
| boolean left = evalBoolean(op.bodies().get(0)); | ||
| boolean right = evalBoolean(op.bodies().get(1)); | ||
| yield left && right; | ||
| } | ||
| case ConditionalOrOp _ -> { | ||
| boolean left = evalBoolean(op.bodies().get(0)); | ||
| boolean right = evalBoolean(op.bodies().get(1)); | ||
| yield left || right; | ||
| } | ||
| default -> throw new NonConstantExpression(); | ||
| }; | ||
| m.put(op.result(), r); | ||
| return r; | ||
| } | ||
| throw new NonConstantExpression(); | ||
| } | ||
|
|
||
| private static Object eval(MethodHandles.Lookup l, Body body) throws NonConstantExpression { | ||
| if (body.blocks().size() != 1 || | ||
| !(body.entryBlock().terminatingOp() instanceof CoreOp.YieldOp yop) || | ||
| yop.yieldValue() == null || | ||
| !isConstantType(yop.yieldValue().type())) { | ||
| private Object eval(Value v) { | ||
| if (v.declaringElement() instanceof JavaExpression e) { | ||
| return eval((Op & JavaExpression) e); | ||
| } | ||
| throw new NonConstantExpression(); | ||
| } | ||
| return eval(l, yop.yieldValue()); | ||
| } | ||
|
|
||
| private static boolean evalBoolean(MethodHandles.Lookup l, Body body) throws NonConstantExpression { | ||
| Object eval = eval(l, body); | ||
| if (!(eval instanceof Boolean b)) { | ||
| throw new NonConstantExpression(); | ||
| private Object eval(Body body) throws NonConstantExpression { | ||
| if (body.blocks().size() != 1 || | ||
| !(body.entryBlock().terminatingOp() instanceof CoreOp.YieldOp yop) || | ||
| yop.yieldValue() == null || | ||
| !isConstantType(yop.yieldValue().type())) { | ||
| throw new NonConstantExpression(); | ||
| } | ||
| return eval(yop.yieldValue()); | ||
| } | ||
|
|
||
| return b; | ||
| } | ||
| private boolean evalBoolean(Body body) throws NonConstantExpression { | ||
| Object eval = eval(body); | ||
| if (!(eval instanceof Boolean b)) { | ||
| throw new NonConstantExpression(); | ||
| } | ||
| return b; | ||
| } | ||
|
|
||
| private static boolean isConstant(CoreOp.ConstantOp op) { | ||
| return isConstantType(op.resultType()) && isConstantValue(op.value()); | ||
| } | ||
| private static boolean isConstant(CoreOp.ConstantOp op) { | ||
| return isConstantType(op.resultType()) && isConstantValue(op.value()); | ||
| } | ||
|
|
||
| private static boolean isConstant(VarOp op) { | ||
| // Requirement: the local variable must be a constant variable. | ||
| // Current checks: | ||
| // 1) The variable is initialized, and the initializer is a constant expression. | ||
| // 2) The variable type is a primitive or String. | ||
| // Missing check: | ||
| // 3) Ensure the variable is declared final | ||
| return isConstantType(op.varValueType()) && | ||
| !op.isUninitialized() && | ||
| // @@@ Add to VarOp | ||
| op.result().uses().stream().noneMatch(u -> u.op() instanceof CoreOp.VarAccessOp.VarStoreOp); | ||
| } | ||
| private static boolean isConstant(VarOp op) { | ||
| // Requirement: the local variable must be a constant variable. | ||
| // Current checks: | ||
| // 1) The variable is initialized, and the initializer is a constant expression. | ||
| // 2) The variable type is a primitive or String. | ||
| // Missing check: | ||
| // 3) Ensure the variable is declared final | ||
| return isConstantType(op.varValueType()) && | ||
| !op.isUninitialized() && | ||
| // @@@ Add to VarOp | ||
| op.result().uses().stream().noneMatch(u -> u.op() instanceof CoreOp.VarAccessOp.VarStoreOp); | ||
| } | ||
|
|
||
| private static boolean isConstantValue(Object o) { | ||
| return switch (o) { | ||
| case String _ -> true; | ||
| case Boolean _, Byte _, Short _, Character _, Integer _, Long _, Float _, Double _ -> true; | ||
| case null, default -> false; | ||
| }; | ||
| } | ||
| private static boolean isConstantValue(Object o) { | ||
| return switch (o) { | ||
| case String _ -> true; | ||
| case Boolean _, Byte _, Short _, Character _, Integer _, Long _, Float _, Double _ -> true; | ||
| case null, default -> false; | ||
| }; | ||
| } | ||
|
|
||
| private static boolean isConstantType(TypeElement e) { | ||
| return (e instanceof PrimitiveType && !VOID.equals(e)) || J_L_STRING.equals(e); | ||
| private static boolean isConstantType(TypeElement e) { | ||
| return (e instanceof PrimitiveType && !VOID.equals(e)) || J_L_STRING.equals(e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.