search

tanqidong1992 / protobuf-dt

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

Extensions and complete enum literal support for custom options #111

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
A few issues & fixes are bundled together here as they are interrelated and are 
better solved together...

What steps will reproduce the problem?
1.  Custom Field options do not support various types of enum literals...

1)
Literals present in MessageNotation / FieldNotation form (results in missing 
reference error)

import "descriptor.proto";

extend google.protobuf.FieldOptions {
  optional MyCustomOptionType myoptionfield = 50000;
}
message MyCustomOptionType {
  optional MyEnum myoptionfield = 1;
}
enum MyEnum {
  MyEnumLiteral = 1;
}
message MyMessage {
 optional int32 foo = 1 [(myoption)= { myoptionfield: MyEnumLiteral } ];
}

"MyEnumLiteral" results in an error

2)
Option fields from other namespaces are not supported

import "descriptor.proto";

package firstfile;

extend google.protobuf.FieldOptions {
  optional MyCustomOptionType myoptionfield = 50000;
}
message MyCustomOptionType {
  optional MyEnum myoptionfield = 1;
  extensions 5 to 10;
}
enum MyEnum {
  MyEnumLiteral = 1;
}

...

import "ExtensionInNotationFirstFile.proto";

package secondfile;

extend firstfile.MyCustomOptionType {
  optional MyNewEnum myextensionfield = 5;
}
enum MyNewEnum {
  MyNewEnumLiteral = 1;
}
message MyMessage {
 optional int32 foo = 1 [(myoption)= { [secondfile.myextensionfield]: MyNewEnumLiteral } ];
}

"[secondfile.myextensionfield]" results in a syntax error (QualifiedName was 
not used for the FieldNotation element, no '[<name>]' was included for 
extensions)

A hidden issue here is also that Enum Literals are not being pulled from 
imported files for MessageNotation... that is also taken care of in the 
provided fix.

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

These valid proto files should not have any errors.

What version of the product are you using? On what operating system?
1.0.1
Windows 7 x64

Please provide any additional information below.

I've attached the three test files called out above.

I have also attaches an updated XText Grammar (only change is to FieldNotation) 
and an updated source file for 
com.google.eclipse.protobuf.scoping.ProtobufScopeProvider that correctly 
locates and uses Enum's for validation.

Notably, the solution provided in the Scope Provider may be an excellent start 
to complete MessageNotation validation as well as having actual references 
(hot-linked back to the correct field).

This solution does not update the UI Proposal Provider... I will have to take a 
look at that another day and update it to add content assist for Literals in 
MessageNotation.

Original issue reported on code.google.com by compuwar...@gmail.com on 13 Sep 2011 at 6:41

Attachments:

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 13 Sep 2011 at 10:22

GoogleCodeExporter commented 9 years ago

Original comment by alr...@google.com on 13 Sep 2011 at 10:22

GoogleCodeExporter commented 9 years ago 
Sorry the test files were no good, i had a typo in there, fixed in the new 
upload.

Original comment by compuwar...@gmail.com on 14 Sep 2011 at 10:59

Attachments:

GoogleCodeExporter commented 9 years ago 
Updated the proposal provider to be consistent with validating / scoping...

needed two added components...

  @Inject private ProtobufScopeProvider scopeProvider;

and

  @Override
  public void completeLiteralRef_Literal(EObject model, Assignment assignment, 
          ContentAssistContext context, ICompletionProposalAcceptor acceptor) {
    Enum enumType = scopeProvider.enumTypeOfOption(model);
    if (enumType != null) {
        proposeAndAccept(enumType, context, acceptor);
        return;
    }
  }

I also had to make ..."scopeProvider.enumTypeOfOption()" public to share access.

This definitely has the feel of a partial solution begging to be fully 
implemented for the rest of MessageNotation... I might take a shot at that when 
I have some free time unless someone else is already looking into solutions for 
that...

Original comment by compuwar...@gmail.com on 14 Sep 2011 at 11:05

Attachments:

GoogleCodeExporter commented 9 years ago 
Tacking on two more issues to this issue... if preferred I can make separate 
issues but I tend to be of the "related issues in one issue > issue spam" 
court...

Repeated fields result in a syntax error in MessageNotation

using a repeated field such as 

import "descriptor.proto";

extend google.protobuf.FieldOptions {
  optional MyCustomOptionType myoption = 50000;
}
message MyCustomOptionType {
  repeated int32 myrepeatedfield = 1;
}
message MyMessage {
 optional int32 foo = 1 [(myoption)= {
        myrepeatedfield: 12 ,
        myrepeatedfield: 24 ,
        myrepeatedfield: 48
 } ];

results in a syntax error due to unique name enforcement by XText

This is fixed simply by not using the special "name" variable in XText:
FieldNotation:
  (fieldName=QualifiedName | extension?='[' fieldName=QualifiedName ']') ':' value=SimpleRef;

Lack of commas between fields in MessageNotation result in a syntax error

Commas are not required between fields in message notation by protoc.  This 
should be reflected in protobuf-dt.

 optional int32 bar = 2 [(myoption)= {
        myrepeatedfield: 12
        myrepeatedfield: 24
        myrepeatedfield: 48
 } ];

 optional int32 badprogrammer = 3 [(myoption)= {
        myrepeatedfield: 12
        myrepeatedfield: 24 , //this is still 'valid'...
        myrepeatedfield: 48
 } ];

Simple fix is to make commas optional...

MessageNotation:
  '{' 
  fields+=FieldNotation (','? fields+=FieldNotation)*    
  '}' 
;

Original comment by compuwar...@gmail.com on 14 Sep 2011 at 11:39

Attachments:

GoogleCodeExporter commented 9 years ago 
Also, I updated to the 1.0.2 release - the changes had no effect on the problem 
nor solution.

Original comment by compuwar...@gmail.com on 14 Sep 2011 at 11:44

GoogleCodeExporter commented 9 years ago 
As an added note here, with these proposed changes implemented all valid 
message notation in custom options *should* be free of errors in the editor.  
That is not to say that all message notation will be properly validated - 
invalid notation could still appear valid (use int instead of string, etc).

Original comment by compuwar...@gmail.com on 14 Sep 2011 at 11:47

GoogleCodeExporter commented 9 years ago 
Correction to my previous note... nested message types in custom options are 
not supported by the grammar.  This issue is addressed separately in
https://code.google.com/p/protobuf-dt/issues/detail?id=121

Original comment by compuwar...@gmail.com on 15 Sep 2011 at 7:49

GoogleCodeExporter commented 9 years ago 
I believe this issue and solution are superseded by 
https://code.google.com/p/protobuf-dt/issues/detail?id=125
and should be closed / rejected if and when that addresses the issues raised 
here.

Original comment by compuwar...@gmail.com on 30 Sep 2011 at 6:11

GoogleCodeExporter commented 9 years ago 
I believe this issue and solution are superseded by 
https://code.google.com/p/protobuf-dt/issues/detail?id=125
and should be closed / rejected if and when that addresses the issues raised 
here.

Original comment by compuwar...@gmail.com on 30 Sep 2011 at 6:11

GoogleCodeExporter commented 9 years ago 
Postponing fix to next release.

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

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 
Fixed as Issue 155. I started working on full support of custom options based 
on bugs filed internally at Google, as I kept working on it I ended fixing this 
issue as well.

r3bc2f7ac4c7c, r96a713437062, r3216303da8e9, reca7b70fcd9a, r2f429441e726, 
r90b9a83fd09c, r12ee03ab3bfb, r5268fff7f929, rc209d861a5b7, rde0f3163a72d, 
r170e19933ee3, rb7bf6780440a, rfdb47648d534, r60501de66435

Original comment by alr...@google.com on 22 Nov 2011 at 1:18