Improvments to checking for conflicts among tag numbers and names:
1) Do not consider elements of a nested "extend" when checking for conflicts
with "reserved" or "extensions" declarations.
2) Do not report more than one conflict error per element.
3) Consider elements of groups when checking for field-number conflicts.
Change-Id: Iee85fd7b16ef79686984f1f20ee9b22053577dd6
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForIndexConflicts_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForIndexConflicts_Test.java
new file mode 100644
index 0000000..2732377
--- /dev/null
+++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForIndexConflicts_Test.java
@@ -0,0 +1,227 @@
+/*
+ * Copyright (c) 2015 Google Inc.
+ *
+ * All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse
+ * Public License v1.0 which accompanies this distribution, and is available at
+ *
+ * http://www.eclipse.org/legal/epl-v10.html
+ */
+package com.google.eclipse.protobuf.validation;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
+import static com.google.eclipse.protobuf.junit.core.UnitTestModule.unitTestModule;
+import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
+
+import java.util.List;
+
+import org.eclipse.emf.ecore.EObject;
+import org.eclipse.emf.ecore.EStructuralFeature;
+import org.eclipse.xtext.validation.ValidationMessageAcceptor;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import com.google.eclipse.protobuf.junit.core.XtextRule;
+import com.google.eclipse.protobuf.protobuf.Group;
+import com.google.eclipse.protobuf.protobuf.IndexRange;
+import com.google.eclipse.protobuf.protobuf.Message;
+import com.google.eclipse.protobuf.protobuf.MessageField;
+import com.google.eclipse.protobuf.protobuf.ProtobufPackage;
+import com.google.inject.Inject;
+
+public class ProtobufJavaValidator_checkForIndexConflicts_Test {
+ @Rule public XtextRule xtext = overrideRuntimeModuleWith(unitTestModule());
+
+ @Inject private ProtobufJavaValidator validator;
+ private ValidationMessageAcceptor messageAcceptor;
+
+ @Before public void setUp() {
+ messageAcceptor = mock(ValidationMessageAcceptor.class);
+ validator.setMessageAcceptor(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Person {
+ // reserved 10, 20 to 30;
+ // reserved 10;
+ // reserved 20;
+ // reserved 25 to 26;
+ // reserved 30 to 31;
+ // }
+ @Test public void should_error_on_conflict_between_reserved_and_reserved() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ List<IndexRange> indexRanges = xtext.findAll(IndexRange.class);
+ verifyError("Tag number 10 conflicts with reserved 10.", indexRanges.get(2));
+ verifyError("Tag number 20 conflicts with reserved 20 to 30.", indexRanges.get(3));
+ verifyError("Tag number range 25 to 26 conflicts with reserved 20 to 30.", indexRanges.get(4));
+ verifyError("Tag number range 30 to 31 conflicts with reserved 20 to 30.", indexRanges.get(5));
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Person {
+ // reserved 10, 20 to 30;
+ // extensions 10, 15 to max;
+ // }
+ @Test public void should_error_on_conflict_between_reserved_and_extensions() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ List<IndexRange> indexRanges = xtext.findAll(IndexRange.class);
+ verifyError("Tag number 10 conflicts with reserved 10.", indexRanges.get(2));
+ verifyError("Tag number range 15 to max conflicts with reserved 20 to 30.", indexRanges.get(3));
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Person {
+ // reserved 10, 20 to 30;
+ // optional bool foo = 10;
+ // optional bool bar = 25;
+ // group baz = 26 {
+ // optional bool a = 27;
+ // }
+ // }
+ @Test public void should_error_on_conflict_between_reserved_and_indexed_element() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ verifyError(
+ "Tag number 10 conflicts with reserved 10.",
+ xtext.findAll(MessageField.class).get(0),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 25 conflicts with reserved 20 to 30.",
+ xtext.findAll(MessageField.class).get(1),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 26 conflicts with reserved 20 to 30.",
+ xtext.findAll(Group.class).get(0),
+ ProtobufPackage.Literals.GROUP__INDEX);
+ verifyError(
+ "Tag number 27 conflicts with reserved 20 to 30.",
+ xtext.findAll(MessageField.class).get(2),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Person {
+ // extensions 10, 20 to 30;
+ // optional bool foo = 10;
+ // optional bool bar = 25;
+ // group baz = 26 {
+ // optional bool a = 27;
+ // }
+ // }
+ @Test public void should_error_on_conflict_between_extensions_and_indexed_element() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ verifyError(
+ "Tag number 10 conflicts with extensions 10.",
+ xtext.findAll(MessageField.class).get(0),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 25 conflicts with extensions 20 to 30.",
+ xtext.findAll(MessageField.class).get(1),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 26 conflicts with extensions 20 to 30.",
+ xtext.findAll(Group.class).get(0),
+ ProtobufPackage.Literals.GROUP__INDEX);
+ verifyError(
+ "Tag number 27 conflicts with extensions 20 to 30.",
+ xtext.findAll(MessageField.class).get(2),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Person {
+ // optional bool foo1 = 1;
+ // optional bool foo2 = 1;
+ // repeated group foo3 = 1 {
+ // optional bool foo4 = 1;
+ // }
+ // oneof choose_one {
+ // optional bool foo5 = 1;
+ // }
+ //
+ // repeated group foo6 = 2 {
+ // optional bool foo7 = 3;
+ // }
+ // optional bool foo8 = 2;
+ // }
+ @Test public void should_error_on_conflict_between_indexed_elements() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ verifyError(
+ "Tag number 1 conflicts with field \"foo1\".",
+ xtext.findAll(MessageField.class).get(1),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 1 conflicts with field \"foo1\".",
+ xtext.findAll(Group.class).get(0),
+ ProtobufPackage.Literals.GROUP__INDEX);
+ verifyError(
+ "Tag number 1 conflicts with field \"foo1\".",
+ xtext.findAll(MessageField.class).get(2),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 1 conflicts with field \"foo1\".",
+ xtext.findAll(MessageField.class).get(3),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyError(
+ "Tag number 2 conflicts with group \"foo6\".",
+ xtext.findAll(MessageField.class).get(5),
+ ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Car {
+ // }
+ //
+ // message Person {
+ // extensions 10 to 20;
+ // reserved 30 to 40;
+ // optional bool foo = 50;
+ // extend Car {
+ // optional Person passenger_1 = 15;
+ // optional Person passenger_2 = 35;
+ // optional Person passenger_3 = 50;
+ // }
+ // }
+ @Test public void should_not_find_conflicts_with_nested_type_extension_contents() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ verifyZeroInteractions(messageAcceptor);
+ }
+
+ // syntax = "proto2";
+ //
+ // message Person {
+ // extensions 10 to 20;
+ // reserved 30 to 40;
+ // optional bool foo = 50;
+ // message Car {
+ // optional Person passenger_1 = 15;
+ // optional Person passenger_2 = 35;
+ // optional Person passenger_3 = 50;
+ // }
+ // }
+ @Test public void should_not_find_conflicts_with_nested_message_contents() {
+ validator.checkForIndexConflicts(xtext.findFirst(Message.class));
+ verifyZeroInteractions(messageAcceptor);
+ }
+
+ private void verifyError(String message, EObject errorSource) {
+ verifyError(message, errorSource, null);
+ }
+
+ private void verifyError(String message, EObject errorSource, EStructuralFeature errorFeature) {
+ verify(messageAcceptor).acceptError(message, errorSource, errorFeature, -1, null);
+ }
+}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForIndexRangeConflicts_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForIndexRangeConflicts_Test.java
deleted file mode 100644
index a29c8c3..0000000
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForIndexRangeConflicts_Test.java
+++ /dev/null
@@ -1,142 +0,0 @@
-/*
- * Copyright (c) 2015 Google Inc.
- *
- * All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse
- * Public License v1.0 which accompanies this distribution, and is available at
- *
- * http://www.eclipse.org/legal/epl-v10.html
- */
-package com.google.eclipse.protobuf.validation;
-
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static com.google.eclipse.protobuf.junit.core.UnitTestModule.unitTestModule;
-import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
-
-import java.util.List;
-
-import org.eclipse.emf.ecore.EObject;
-import org.eclipse.emf.ecore.EStructuralFeature;
-import org.eclipse.xtext.validation.ValidationMessageAcceptor;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-
-import com.google.eclipse.protobuf.junit.core.XtextRule;
-import com.google.eclipse.protobuf.protobuf.Group;
-import com.google.eclipse.protobuf.protobuf.IndexRange;
-import com.google.eclipse.protobuf.protobuf.Message;
-import com.google.eclipse.protobuf.protobuf.MessageField;
-import com.google.eclipse.protobuf.protobuf.ProtobufPackage;
-import com.google.inject.Inject;
-
-public class ProtobufJavaValidator_checkForIndexRangeConflicts_Test {
- @Rule public XtextRule xtext = overrideRuntimeModuleWith(unitTestModule());
-
- @Inject private ProtobufJavaValidator validator;
- private ValidationMessageAcceptor messageAcceptor;
-
- @Before public void setUp() {
- messageAcceptor = mock(ValidationMessageAcceptor.class);
- validator.setMessageAcceptor(messageAcceptor);
- }
-
- // syntax = "proto2";
- //
- // message Person {
- // reserved 10, 20 to 30;
- // reserved 10;
- // reserved 20;
- // reserved 25 to 26;
- // reserved 30 to 31;
- // }
- @Test public void should_error_on_conflict_between_reserved_and_reserved() {
- validator.checkForIndexRangeConflicts(xtext.findFirst(Message.class));
- List<IndexRange> indexRanges = xtext.findAll(IndexRange.class);
- verifyError("10 conflicts with reserved 10", indexRanges.get(2));
- verifyError("20 conflicts with reserved 20 to 30", indexRanges.get(3));
- verifyError("25 to 26 conflicts with reserved 20 to 30", indexRanges.get(4));
- verifyError("30 to 31 conflicts with reserved 20 to 30", indexRanges.get(5));
- }
-
- // syntax = "proto2";
- //
- // message Person {
- // reserved 10, 20 to 30;
- // extensions 10, 15 to max;
- // }
- @Test public void should_error_on_conflict_between_reserved_and_extensions() {
- validator.checkForIndexRangeConflicts(xtext.findFirst(Message.class));
- List<IndexRange> indexRanges = xtext.findAll(IndexRange.class);
- verifyError("10 conflicts with reserved 10", indexRanges.get(2));
- verifyError("15 to max conflicts with reserved 20 to 30", indexRanges.get(3));
- }
-
- // syntax = "proto2";
- //
- // message Person {
- // reserved 10, 20 to 30;
- // optional bool foo = 10;
- // optional bool bar = 25;
- // group baz = 26 {
- // optional bool a = 27;
- // }
- // }
- @Test public void should_error_on_conflict_between_reserved_and_indexed_element() {
- validator.checkForIndexRangeConflicts(xtext.findFirst(Message.class));
- verifyError(
- "10 conflicts with reserved 10",
- xtext.findAll(MessageField.class).get(0),
- ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
- verifyError(
- "25 conflicts with reserved 20 to 30",
- xtext.findAll(MessageField.class).get(1),
- ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
- verifyError(
- "26 conflicts with reserved 20 to 30",
- xtext.findAll(Group.class).get(0),
- ProtobufPackage.Literals.GROUP__INDEX);
- verifyError(
- "27 conflicts with reserved 20 to 30",
- xtext.findAll(MessageField.class).get(2),
- ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
- }
-
- // syntax = "proto2";
- //
- // message Person {
- // extensions 10, 20 to 30;
- // optional bool foo = 10;
- // optional bool bar = 25;
- // group baz = 26 {
- // optional bool a = 27;
- // }
- // }
- @Test public void should_error_on_conflict_between_extensions_and_indexed_element() {
- validator.checkForIndexRangeConflicts(xtext.findFirst(Message.class));
- verifyError(
- "10 conflicts with extensions 10",
- xtext.findAll(MessageField.class).get(0),
- ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
- verifyError(
- "25 conflicts with extensions 20 to 30",
- xtext.findAll(MessageField.class).get(1),
- ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
- verifyError(
- "26 conflicts with extensions 20 to 30",
- xtext.findAll(Group.class).get(0),
- ProtobufPackage.Literals.GROUP__INDEX);
- verifyError(
- "27 conflicts with extensions 20 to 30",
- xtext.findAll(MessageField.class).get(2),
- ProtobufPackage.Literals.MESSAGE_FIELD__INDEX);
- }
-
- private void verifyError(String message, EObject errorSource) {
- verifyError(message, errorSource, null);
- }
-
- private void verifyError(String message, EObject errorSource, EStructuralFeature errorFeature) {
- verify(messageAcceptor).acceptError(message, errorSource, errorFeature, -1, null);
- }
-}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForReservedNameConflicts_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForReservedNameConflicts_Test.java
index 307d127..074c5ea 100644
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForReservedNameConflicts_Test.java
+++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkForReservedNameConflicts_Test.java
@@ -10,6 +10,7 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyZeroInteractions;
import static com.google.eclipse.protobuf.junit.core.UnitTestModule.unitTestModule;
import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
@@ -50,8 +51,8 @@
@Test public void should_error_on_conflict_between_reserved_and_reserved() {
validator.checkForReservedNameConflicts(xtext.findFirst(Message.class));
List<StringLiteral> stringLiterals = xtext.findAll(StringLiteral.class);
- verifyError("\"foo\" conflicts with reserved \"foo\"", stringLiterals.get(3));
- verifyError("\"bar\" conflicts with reserved \"bar\"", stringLiterals.get(4));
+ verifyError("Name \"foo\" conflicts with reserved \"foo\".", stringLiterals.get(3));
+ verifyError("Name \"bar\" conflicts with reserved \"bar\".", stringLiterals.get(4));
}
// syntax = "proto2";
@@ -66,19 +67,37 @@
@Test public void should_error_on_conflict_between_reserved_and_indexed_element() {
validator.checkForReservedNameConflicts(xtext.findFirst(Message.class));
verifyError(
- "\"foo\" conflicts with reserved \"foo\"",
+ "Name \"foo\" conflicts with reserved \"foo\".",
xtext.findAll(MessageField.class).get(0),
ProtobufPackage.Literals.MESSAGE_FIELD__NAME);
verifyError(
- "\"bar\" conflicts with reserved \"bar\"",
+ "Name \"bar\" conflicts with reserved \"bar\".",
xtext.findAll(Group.class).get(0),
ProtobufPackage.Literals.COMPLEX_TYPE__NAME);
verifyError(
- "\"baz\" conflicts with reserved \"baz\"",
+ "Name \"baz\" conflicts with reserved \"baz\".",
xtext.findAll(MessageField.class).get(1),
ProtobufPackage.Literals.MESSAGE_FIELD__NAME);
}
+ // syntax = "proto2";
+ //
+ // message Car {
+ // }
+ //
+ // message Person {
+ // reserved "owner";
+ // optional bool passenger = 1;
+ // extend Car {
+ // optional Person owner = 1;
+ // repeated Person passenger = 2;
+ // }
+ // }
+ @Test public void should_not_find_conflict_with_nested_extension() {
+ validator.checkForReservedNameConflicts(xtext.findFirst(Message.class));
+ verifyZeroInteractions(messageAcceptor);
+ }
+
private void verifyError(String message, EObject errorSource) {
verifyError(message, errorSource, null);
}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkTagNumberIsUnique_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkTagNumberIsUnique_Test.java
deleted file mode 100644
index c15c4e0..0000000
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkTagNumberIsUnique_Test.java
+++ /dev/null
@@ -1,71 +0,0 @@
-/*
- * Copyright (c) 2011 Google Inc.
- *
- * All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse
- * Public License v1.0 which accompanies this distribution, and is available at
- *
- * http://www.eclipse.org/legal/epl-v10.html
- */
-package com.google.eclipse.protobuf.validation;
-
-import static org.eclipse.xtext.validation.ValidationMessageAcceptor.INSIGNIFICANT_INDEX;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyZeroInteractions;
-
-import static com.google.eclipse.protobuf.junit.core.UnitTestModule.unitTestModule;
-import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
-import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.MESSAGE_FIELD__INDEX;
-import static com.google.eclipse.protobuf.validation.ProtobufJavaValidator.INVALID_FIELD_TAG_NUMBER_ERROR;
-
-import org.eclipse.xtext.validation.ValidationMessageAcceptor;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-
-import com.google.eclipse.protobuf.junit.core.XtextRule;
-import com.google.eclipse.protobuf.protobuf.IndexedElement;
-import com.google.eclipse.protobuf.protobuf.MessageField;
-import com.google.inject.Inject;
-
-/**
- * Tests for <code>{@link ProtobufJavaValidator#checkTagNumberIsUnique(IndexedElement)}</code>
- *
- * @author alruiz@google.com (Alex Ruiz)
- */
-public class ProtobufJavaValidator_checkTagNumberIsUnique_Test {
- @Rule public XtextRule xtext = overrideRuntimeModuleWith(unitTestModule());
-
- @Inject private ProtobufJavaValidator validator;
- private ValidationMessageAcceptor messageAcceptor;
-
- @Before public void setUp() {
- messageAcceptor = mock(ValidationMessageAcceptor.class);
- validator.setMessageAcceptor(messageAcceptor);
- }
-
- // syntax = "proto2";
- //
- // message Person {
- // optional long id = 1;
- // optional string name = 1;
- // }
- @Test public void should_create_error_if_field_does_not_have_unique_tag_number() {
- MessageField field = xtext.find("name", MessageField.class);
- validator.checkTagNumberIsUnique(field);
- String message = "Field number 1 has already been used in \"Person\" by field \"id\".";
- verify(messageAcceptor).acceptError(message, field, MESSAGE_FIELD__INDEX, INSIGNIFICANT_INDEX, INVALID_FIELD_TAG_NUMBER_ERROR);
- }
-
- // syntax = "proto2";
- //
- // message Person {
- // optional long id = 1;
- // optional string name = 2;
- // }
- @Test public void should_not_create_error_if_field_has_unique_tag_number() {
- MessageField field = xtext.find("name", MessageField.class);
- validator.checkTagNumberIsUnique(field);
- verifyZeroInteractions(messageAcceptor);
- }
-}
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.java
index a54836c..85c2f9d 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.java
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.java
@@ -14,8 +14,11 @@
* @author alruiz@google.com (Alex Ruiz)
*/
public class Messages extends NLS {
- public static String conflictsWithExtensions;
- public static String conflictsWithReserved;
+ public static String conflictingExtensions;
+ public static String conflictingField;
+ public static String conflictingGroup;
+ public static String conflictingReservedName;
+ public static String conflictingReservedNumber;
public static String expectedFieldName;
public static String expectedFieldNumber;
public static String expectedIdentifier;
@@ -25,7 +28,6 @@
public static String expectedString;
public static String expectedSyntaxIdentifier;
public static String expectedTrueOrFalse;
- public static String fieldNumberAlreadyUsed;
public static String fieldNumbersMustBePositive;
public static String importingUnsupportedSyntax;
public static String importNotFound;
@@ -37,11 +39,14 @@
public static String missingFieldNumber;
public static String missingModifier;
public static String multiplePackages;
+ public static String nameConflict;
public static String oneofFieldWithModifier;
public static String requiredInProto3;
public static String reservedIndexAndName;
public static String reservedToMax;
public static String scopingError;
+ public static String tagNumberRangeConflict;
+ public static String tagNumberConflict;
public static String unknownSyntax;
public static String unrecognizedSyntaxIdentifier;
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.properties b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.properties
index 7f46818..15aaf6a 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.properties
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages.properties
@@ -1,3 +1,8 @@
+conflictingExtensions = extensions %s
+conflictingField = field "%s"
+conflictingGroup = group "%s"
+conflictingReservedName = reserved "%s"
+conflictingReservedNumber = reserved %s
expectedFieldName = Expected field name.
expectedFieldNumber = Expected field number.
expectedIdentifier = Expected identifier.
@@ -7,7 +12,6 @@
expectedString = Expected string.
expectedSyntaxIdentifier = Expected syntax identifier.
expectedTrueOrFalse = Expected "true" or "false".
-fieldNumberAlreadyUsed = Field number %d has already been used in \"%s\" by field \"%s\".
fieldNumbersMustBePositive = Field numbers must be positive integers.
importingUnsupportedSyntax = Importing unsupported file (directly or indirectly.) This may cause errors related to unresolved references.
importNotFound = Import \"%s\" was not found.
@@ -20,11 +24,12 @@
missingModifier = Missing modifier: \"required\", \"optional\", or \"repeated\".
oneofFieldWithModifier = Fields inside "oneof" cannot have a modifier.
multiplePackages = Multiple package definitions.
-conflictsWithExtensions = %s conflicts with extensions %s
-conflictsWithReserved = %s conflicts with reserved %s
+nameConflict = Name "%s" conflicts with %s.
requiredInProto3 = Required fields are not allowed in proto3.
reservedIndexAndName = A reserved declaration may not include both numbers and names.
reservedToMax = Reserved index range must have finite upper bound.
scopingError = It may be caused by an imported non-proto2 file.
+tagNumberConflict = Tag number %s conflicts with %s.
+tagNumberRangeConflict = Tag number range %s conflicts with %s.
unknownSyntax = Unknown syntax. This parser only recognizes \"proto2\" or \"proto3\".
unrecognizedSyntaxIdentifier = Unrecognized syntax identifier \"%s\". This parser only recognizes \"proto2\" and \"proto3\".
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator.java
index 00b0350..d37b92d 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator.java
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator.java
@@ -13,11 +13,8 @@
import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.MESSAGE_FIELD__MODIFIER;
import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.PACKAGE__NAME;
import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.SYNTAX__NAME;
-import static com.google.eclipse.protobuf.validation.Messages.conflictsWithExtensions;
-import static com.google.eclipse.protobuf.validation.Messages.conflictsWithReserved;
import static com.google.eclipse.protobuf.validation.Messages.expectedFieldNumber;
import static com.google.eclipse.protobuf.validation.Messages.expectedSyntaxIdentifier;
-import static com.google.eclipse.protobuf.validation.Messages.fieldNumberAlreadyUsed;
import static com.google.eclipse.protobuf.validation.Messages.fieldNumbersMustBePositive;
import static com.google.eclipse.protobuf.validation.Messages.invalidMapKeyType;
import static com.google.eclipse.protobuf.validation.Messages.invalidMapValueType;
@@ -25,10 +22,18 @@
import static com.google.eclipse.protobuf.validation.Messages.mapWithinTypeExtension;
import static com.google.eclipse.protobuf.validation.Messages.missingModifier;
import static com.google.eclipse.protobuf.validation.Messages.multiplePackages;
+import static com.google.eclipse.protobuf.validation.Messages.nameConflict;
import static com.google.eclipse.protobuf.validation.Messages.oneofFieldWithModifier;
import static com.google.eclipse.protobuf.validation.Messages.requiredInProto3;
import static com.google.eclipse.protobuf.validation.Messages.reservedIndexAndName;
import static com.google.eclipse.protobuf.validation.Messages.reservedToMax;
+import static com.google.eclipse.protobuf.validation.Messages.tagNumberRangeConflict;
+import static com.google.eclipse.protobuf.validation.Messages.tagNumberConflict;
+import static com.google.eclipse.protobuf.validation.Messages.conflictingExtensions;
+import static com.google.eclipse.protobuf.validation.Messages.conflictingField;
+import static com.google.eclipse.protobuf.validation.Messages.conflictingGroup;
+import static com.google.eclipse.protobuf.validation.Messages.conflictingReservedName;
+import static com.google.eclipse.protobuf.validation.Messages.conflictingReservedNumber;
import static com.google.eclipse.protobuf.validation.Messages.unknownSyntax;
import static com.google.eclipse.protobuf.validation.Messages.unrecognizedSyntaxIdentifier;
import static java.lang.String.format;
@@ -36,10 +41,12 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
-import java.util.List;
+import java.util.Map;
import java.util.Set;
import com.google.common.collect.Iterables;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.Multimap;
import com.google.common.collect.Range;
import com.google.eclipse.protobuf.model.util.IndexRanges;
import com.google.eclipse.protobuf.model.util.IndexedElements;
@@ -48,12 +55,12 @@
import com.google.eclipse.protobuf.model.util.Syntaxes;
import com.google.eclipse.protobuf.naming.NameResolver;
import com.google.eclipse.protobuf.protobuf.Extensions;
+import com.google.eclipse.protobuf.protobuf.Group;
import com.google.eclipse.protobuf.protobuf.IndexRange;
import com.google.eclipse.protobuf.protobuf.IndexedElement;
import com.google.eclipse.protobuf.protobuf.MapType;
import com.google.eclipse.protobuf.protobuf.MapTypeLink;
import com.google.eclipse.protobuf.protobuf.Message;
-import com.google.eclipse.protobuf.protobuf.MessageElement;
import com.google.eclipse.protobuf.protobuf.MessageField;
import com.google.eclipse.protobuf.protobuf.ModifierEnum;
import com.google.eclipse.protobuf.protobuf.OneOf;
@@ -71,12 +78,11 @@
import com.google.eclipse.protobuf.protobuf.TypeLink;
import com.google.inject.Inject;
+import org.eclipse.emf.common.util.TreeIterator;
import org.eclipse.emf.ecore.EAttribute;
import org.eclipse.emf.ecore.EObject;
import org.eclipse.emf.ecore.EStructuralFeature;
import org.eclipse.xtext.EcoreUtil2;
-import org.eclipse.xtext.naming.IQualifiedNameProvider;
-import org.eclipse.xtext.naming.QualifiedName;
import org.eclipse.xtext.util.SimpleAttributeResolver;
import org.eclipse.xtext.validation.Check;
import org.eclipse.xtext.validation.ComposedChecks;
@@ -101,7 +107,6 @@
@Inject private NameResolver nameResolver;
@Inject private StringLiterals stringLiterals;
@Inject private Protobufs protobufs;
- @Inject private IQualifiedNameProvider qualifiedNameProvider;
@Inject private Syntaxes syntaxes;
@Check public void checkIsKnownSyntax(Protobuf protobuf) {
@@ -119,46 +124,67 @@
error(msg, syntax, SYNTAX__NAME, SYNTAX_IS_NOT_KNOWN_ERROR);
}
- @Check public void checkForIndexRangeConflicts(Message message) {
- Collection<Range<Long>> reservedRanges = new ArrayList<>();
- for (Reserved reserved : getOwnedElements(Message.class, message, Reserved.class)) {
+ @Check public void checkForIndexConflicts(Message message) {
+ Multimap<EObject, Range<Long>> rangeUsages = LinkedHashMultimap.create();
+
+ for (Reserved reserved : getOwnedElements(message, Reserved.class)) {
for (IndexRange indexRange : Iterables.filter(reserved.getReservations(), IndexRange.class)) {
Range<Long> range = indexRanges.toLongRange(indexRange);
- errorOnConflicts(range, reservedRanges, conflictsWithReserved, indexRange, null);
- reservedRanges.add(range);
+ errorOnConflicts(range, rangeUsages, indexRange, null);
+ rangeUsages.put(reserved, range);
}
}
- Collection<Range<Long>> extensionsRanges = new ArrayList<>();
- for (Extensions extensions : getOwnedElements(Message.class, message, Extensions.class)) {
+ for (Extensions extensions : getOwnedElements(message, Extensions.class)) {
for (IndexRange indexRange : extensions.getRanges()) {
Range<Long> range = indexRanges.toLongRange(indexRange);
- errorOnConflicts(range, reservedRanges, conflictsWithReserved, indexRange, null);
- errorOnConflicts(range, extensionsRanges, conflictsWithExtensions, indexRange, null);
- extensionsRanges.add(range);
+ errorOnConflicts(range, rangeUsages, indexRange, null);
+ rangeUsages.put(extensions, range);
}
}
- for (IndexedElement element : getOwnedElements(Message.class, message, IndexedElement.class)) {
+ for (IndexedElement element : getOwnedElements(message, IndexedElement.class)) {
long index = indexedElements.indexOf(element);
Range<Long> range = Range.singleton(index);
- EStructuralFeature indexFeature = indexedElements.indexFeatureOf(element);
- errorOnConflicts(range, reservedRanges, conflictsWithReserved, element, indexFeature);
- errorOnConflicts(range, extensionsRanges, conflictsWithExtensions, element, indexFeature);
+ EStructuralFeature feature = indexedElements.indexFeatureOf(element);
+ errorOnConflicts(range, rangeUsages, element, feature);
+ rangeUsages.put(element, range);
}
}
private void errorOnConflicts(
Range<Long> range,
- Iterable<Range<Long>> existingRanges,
- String errorTemplate,
+ Multimap<EObject, Range<Long>> rangeUsages,
EObject errorSource,
EStructuralFeature errorFeature) {
- for (Range<Long> existingRange : existingRanges) {
- if (range.isConnected(existingRange)) {
- String message =
- String.format(errorTemplate, rangeToString(range), rangeToString(existingRange));
+ for (Map.Entry<EObject, Range<Long>> rangeUsage : rangeUsages.entries()) {
+ Range<Long> usedRange = rangeUsage.getValue();
+ if (range.isConnected(usedRange)) {
+ EObject rangeUser = rangeUsage.getKey();
+
+ boolean rangeIsSingular =
+ range.hasUpperBound() && range.upperEndpoint() == range.lowerEndpoint();
+ String template = rangeIsSingular ? tagNumberConflict : tagNumberRangeConflict;
+
+ String rangeUserString;
+ String usedRangeString = rangeToString(usedRange);
+ if (rangeUser instanceof MessageField) {
+ rangeUserString =
+ String.format(conflictingField, nameResolver.nameOf(rangeUser), usedRangeString);
+ } else if (rangeUser instanceof Group) {
+ rangeUserString =
+ String.format(conflictingGroup, nameResolver.nameOf(rangeUser), usedRangeString);
+ } else if (rangeUser instanceof Reserved) {
+ rangeUserString = String.format(conflictingReservedNumber, usedRangeString);
+ } else {
+ rangeUserString = String.format(conflictingExtensions, usedRangeString);
+ }
+
+ String message = String.format(template, rangeToString(range), rangeUserString);
error(message, errorSource, errorFeature);
+
+ // Don't report more than one error per element.
+ return;
}
}
}
@@ -185,7 +211,7 @@
@Check public void checkForReservedNameConflicts(Message message) {
Set<String> reservedNames = new HashSet<>();
- for (Reserved reserved : getOwnedElements(Message.class, message, Reserved.class)) {
+ for (Reserved reserved : getOwnedElements(message, Reserved.class)) {
for (StringLiteral stringLiteral :
Iterables.filter(reserved.getReservations(), StringLiteral.class)) {
String name = stringLiterals.getCombinedString(stringLiteral);
@@ -194,7 +220,7 @@
}
}
- for (IndexedElement element : getOwnedElements(Message.class, message, IndexedElement.class)) {
+ for (IndexedElement element : getOwnedElements(message, IndexedElement.class)) {
String name = nameResolver.nameOf(element);
if (name != null) {
EAttribute nameAttribute = SimpleAttributeResolver.NAME_RESOLVER.getAttribute(element);
@@ -222,38 +248,36 @@
private void reportReservedNameConflicts(
String name, Set<String> reservedNames, EObject errorSource, EAttribute errorFeature) {
if (reservedNames.contains(name)) {
- String quotedName = '"' + name + '"';
- String message = String.format(conflictsWithReserved, quotedName, quotedName);
+ String nameUser = String.format(conflictingReservedName, name);
+ String message = String.format(nameConflict, name, nameUser);
error(message, errorSource, errorFeature);
}
}
- private <E extends EObject, C extends EObject> Collection<E> getOwnedElements(
- Class<C> containerType, C container, Class<E> elementType) {
- List<E> allElements = EcoreUtil2.getAllContentsOfType(container, elementType);
- List<E> ownedElements = new ArrayList<>(allElements.size());
- for (E element : allElements) {
- if (EcoreUtil2.getContainerOfType(element, containerType) == container) {
- ownedElements.add(element);
+ /**
+ * Returns elements of the given type contained within the given message, except those contained
+ * within intervening Messages or TypeExtensions.
+ */
+ private static <E extends EObject> Collection<E> getOwnedElements(
+ Message container, Class<E> elementType) {
+ Collection<E> elements = new ArrayList<E>();
+
+ TreeIterator<EObject> elementsIterator = container.eAllContents();
+ while (elementsIterator.hasNext()) {
+ EObject element = elementsIterator.next();
+
+ if (elementType.isAssignableFrom(element.getClass())) {
+ @SuppressWarnings("unchecked")
+ E elementAsElementType = (E) element;
+ elements.add(elementAsElementType);
+ }
+
+ if (element instanceof Message || element instanceof TypeExtension) {
+ elementsIterator.prune();
}
}
- return ownedElements;
- }
- @Check public void checkTagNumberIsUnique(IndexedElement e) {
- if (isNameNull(e)) {
- return; // we already show an error if name is null, no need to go further.
- }
-
- EObject container = e.eContainer();
- if (container instanceof OneOf) {
- container = container.eContainer();
- }
- if (container instanceof Message) {
- Message message = (Message) container;
- Iterable<MessageElement> elements = message.getElements();
- checkTagNumberIsUnique(e, message, elements);
- }
+ return elements;
}
@Check public void checkFieldModifiers(MessageField field) {
@@ -308,33 +332,6 @@
return false;
}
- private boolean checkTagNumberIsUnique(IndexedElement e, EObject message,
- Iterable<MessageElement> elements) {
- long index = indexedElements.indexOf(e);
-
- for (MessageElement element : elements) {
- if (element instanceof OneOf) {
- if (!checkTagNumberIsUnique(e, message, ((OneOf) element).getElements())) {
- return false;
- }
- } else if (element instanceof IndexedElement) {
- IndexedElement other = (IndexedElement) element;
- if (other == e) {
- return true;
- }
- if (indexedElements.indexOf(other) == index) {
- QualifiedName messageName = qualifiedNameProvider.getFullyQualifiedName(message);
- String msg = format(fieldNumberAlreadyUsed, index, messageName.toString(),
- nameResolver.nameOf(other));
- invalidTagNumberError(msg, e);
- return false;
- }
- }
- }
-
- return true;
- }
-
@Check public void checkTagNumberIsGreaterThanZero(IndexedElement e) {
if (isNameNull(e))
{
