search

jonahgraham / protobuf-dt

Automatically exported from code.google.com/p/protobuf-dt
0 stars 0 forks source link

Suggestion: Duplicate Enum Literal tag number should be a warning #120

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Create a proto file with an Enum
2. Create two tags with the same number
3.

What is the expected output? What do you see instead?

While this scenario is valid in protocol buffers and during decode the first 
Enum Literal with the provided tag will be used, I think more often than not 
this is a user error / unintended situation when creating proto files.

As such, I propose duplicate enum literal tags be marked with a warning 
explaining the consequences of this situation and offering a quick fix to 
increment the number to an unused tag.

Right now the only detractor for this is that we currently have no mechanism to 
suppress warnings... I still think it's a good thing to do though.

What version of the product are you using? On what operating system?
1.0.2

Please provide any additional information below.
If the proposal is accepted, the implementation should be some fairly minor 
changes in 
com.google.eclipse.protobuf.validation.ProtobufJavaValidator
(and possibly related classes)
and for a quick fix...
com.google.eclipse.protobuf.ui.quickfix.ProtobufQuickfixProvider

I'll also note here that no quick fixes are provided for duplicate tags in 
extensions/messages/groups - maybe this should also be added?

Original issue reported on code.google.com by compuwar...@gmail.com on 15 Sep 2011 at 3:59

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 12 Oct 2011 at 6:06

GoogleCodeExporter commented 9 years ago 
Addition:

Negative EnumValue tag numbers should be an error.

Original comment by compuwar...@gmail.com on 14 Oct 2011 at 5:49

GoogleCodeExporter commented 9 years ago 
Implemented fixes for both negative enum value tag numbers being an error and 
duplicate enum value tag numbers being a warning.

I refactored the error type name 
INVALID_FIELD_TAG_NUMBER_ERROR
to
INVALID_FIELD_OR_LITERAL_TAG_NUMBER_ERROR
for clarity.

This is why a change occurred in the test project.  I have not --yet-- 
implemented unit tests for these fixes.

The quick fix provider for field tag numbers was updated to handle enum 
literals as well - there doesn't seem to be a need to make a separate one.

Two new checks were added to the Validator class to check for these issues.

Modified Source:
/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages
.java
/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Protobuf
JavaValidator.java
/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/Messages
.properties
/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/Pro
tobufJavaValidator_checkTagNumberIsGreaterThanZero_Test.java
/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/Pro
tobufJavaValidator_checkTagNumberIsUnique_Test.java
/com.google.eclipse.protobuf.ui/src/com/google/eclipse/protobuf/ui/quickfix/Prot
obufQuickfixProvider.java

Attached a zip with -only- the changed files in the appropriate directory tree 
structure.

Original comment by compuwar...@gmail.com on 14 Oct 2011 at 6:19

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Implementing tests and improving messages a bit...

Is there a reason that the error message is different for negative and zero 
value tag numbers for fields?

The error for negative tag numbers is
"Field numbers must be positive integers."
while the error for zero tag numbers is
"Expected field number."

It seems the first error is more appropriate and descriptive for both cases.

Let me know and I'll make both Enum Literal and Field tag number errors 
consistent with whatever the intended message is.

Original comment by compuwar...@gmail.com on 14 Oct 2011 at 6:54

GoogleCodeExporter commented 9 years ago 
Sorry for the multiple posts... should have just figured out the tests to start 
with.

Tests have been added to verify added checks for Enum Value tag numbers.

Notably... i realized 0 is valid for an enum value tag, so my previous question 
only applies to whether or not those two errors make sense for Fields.

Original comment by compuwar...@gmail.com on 14 Oct 2011 at 8:03

Attachments:

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 18 Oct 2011 at 7:58

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 3 Nov 2011 at 1:06

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 13 Jan 2012 at 3:22

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 21 Oct 2012 at 9:13