Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PBJ Compiler fails on comment preceding reserved field or syntax keyword #217

Closed
jsync-swirlds opened this issue Mar 4, 2024 · 1 comment · Fixed by #347
Closed

PBJ Compiler fails on comment preceding reserved field or syntax keyword #217

jsync-swirlds opened this issue Mar 4, 2024 · 1 comment · Fixed by #347
Assignees

Comments

@jsync-swirlds
Copy link
Member

Placing a field comment before a reserved field causes PBJ to fail.

To reproduce:
Add a reserved field with a field comment in any .proto file:

    /**
     * <h2>Removed Field.</h2>
     */
    reserved 8;

The PBJ compiler fails with an error (example from contract_types.proto):

line 142:13 mismatched input '8' expecting {'syntax', 'import', 'weak', 'public', 'package', 'option', 'repeated', 'oneof', 'map', 'int32', 'int64', 'uint32', 'uint64', 'sint32', 'sint64', 'fixed32', 'fixed64', 'sfixed32', 'sfixed64', 'bool', 'string', 'double', 'float', 'bytes', 'reserved', 'to', 'max', 'enum', 'message', 'service', 'rpc', 'stream', 'returns', BOOL_LIT, IDENTIFIER}
line 142:13 mismatched input '8' expecting {'syntax', 'import', 'weak', 'public', 'package', 'option', 'repeated', 'oneof', 'map', 'int32', 'int64', 'uint32', 'uint64', 'sint32', 'sint64', 'fixed32', 'fixed64', 'sfixed32', 'sfixed64', 'bool', 'string', 'double', 'float', 'bytes', 'reserved', 'to', 'max', 'enum', 'message', 'service', 'rpc', 'stream', 'returns', BOOL_LIT, IDENTIFIER}
Exception while processing file: /Users/user/Projects/Hashgraph/Pbj/pbj-integration-tests/build/repos/hapi/services/contract_types.proto
com.hedera.pbj.compiler.impl.PbjCompilerException: Failed to find fully qualified message type for [reserved] in file [/Users/user/Projects/Hashgraph/Pbj/pbj-integration-tests/build/repos/hapi/services/contract_types.proto] imports = [/Users/user/Projects/Hashgraph/Pbj/pbj-integration-tests/build/repos/hapi/services/basic_types.proto]
    com.hedera.pbj.compiler.impl.LookupHelper.getFullyQualifiedProtoName(LookupHelper.java:187)
    com.hedera.pbj.compiler.impl.LookupHelper.isEnum(LookupHelper.java:358)
    com.hedera.pbj.compiler.impl.ContextualLookupHelper.isEnum(ContextualLookupHelper.java:112)
    com.hedera.pbj.compiler.impl.Field$FieldType.of(Field.java:304)
    com.hedera.pbj.compiler.impl.SingleField.<init>(SingleField.java:33)
    com.hedera.pbj.compiler.impl.generators.ModelGenerator.generateCodeForField(ModelGenerator.java:470)
    com.hedera.pbj.compiler.impl.generators.ModelGenerator.generate(ModelGenerator.java:107)
    com.hedera.pbj.compiler.PbjCompilerTask.compileFilesIn(PbjCompilerTask.java:107)

protoc properly ignores the comment, rather than failing to parse the file properly.
The grammar for PBJ appears to be missing a term for the comment preceding the reserved keyword.

@jsync-swirlds
Copy link
Member Author

PBJ also appears to fail if a comment precedes the syntax keyword.
protoc also, appropriately, ignores such comments, and these are needed for automatically generated API documentation.

@jsync-swirlds jsync-swirlds changed the title PBJ Compiler fails on comment preceding reserved field PBJ Compiler fails on comment preceding reserved field or syntax keyword May 9, 2024
jsync-swirlds added a commit that referenced this issue Jan 9, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added docComment before `syntax`, `import`, `package`, `option`, and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so we have to add it everywhere.
        Skipped comments (the `//` type) can be absolutely anywhere because the grammar _skips_ them.
   * Note, #319 still prevents using docComment before `reserved`.

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 9, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added skippedDocComment before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 10, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added skippedDocComment before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 10, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added documentation comments before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
@jsync-swirlds jsync-swirlds self-assigned this Jan 10, 2025
jsync-swirlds added a commit that referenced this issue Jan 10, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added documentation comments before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 13, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added documentation comments before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 13, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added documentation comments before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 14, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added documentation comments before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
jsync-swirlds added a commit that referenced this issue Jan 14, 2025
* Fixes #145
   * Renamed `intergration` to `integration` globally
* Fixes #217
   * Added documentation comments before `syntax`, `import`, `package`, `option`,
     and `reserved` keywords in grammar
      * The root cause is the change to docComment to make it non-skipped, so
        we have to add it everywhere. Skipped comments (the `//` type) can be
        absolutely anywhere because the grammar _skips_ them.
   * Note, this _might_ also fix #319

Signed-off-by: Joseph Sinclair <121976561+jsync-swirlds@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant