Skip to content

Commit

Permalink
[WASM] Change PropagateConstants to process JsEnum constants separately.
Browse files Browse the repository at this point in the history
There is an issue around ordering of PropagateConstants, string concatenation passes, and JsEnum boxing/unboxing passes:

- PropagateConstants must run before StaticallyEvaluateStringConcatenation, which must run before ImplementStringConcatenation.
- InsertJsEnumBoxingAndUnboxingConversions must run after ImplementStringConcatenation but before PropagateConstants, because PropagateConstants removes type information around JsEnums.

To account for this, we split PropagateConstants to handle JsEnums separately, so that we can run the non-JsEnum part before JsEnum boxing/unboxing, and the JsEnum part afterwards.

Note: This change is a no-op for the output
PiperOrigin-RevId: 570753214
  • Loading branch information
Googler authored and copybara-github committed Oct 4, 2023
1 parent d92d143 commit 2a511c2
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 14 deletions.
12 changes: 12 additions & 0 deletions transpiler/java/com/google/j2cl/transpiler/ast/AstUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,18 @@ public static boolean isJsEnumCustomValueField(MemberDescriptor memberDescriptor
&& memberDescriptor.getEnclosingTypeDescriptor().isJsEnum();
}

public static FieldDescriptor createJsEnumConstantFieldDescriptor(
FieldDescriptor fieldDescriptor) {
TypeDescriptor enumValueType =
getJsEnumValueFieldType(fieldDescriptor.getEnclosingTypeDescriptor().getTypeDeclaration());
return FieldDescriptor.Builder.from(fieldDescriptor)
.setTypeDescriptor(enumValueType)
.setFinal(true)
.setCompileTimeConstant(true)
.setEnumConstant(true)
.build();
}

/** Returns the value field for a JsEnum. */
public static TypeDescriptor getJsEnumValueFieldType(TypeDeclaration typeDeclaration) {
FieldDescriptor valueFieldDescriptor = getJsEnumValueFieldDescriptor(typeDeclaration);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@
import com.google.j2cl.transpiler.passes.OptimizeAutoValue;
import com.google.j2cl.transpiler.passes.OptimizeEnums;
import com.google.j2cl.transpiler.passes.PropagateConstants;
import com.google.j2cl.transpiler.passes.PropagateJsEnumConstants;
import com.google.j2cl.transpiler.passes.PropagateNullabilityJ2kt;
import com.google.j2cl.transpiler.passes.RecoverShortcutBooleanOperator;
import com.google.j2cl.transpiler.passes.RemoveAssertStatements;
Expand Down Expand Up @@ -411,6 +412,7 @@ public ImmutableList<Supplier<NormalizationPass>> getPassFactories(BackendOption
// Propagate constants needs to run after NormalizeSwitchStatements since it introduces
// field references to constant fields.
PropagateConstants::new,
PropagateJsEnumConstants::new,
StaticallyEvaluateStringConcatenation::new,
StaticallyEvaluateStringComparison::new,
ImplementStringConcatenation::new,
Expand Down Expand Up @@ -544,6 +546,7 @@ public ImmutableList<Supplier<NormalizationPass>> getPassFactories(BackendOption
// Propagate constants needs to run after NormalizeSwitchStatements since it introduces
// field references to constant fields.
PropagateConstants::new,
PropagateJsEnumConstants::new,
StaticallyEvaluateStringConcatenation::new,
StaticallyEvaluateStringComparison::new,
ImplementStringConcatenation::new,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.j2cl.transpiler.ast.Expression;
import com.google.j2cl.transpiler.ast.Field;
import com.google.j2cl.transpiler.ast.FieldAccess;
import com.google.j2cl.transpiler.ast.FieldDescriptor;
import com.google.j2cl.transpiler.ast.FieldDescriptor.FieldOrigin;
import com.google.j2cl.transpiler.ast.JsDocCastExpression;
import com.google.j2cl.transpiler.ast.MemberReference;
Expand Down Expand Up @@ -106,18 +105,8 @@ public Node rewriteMethod(Method method) {
* invocation.
*/
private static Field createJsEnumConstant(Field field) {
TypeDescriptor enumValueType =
AstUtils.getJsEnumValueFieldType(
field.getDescriptor().getEnclosingTypeDescriptor().getTypeDeclaration());
FieldDescriptor closureEnumConstantFieldDescriptor =
FieldDescriptor.Builder.from(field.getDescriptor())
.setTypeDescriptor(enumValueType)
.setFinal(true)
.setCompileTimeConstant(true)
.setEnumConstant(true)
.build();
return Field.Builder.from(field)
.setDescriptor(closureEnumConstantFieldDescriptor)
.setDescriptor(AstUtils.createJsEnumConstantFieldDescriptor(field.getDescriptor()))
.setInitializer(getJsEnumConstantValue(field))
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static com.google.common.base.Preconditions.checkState;

import com.google.j2cl.transpiler.ast.AbstractRewriter;
import com.google.j2cl.transpiler.ast.AstUtils;
import com.google.j2cl.transpiler.ast.Expression;
import com.google.j2cl.transpiler.ast.Field;
import com.google.j2cl.transpiler.ast.FieldAccess;
Expand Down Expand Up @@ -71,7 +72,10 @@ private void rewriteCompileTimeConstantFieldReferences(
new AbstractRewriter() {
@Override
public Node rewriteFieldAccess(FieldAccess fieldAccess) {
FieldDescriptor target = fieldAccess.getTarget();
if (!shouldPropagateConstant(fieldAccess.getTarget())) {
return fieldAccess;
}
FieldDescriptor target = getConstantFieldDescriptor(fieldAccess);
Expression literal = literalsByField.get(target.getDeclarationDescriptor());
if (literal == null) {
return fieldAccess;
Expand All @@ -84,7 +88,15 @@ public Node rewriteFieldAccess(FieldAccess fieldAccess) {
});
}

private static boolean isCompileTimeConstant(Field field) {
/** Gets the relevant field descriptor for the specified {@link FieldAccess}. */
protected FieldDescriptor getConstantFieldDescriptor(FieldAccess fieldAccess) {
return fieldAccess.getTarget();
}

private boolean isCompileTimeConstant(Field field) {
if (!shouldPropagateConstant(field.getDescriptor())) {
return false;
}
return field.isCompileTimeConstant()
// Consider final static fields that are initialized to literals to be compile time
// constants. These might be driven from TypeLiterals or System.getProperty calls and it
Expand All @@ -94,4 +106,10 @@ private static boolean isCompileTimeConstant(Field field) {
&& (field.getInitializer() instanceof TypeLiteral
|| field.getInitializer() instanceof StringLiteral));
}

protected boolean shouldPropagateConstant(FieldDescriptor fieldDescriptor) {
// Skip JsEnum constants, which must be handled separately.
return !(fieldDescriptor.isEnumConstant()
&& AstUtils.isNonNativeJsEnum(fieldDescriptor.getEnclosingTypeDescriptor()));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright 2023 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.google.j2cl.transpiler.passes;

import com.google.j2cl.transpiler.ast.AstUtils;
import com.google.j2cl.transpiler.ast.FieldAccess;
import com.google.j2cl.transpiler.ast.FieldDescriptor;

// TODO(b/303316598): This approach needs more thought for modular. With modular transpilation, we
// don't have enough information about JsEnum to propagate, in particular, the AST which includes
// the constant field values.
/** Propagates JsEnum constant fields. */
public class PropagateJsEnumConstants extends PropagateConstants {

@Override
protected boolean shouldPropagateConstant(FieldDescriptor fieldDescriptor) {
return fieldDescriptor.isEnumConstant()
&& AstUtils.isNonNativeJsEnum(fieldDescriptor.getEnclosingTypeDescriptor());
}

// TODO(b/303309109) This is a workaround for NormalizeJsEnums not updating field references to
// point to the correct descriptor. Ideally, this shouldn't be needed. Clean up after rethinking
// JsEnum normalization passes.
@Override
protected FieldDescriptor getConstantFieldDescriptor(FieldAccess fieldAccess) {
return AstUtils.createJsEnumConstantFieldDescriptor(fieldAccess.getTarget());
}
}

0 comments on commit 2a511c2

Please sign in to comment.