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

AST parent pointer implementation #147

Merged
merged 24 commits into from
Oct 26, 2022
Merged

AST parent pointer implementation #147

merged 24 commits into from
Oct 26, 2022

Conversation

anweiss
Copy link
Owner

@anweiss anweiss commented Oct 14, 2022

Implements #150.

Based on the work started by @SebastienGllmt in #134. Removes parent fields from the AST in favor of a rudimentary "arena" implementation which negates the need for AST manipulation. Introduces a ParentVisitor which houses the arena tree and implements the Visitor. Also introduces a CDDLType enum which holds all of the different AST nodes.

ParentVisitor and CDDLType can be used as follows to identify an AST node's parent:

let cddl_ast = cddl_from_str(r#"a = "myrule""#, true).unwrap();
let pv = ParentVisitor::new(&c).unwrap();
let rule = cddl_ast.rules.first().unwrap();

assert_eq!(
  CDDLType::from(rule).parent(&pv).unwrap(),
  &CDDLType::from(&cddl_ast)
);

You can also walk up the parent tree by recursively calling parent() on the returned CDDLType as follows:

let cddl_ast = cddl_from_str(r#"a = "myrule""#, true).unwrap();
let pv = ParentVisitor::new(&cddl_ast).unwrap();

if let Rule::Type { rule: tr, .. } = cddl_ast.rules.first().unwrap() {
  assert_eq!(
    CDDLType::from(tr).parent(&pv).unwrap().parent(&pv).unwrap(),
    &CDDLType::from(&cddl_ast)
   );
}

In the example above, the TypeRule's parent's parent is the main CDDL struct itself.

CDDLType::From has also been implemented for all of the AST types.

@SebastienGllmt let me know if this API works for you.

@anweiss anweiss added the enhancement New feature or request label Oct 14, 2022
@anweiss anweiss added this to the v1.0.0 milestone Oct 14, 2022
@anweiss anweiss self-assigned this Oct 14, 2022
@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch from b283e70 to fb6face Compare October 14, 2022 15:21
@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch from 1b8817d to 7a626dd Compare October 14, 2022 17:47
@anweiss anweiss mentioned this pull request Oct 14, 2022
5 tasks
@SebastienGllmt
Copy link
Contributor

SebastienGllmt commented Oct 15, 2022

Slightly unfortunate this approach means you lose the type of the parent and have to use a broad CDDLType type as it will make pattern matching on the parent easier to get wrong. It might be better to bring back the original parent types from my original PR and put the casts somewhere in this library just to avoid downstream users having to think about it (ex: every node type could have a function that calls its own parent function and then performs the cast). This can be done in a followup PR though if we want it

@anweiss
Copy link
Owner Author

anweiss commented Oct 15, 2022

Yea this was one of the trade offs vs having to manipulate the existing AST and storing the parents in the AST. But I agree that the specific concrete type of the parent should be returned. Will make some changes.

@anweiss
Copy link
Owner Author

anweiss commented Oct 17, 2022

@SebastienGllmt updated to abstract the CDDLType type and introduced a Parent trait which is implemented on the AST types. So you get this usage instead:

let cddl = cddl_from_str(r#"a = "myrule""#, true).unwrap();
let pv = ParentVisitor::new(&cddl).unwrap();
let rule = cddl.rules.first().unwrap();

assert_eq!(rule.parent(&pv).unwrap(), &cddl);

Parent trait definition:

pub trait Parent<'a, 'b: 'a, T> {
  fn parent(&'a self, parent_visitor: &'b ParentVisitor<'a, 'b>) -> Option<&T>;
}

In situations where certain AST nodes can have different parent types based on context, you'll need to manually cast ... so for example:

let cddl = cddl_from_str(r#"a = "myrule""#, true).unwrap();
let pv = ParentVisitor::new(&cddl).unwrap();

if let Rule::Type { rule, .. } = cddl.rules.first().unwrap() {
  let parent: &TypeRule = rule.name.parent(&pv).unwrap();
  assert_eq!(parent, rule);
}

... the rule name's ident a has the parent of type TypeRule ...

vs.

let cddl = cddl_from_str(
  r#"
    terminal-color = &basecolors
    basecolors = (
      black: 0,  red: 1,  green: 2,  yellow: 3,
      blue: 4,  magenta: 5,  cyan: 6,  white: 7,
    )
  "#,
  true,
)
.unwrap();
let pv = ParentVisitor::new(&cddl).unwrap();

if let Rule::Type { rule, .. } = cddl.rules.first().unwrap() {
  if let t2 @ Type2::ChoiceFromGroup { ident, .. } =
    &rule.value.type_choices.first().unwrap().type1.type2
  {
    let parent: &Type2 = ident.parent(&pv).unwrap();
    assert_eq!(parent, t2);
  }
}

... wherein the ident &basecolors has the parent of type Type2.

@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch from a2a8e77 to 6e78e23 Compare October 19, 2022 13:48
@anweiss
Copy link
Owner Author

anweiss commented Oct 19, 2022

@SebastienGllmt PR is ready for final review and merge

@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch 2 times, most recently from d7dc14f to c0c7765 Compare October 20, 2022 13:27
@SebastienGllmt
Copy link
Contributor

SebastienGllmt commented Oct 23, 2022

I think you forgot RangeOp / CtlOp

Also, CtlOp has a ctr: &'a str inside it, but I think this should be Identifier and marked as a parent

@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch from 7a6259e to d0e2b76 Compare October 25, 2022 14:30
@anweiss
Copy link
Owner Author

anweiss commented Oct 25, 2022

@SebastienGllmt RangeCtlOp parent implemented. Moved the control operator tokens into a separate enum.

@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch from 70a3e08 to 2c271dd Compare October 25, 2022 15:18
@anweiss
Copy link
Owner Author

anweiss commented Oct 25, 2022

@SebastienGllmt this is ready to merge

@SebastienGllmt
Copy link
Contributor

LGTM. Integration worked as expected so far in our codebase 👍

@anweiss anweiss force-pushed the dcSpark-ast-parent-pointer branch from 6e1dc6e to 8a3cabd Compare October 25, 2022 19:57
@anweiss anweiss linked an issue Oct 25, 2022 that may be closed by this pull request
@anweiss anweiss merged commit 10eb9ba into main Oct 26, 2022
@anweiss anweiss deleted the dcSpark-ast-parent-pointer branch October 26, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parent node visitor
2 participants