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.

Sign up for GitHub

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

Jump to bottom

feat: CDDL grammar correction for RFC8610 #61

Merged
merged 43 commits into from
Jan 30, 2024
Merged

feat: CDDL grammar correction for RFC8610 #61

merged 43 commits into from
Jan 30, 2024

Conversation

apskhem
Copy link
Collaborator

@apskhem apskhem commented Jan 17, 2024 •
edited
Loading

Description

Correct parsing grammar according to RFC8610, and add exhaustive unit test cases for each rule in the grammar. RFC8610 grammar is based on https://github.com/cbor-wg/cddl/blob/master/cddl.abnf.

Related Issue(s)

Closes #57

Description of Changes

  • Changed grammar file name from rfc_9615.pest to rfc_9165.pest
  • Added decent coverage unit test cases for each grammar rule
  • Added sample CDDL files from its official documentation for integration parsing testing
  • Changed and corrected rfc_8610.pest grammar to reflect its specification

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@apskhem apskhem added the draft Draft label Jan 17, 2024
@apskhem apskhem self-assigned this Jan 17, 2024
apskhem
apskhem commented Jan 17, 2024
View reviewed changes
Copy link
Collaborator Author

@apskhem apskhem left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes here:

I compacted the id rule to simply accept as name. There is no more group_socket or type_socket as this can be checked during the semantic checks phase. I also changed the bytes to be more flexible as it caused parsing ambiguity here:

gen-value = ( b64'asdf': uint )

would produce pairs like this:

- grpent > group > grpchoice > grpent
   - memberkey > value > bytes > bytes_b64: "b64'asdf'"

but for hex string:

gen-value = ( h'atat': uint )

- grpent > group > grpchoice
   - grpent > type > type1 > type2 > typename > id > name: "h"
   - optcom: ""
   - grpent
    - memberkey > value > bytes > bytes_text: "'atat'"
 
 instead of:
 - grpent > group > grpchoice > grpent
   - memberkey > value > bytes > bytes_hex: "h'atat'"

As you can see, it produced 2 grpents which is unexpected.

The validation of the value inside a byte string validity would leave for semantic checks either.

I also removed both typename_arg and groupname_arg as the official ABNF doesn't contain these rules and it's easier to keep tracking from the original grammar.

@Mr-Leshiy Mr-Leshiy linked an issue Jan 18, 2024 that may be closed by this pull request
Ensure the Errata for rfc8610 is properly reflected in the existing cddl.pest file. #57
Closed
@apskhem apskhem removed the draft Draft label Jan 24, 2024
stevenj
stevenj requested changes Jan 25, 2024
View reviewed changes
hermes/crates/cbork/cddl-parser/tests/byte_sequences.rs Outdated Show resolved Hide resolved
hermes/crates/cbork/cddl-parser/tests/cddl.rs Outdated Show resolved Hide resolved
hermes/crates/cbork/cddl-parser/tests/comments.rs Outdated Show resolved Hide resolved
hermes/crates/cbork/cddl-parser/tests/group_elements.rs Outdated Show resolved Hide resolved
hermes/crates/cbork/cddl-parser/tests/literal_values.rs Outdated Show resolved Hide resolved
hermes/crates/cbork/cddl-parser/tests/text_sequences.rs Outdated Show resolved Hide resolved
hermes/crates/cbork/cddl-parser/tests/type_declarations.rs Outdated Show resolved Hide resolved
@minikin minikin added the review me PR is ready for review label Jan 26, 2024
Mr-Leshiy
Mr-Leshiy approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, despite one place

hermes/crates/cbork/cddl-parser/tests/common/byte_sequences.rs Outdated Show resolved Hide resolved
stevenj
stevenj approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apskhem apskhem merged commit efe780a into feat/cbork Jan 30, 2024
21 checks passed
@apskhem apskhem deleted the feat/errata-rfc8610 branch January 30, 2024 08:55
stevenj added a commit that referenced this pull request Mar 18, 2024
@stevenj @apskhem
feat(cbork): Add new cbor kit crates (#116)
8b2ac7b 
* feat(cbork): First cddl parser code

* feat: Earthfile to CBORK (#53)

* feat: earthfile

* feat: add targets in Rust toolchain

* fix: enable cspell and add file specific words instead

* fix: fmtfix

* fix: resolver

* fix: cspell my name

* fix: comment

* fix: add generate-lockfile

* feat: CDDL module structure (#55)

* feat: module extension

* refactor: compact return type

* chore: fmtfix

* fix: lint

* fix: function docs comment

* feat: add placeholders

* refactor: compact error mapping

* fix: remove cddl test error type

* fix: postlude test

* fix: move postlude content inside test

* docs: CDDLErrorType

* docs: fix typo

* fix: test print

* feat: placeholder files

* refactor: remove direct type

* chore: fix linting

* fix: return unit instead

* fix: input as &mut String

* chore: fmtfix

* feat: CDDL grammar correction for RFC8610 (#61)

* feat: example files

* refactor: rename test files

* refactor: rename from rfc9615 to rfc9165

* feat: error display for testing multiple files

* feat: sort file reading

* fix: whitespace problem for ',', braces, and //

* feat: group elements initial unit test

* fix: comments

* fix: grammar correction for comments

* fix: grammar correcttion according to abnf

* fix: remove genericarg from typename and groupname

* fix: unit tests

* feat: initial group_elements test cases

* fix: put atomic test back

* revert: deleted grammar and tests

* fix: correct test cases

* feat: type decl tests

* feat: type decl tests

* feat: setup type decl test

* feat: type1 test cases

* feat: composition testing

* feat: rules test cases

* feat: rules test cases

* feat: all examples from rfc8610

* feat: add rule level tests

* refactor: use general passes and fails function in unit test

* fix: error msg for cddl

* fix: cddl test file name reading

* fix: cspell

* chore: lintfix

* refactor: cddl filter fn

* refactor: reset

* fix: pub(crate) for unit tests

* chore: fmtfix

* refactor: don't dry util functions

* fix: disable lint warning

* chore: fmtfix

* refactor: change common path

* feat: common

* refactor: remove tmp vars

* feat: common consts

* fix: add tmp allow dead code

* fix: cspell

* feat(cbor): Initial ABNF parser (#113)

* feat: initial commit

* feat: initial abnf grammar

* fix: correct abnf grammar

* feat: update abnf test

* feat: integration test init

* fix: update correction on errata

* fix: correct grammar

* fix: grammar on testing

* feat: test modules

* feat: comment tests

* feat: unit tests placeholder

* feat: c_wsp tests

* feat: value tests

* feat: repeat

* fix: restructure groups and elements

* feat: all unit tests

* fix: partial cspell

* fix: cspell

* fix: config files

* ci: update earthly

* docs: parse function

* fix: minor cspell

* docs: cddl ast temp

* docs: fix cddl docs

* docs: add docs to abnf AST and ABNFError

* fix: dead code

---------

Co-authored-by: Apisit Ritreungroj <38898766+apskhem@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mr-Leshiy Mr-Leshiy Mr-Leshiy approved these changes

@stevenj stevenj stevenj approved these changes

@FelipeRosa FelipeRosa Awaiting requested review from FelipeRosa

@cong-or cong-or Awaiting requested review from cong-or

@saibatizoku saibatizoku Awaiting requested review from saibatizoku

@minikin minikin Awaiting requested review from minikin

Assignees

@apskhem apskhem

Labels
review me PR is ready for review
Projects
Catalyst
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure the Errata for rfc8610 is properly reflected in the existing cddl.pest file.
4 participants
@apskhem @stevenj @Mr-Leshiy @minikin