Skip to content

Commit

Permalink
Cleanup ambiguous proto fields
Browse files Browse the repository at this point in the history 
With the migration to using fully qualified names for extension fields of protos, it is no longer possible for there to be fields in the map with the same name.

GITHUB_BREAKING_CHANGES=None

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=302026543
  • Loading branch information
@emspishak
Soy Authors authored and emspishak committed Mar 23, 2020
1 parent efd4231 commit 4ed9f43
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 169 deletions.
62 changes: 3 additions & 59 deletions java/src/com/google/template/soy/data/SoyProtoValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,22 +69,10 @@ private static final class ProtoClass {
}
}

private abstract static class FieldWithInterpreter extends Field {
FieldWithInterpreter(FieldDescriptor fieldDesc) {
super(fieldDesc);
}

abstract SoyValue interpretField(Message owner);

abstract void assignField(Message.Builder builder, SoyValue value);

abstract boolean hasField(Message proto);
}

private static final class NormalFieldWithInterpreter extends FieldWithInterpreter {
private static final class FieldWithInterpreter extends Field {
@LazyInit ProtoFieldInterpreter interpreter;

NormalFieldWithInterpreter(FieldDescriptor fieldDesc) {
FieldWithInterpreter(FieldDescriptor fieldDesc) {
super(fieldDesc);
}

Expand All @@ -96,17 +84,14 @@ private ProtoFieldInterpreter impl() {
return local;
}

@Override
public SoyValue interpretField(Message message) {
return impl().soyFromProto(message.getField(getDescriptor()));
}

@Override
public void assignField(Message.Builder builder, SoyValue value) {
builder.setField(getDescriptor(), impl().protoFromSoy(value));
}

@Override
boolean hasField(Message proto) {
// TODO(lukes): Currently we assume that a field is present if it is repeated, has an
// explicit default value or is set. However, the type of fields is not generally nullable,
Expand All @@ -117,53 +102,12 @@ boolean hasField(Message proto) {
}
}

private static final class AmbiguousFieldWithInterpreter extends FieldWithInterpreter {
final Set<FieldWithInterpreter> fields;

AmbiguousFieldWithInterpreter(Set<FieldWithInterpreter> fields) {
super(fields.iterator().next().getDescriptor());
this.fields = fields;
}

@Override
SoyValue interpretField(Message owner) {
throw ambiguousFieldsError(getName(), fields);
}

@Override
void assignField(com.google.protobuf.Message.Builder builder, SoyValue value) {
throw ambiguousFieldsError(getName(), fields);
}

@Override
boolean hasField(Message proto) {
for (FieldWithInterpreter field : fields) {
if (field.hasField(proto)) {
return true;
}
}
return false;
}
}

private static final LoadingCache<Descriptor, ProtoClass> classCache =
CacheBuilder.newBuilder()
.weakKeys()
.build(
new CacheLoader<Descriptor, ProtoClass>() {
final Field.Factory<FieldWithInterpreter> factory =
new Field.Factory<FieldWithInterpreter>() {
@Override
public FieldWithInterpreter create(FieldDescriptor fieldDescriptor) {
return new NormalFieldWithInterpreter(fieldDescriptor);
}

@Override
public FieldWithInterpreter createAmbiguousFieldSet(
Set<FieldWithInterpreter> fields) {
return new AmbiguousFieldWithInterpreter(fields);
}
};
final Field.Factory<FieldWithInterpreter> factory = FieldWithInterpreter::new;

@Override
public ProtoClass load(Descriptor descriptor) throws Exception {
Expand Down
72 changes: 3 additions & 69 deletions java/src/com/google/template/soy/internal/proto/Field.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,9 @@

import com.google.common.base.CaseFormat;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Multimaps;
import com.google.common.collect.SetMultimap;
import com.google.protobuf.Descriptors.Descriptor;
import com.google.protobuf.Descriptors.FieldDescriptor;
import com.google.template.soy.base.SourceLocation;
import com.google.template.soy.error.ErrorReporter;
import com.google.template.soy.error.SoyErrorKind;
import java.util.Map;
import java.util.Set;
import java.util.logging.Logger;

/**
* A proto member field.
Expand All @@ -41,69 +31,30 @@
* be handled by subclasses.
*/
public abstract class Field {
private static final Logger logger = Logger.getLogger(Field.class.getName());

private static final SoyErrorKind AMBIGUOUS_FIELDS_ERROR =
SoyErrorKind.of(
"Cannot access {0}. It may refer to any one of the following extensions, "
+ "and Soy does not have enough information to decide which.\n"
+ "{1}\n"
+ "To resolve ensure "
+ "that all extension fields accessed from soy have unique names.");

/** A factory for field types. */
public interface Factory<T extends Field> {
/** Returns a field. */
T create(FieldDescriptor fieldDescriptor);

/**
* Creates a field for when there are several fields with conflicting soy names. This happens in
* the case of extensions. It is expected that the concrete subclass throw an appropriate
* exception (like {@link #ambiguousFieldsError}) when accessed.
*/
T createAmbiguousFieldSet(Set<T> fields);
}

/** Returns the set of fields indexed by soy accessor name for the given type. */
public static <T extends Field> ImmutableMap<String, T> getFieldsForType(
Descriptor descriptor, Set<FieldDescriptor> extensions, Factory<T> factory) {
SetMultimap<String, T> fieldsBySoyName =
MultimapBuilder.hashKeys().linkedHashSetValues().build();
ImmutableMap.Builder<String, T> fields = ImmutableMap.builder();
for (FieldDescriptor fieldDescriptor : descriptor.getFields()) {
if (ProtoUtils.shouldJsIgnoreField(fieldDescriptor)) {
continue;
}
T field = factory.create(fieldDescriptor);
fieldsBySoyName.put(field.getName(), field);
fields.put(field.getName(), field);
}

for (FieldDescriptor extension : extensions) {
T field = factory.create(extension);

// Store fully qualified name of extension fields.
fieldsBySoyName.put(field.getFullyQualifiedName(), field);
}

ImmutableMap.Builder<String, T> fields = ImmutableMap.builder();
for (Map.Entry<String, Set<T>> group : Multimaps.asMap(fieldsBySoyName).entrySet()) {
Set<T> ambiguousFields = group.getValue();
String fieldName = group.getKey();
if (ambiguousFields.size() == 1) {
fields.put(fieldName, Iterables.getOnlyElement(ambiguousFields));
} else {
T value = factory.createAmbiguousFieldSet(ambiguousFields);
logger.severe(
"Proto "
+ descriptor.getFullName()
+ " has multiple extensions with the name \""
+ fieldName
+ "\": "
+ fullFieldNames(ambiguousFields)
+ "\nThis field will not be accessible from soy");
fields.put(fieldName, value);
}
fields.put(field.getFullyQualifiedName(), field);
}

return fields.build();
}

Expand Down Expand Up @@ -171,21 +122,4 @@ private static String fieldSuffix(FieldDescriptor field) {
return "";
}
}

protected static RuntimeException ambiguousFieldsError(String name, Set<? extends Field> fields) {
return new IllegalStateException(AMBIGUOUS_FIELDS_ERROR.format(name, fullFieldNames(fields)));
}

public static void reportAmbiguousFieldsError(
ErrorReporter reporter, SourceLocation location, String name, Set<? extends Field> fields) {
reporter.report(location, AMBIGUOUS_FIELDS_ERROR, name, fullFieldNames(fields));
}

private static ImmutableSet<String> fullFieldNames(Set<? extends Field> fields) {
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
for (Field field : fields) {
builder.add(field.getDescriptor().getFullName());
}
return builder.build();
}
}
7 changes: 3 additions & 4 deletions java/src/com/google/template/soy/passes/ResolveExpressionTypesPass.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,7 @@ private void visitGetExtensionMethod(MethodNode node) {
node.setType(ErrorType.getInstance());
return;
}
node.setType(protoType.getFieldType(fieldName, errorReporter, node.getSourceLocation()));
node.setType(protoType.getFieldType(fieldName));
}

@Nullable
Expand Down Expand Up @@ -1436,8 +1436,7 @@ protected void visitProtoInitNode(ProtoInitNode node) {
expr.getSourceLocation(), PROTO_NULL_ARG_TYPE, fieldName.identifier());
}

SoyType fieldType =
protoType.getFieldType(fieldName.identifier(), errorReporter, fieldName.location());
SoyType fieldType = protoType.getFieldType(fieldName.identifier());

// Let args with unknown or error types pass
if (argType.equals(UnknownType.getInstance())
Expand Down Expand Up @@ -1597,7 +1596,7 @@ private SoyType getFieldType(
case PROTO:
{
SoyProtoType protoType = (SoyProtoType) baseType;
SoyType fieldType = protoType.getFieldType(fieldName, errorReporter, sourceLocation);
SoyType fieldType = protoType.getFieldType(fieldName);
if (fieldType != null) {
return fieldType;
} else {
Expand Down
43 changes: 6 additions & 37 deletions java/src/com/google/template/soy/types/SoyProtoType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@
import com.google.protobuf.Descriptors.Descriptor;
import com.google.protobuf.Descriptors.EnumDescriptor;
import com.google.protobuf.Descriptors.FieldDescriptor;
import com.google.template.soy.base.SourceLocation;
import com.google.template.soy.base.SoyBackendKind;
import com.google.template.soy.error.ErrorReporter;
import com.google.template.soy.internal.proto.Field;
import com.google.template.soy.internal.proto.FieldVisitor;
import com.google.template.soy.internal.proto.JavaQualifiedNames;
Expand Down Expand Up @@ -154,29 +152,20 @@ protected SoyType visitTrustedResourceUrl() {
}
}

private abstract static class FieldWithType extends Field {
FieldWithType(FieldDescriptor fieldDesc) {
super(fieldDesc);
}

abstract SoyType getType(ErrorReporter reporter, SourceLocation location);
}

private static final class NormalFieldWithType extends FieldWithType {
private static final class FieldWithType extends Field {
// NOTE: we lazily resolve types so we can handle recursive messages
@GuardedBy("this")
SoyType type;

@GuardedBy("this")
TypeVisitor visitor;

NormalFieldWithType(FieldDescriptor fieldDesc, TypeVisitor visitor) {
FieldWithType(FieldDescriptor fieldDesc, TypeVisitor visitor) {
super(fieldDesc);
this.visitor = visitor;
}

@Override
synchronized SoyType getType(ErrorReporter reporter, SourceLocation location) {
synchronized SoyType getType() {
if (type == null) {
type = FieldVisitor.visitField(getDescriptor(), visitor);
checkNotNull(type, "Couldn't find a type for: %s", getDescriptor());
Expand All @@ -186,21 +175,6 @@ synchronized SoyType getType(ErrorReporter reporter, SourceLocation location) {
}
}

private static final class AmbiguousFieldWithType extends FieldWithType {
final Set<FieldWithType> fields;

AmbiguousFieldWithType(Set<FieldWithType> fields) {
super(fields.iterator().next().getDescriptor());
this.fields = fields;
}

@Override
SoyType getType(ErrorReporter reporter, SourceLocation location) {
Field.reportAmbiguousFieldsError(reporter, location, getName(), fields);
return ErrorType.getInstance();
}
}

private final Descriptor typeDescriptor;
private final ImmutableMap<String, FieldWithType> fields;
private final ImmutableSet<String> extensionFieldNames;
Expand All @@ -218,12 +192,7 @@ public SoyProtoType(

@Override
public FieldWithType create(FieldDescriptor fieldDescriptor) {
return new NormalFieldWithType(fieldDescriptor, visitor);
}

@Override
public FieldWithType createAmbiguousFieldSet(Set<FieldWithType> fields) {
return new AmbiguousFieldWithType(fields);
return new FieldWithType(fieldDescriptor, visitor);
}
});
this.extensionFieldNames =
Expand Down Expand Up @@ -280,9 +249,9 @@ public FieldDescriptor getFieldDescriptor(String fieldName) {

/** Returns the {@link SoyType} of the given field, or null if the field does not exist. */
@Nullable
public SoyType getFieldType(String fieldName, ErrorReporter reporter, SourceLocation location) {
public SoyType getFieldType(String fieldName) {
FieldWithType field = fields.get(fieldName);
return field != null ? field.getType(reporter, location) : null;
return field != null ? field.getType() : null;
}

/** Returns all the field names of this proto. */
Expand Down

0 comments on commit 4ed9f43

Please sign in to comment.