Repository navigation
Add API for user defined grmtools section entries in GrammarAST #667
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
base: master
Are you sure you want to change the base?
Changes from 13 commits
59f354a
eff4480
14537c3
a96e799
86a4457
adb16f4
fd3fa86
fcba11b
751a4d4
e8012c9
9bb74bf
2c47d29
1cde528
9e0c164
f3079aa
ab83ba4
75127c6
a1d8a8f
0ca6955
6cca7a6
a4e14d5
45fca2b
c587fab
c3b3f6c
e8e1e7f
22d5f11
74c2ba0
70ce6f1
d7512ad
804a34d
534631b
19289b5
a8065ff
16d66fd
5c9ac98
8f90a95
642f94e
d8d00bf
f00c036
e96f2eb
35fe0ec
abbd6c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,10 @@ use super::{ | |
|
|
||
| use crate::{ | ||
| Span, | ||
| header::{GrmtoolsSectionParser, HeaderError, HeaderErrorKind, HeaderValue}, | ||
| header::{ | ||
| GrmtoolsSectionParser, Header, HeaderError, HeaderErrorKind, HeaderValue, RE_CRATE_DOT, | ||
| Value, | ||
| }, | ||
| yacc::YaccOriginalActionKind, | ||
| }; | ||
|
|
||
|
|
@@ -39,6 +42,7 @@ impl fmt::Display for ASTModificationError { | |
| pub struct ASTWithValidityInfo { | ||
| yacc_kind: YaccKind, | ||
| ast: GrammarAST, | ||
| grmtools_section: Header<Span>, | ||
| errs: Vec<YaccGrammarError>, | ||
| } | ||
|
|
||
|
|
@@ -52,18 +56,19 @@ impl ASTWithValidityInfo { | |
| /// already extracted the `YaccKind` if any. | ||
| pub fn new(yacc_kind: YaccKind, s: &str) -> Self { | ||
| let mut errs = Vec::new(); | ||
| let ast = { | ||
| let (ast, grmtools_section) = { | ||
| let mut yp = YaccParser::new(yacc_kind, s); | ||
| yp.parse().map_err(|e| errs.extend(e)).ok(); | ||
| let mut ast = yp.build(); | ||
| let (mut ast, grmtools_section) = yp.build(); | ||
| ast.complete_and_validate(Some(yacc_kind)) | ||
| .map_err(|e| errs.push(e)) | ||
| .ok(); | ||
| ast | ||
| (ast, grmtools_section) | ||
| }; | ||
| ASTWithValidityInfo { | ||
| ast, | ||
| errs, | ||
| grmtools_section, | ||
| yacc_kind, | ||
| } | ||
| } | ||
|
|
@@ -108,6 +113,48 @@ impl ASTWithValidityInfo { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Performs a lookup in the grmtools section for an entry with the key `crate_name.key_name` and returns it. | ||
| /// If the entry is found it marks the key as `used`, for the purposes of `unused_header_keys_for_crate`. | ||
| pub fn header_value_for_crate( | ||
| &mut self, | ||
| crate_name: &str, | ||
| key_name: &str, | ||
| ) -> Option<(Span, &Value<Span>)> { | ||
| let key = format!("{crate_name}.{key_name}"); | ||
| self.grmtools_section.mark_used(&key); | ||
| if let Some(HeaderValue(span, value)) = self.grmtools_section.get(&key) { | ||
| Some((*span, value)) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| /// Returns all key names given in the header specified by a `%grmtools` directive with the | ||
| /// `crate_name.` prefix for the given crate. If the `crate_name` is None returns any unused | ||
| /// keys with no crate prefix specified. | ||
| pub fn unused_header_keys_for_crate(&self, crate_name: Option<&str>) -> Vec<(String, Span)> { | ||
| self.grmtools_section | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, one thing with this function that could be improved is that it gets called for each unused key, for each crate. It could if passed a set of crate names, do a single pass checking if the prefix of each unused key exists in the set of crate_prefixes. So instead of doing a pass for each crate, it could do a single pass for all crates. This wouldn't be hard to do now, I think it'd even be deterministic for the testsuite, I'm not sure if it is worth the trouble with the low number of crates and keys though?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had done this in c3b3f6c |
||
| .unused() | ||
| .iter() | ||
| .filter_map(|(key_name, HeaderValue(key_span, _))| { | ||
| if let Some(crate_name) = crate_name { | ||
| let crate_prefix = format!("{crate_name}."); | ||
| if key_name.starts_with(&crate_prefix) { | ||
| Some((key_name.clone(), *key_span)) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| if !RE_CRATE_DOT.is_match(key_name) { | ||
| Some((key_name.clone(), *key_span)) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| } | ||
| } | ||
|
|
||
| impl FromStr for ASTWithValidityInfo { | ||
|
|
@@ -120,19 +167,20 @@ impl FromStr for ASTWithValidityInfo { | |
| .map_err(|mut errs| errs.drain(..).map(|e| e.into()).collect::<Vec<_>>())?; | ||
| if let Some(HeaderValue(_, yk_val)) = header.get("cfgrammar.yacckind") { | ||
| let yacc_kind = YaccKind::try_from(yk_val).map_err(|e| vec![e.into()])?; | ||
| let ast = { | ||
| let (ast, grmtools_section) = { | ||
| // We don't want to strip off the header so that span's will be correct. | ||
| let mut yp = YaccParser::new(yacc_kind, src); | ||
| yp.parse().map_err(|e| errs.extend(e)).ok(); | ||
| let mut ast = yp.build(); | ||
| let (mut ast, grmtools_section) = yp.build(); | ||
| ast.complete_and_validate(Some(yacc_kind)) | ||
| .map_err(|e| errs.push(e)) | ||
| .ok(); | ||
| ast | ||
| (ast, grmtools_section) | ||
| }; | ||
| Ok(ASTWithValidityInfo { | ||
| ast, | ||
| errs, | ||
| grmtools_section, | ||
| yacc_kind, | ||
| }) | ||
| } else { | ||
|
|
@@ -984,4 +1032,205 @@ start -> () : "a" {$;;;; }; | |
| }] | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_grmtools_section_values() { | ||
| use super::*; | ||
| use crate::header::Value; | ||
| let src = r#" | ||
| %grmtools { | ||
| yacckind: Grmtools, | ||
| lrpar.recoverer: CPCTPlus, | ||
| test.Flag, | ||
| !test.Negative, | ||
| test.string: "Foo", | ||
| test.vec: ["Aaaa", "Bbbb"], | ||
| test.num: 1234, | ||
| test.unused: 5678 | ||
| } | ||
| %token a | ||
| %% | ||
| start -> () : "a" { () }; | ||
| "#; | ||
| let mut ast_validity = ASTWithValidityInfo::from_str(src).unwrap(); | ||
| for (key, (expected_span, expected_value)) in [ | ||
| ( | ||
| "Flag", | ||
| ( | ||
| src.find_span("test.Flag"), | ||
| Value::Bool(true, src.find_span("test.Flag")), | ||
| ), | ||
| ), | ||
| ( | ||
| "Negative", | ||
| ( | ||
| src.find_span("test.Negative"), | ||
| Value::Bool(false, src.find_span("!test.Negative")), | ||
| ), | ||
| ), | ||
| ( | ||
| "string", | ||
| ( | ||
| src.find_span("test.string"), | ||
| Value::String("Foo".to_string(), src.find_span("Foo")), | ||
| ), | ||
| ), | ||
| ( | ||
| "vec", | ||
| ( | ||
| src.find_span("test.vec"), | ||
| Value::Array( | ||
| vec![ | ||
| Value::String("Aaaa".to_string(), src.find_span("Aaaa")), | ||
| Value::String("Bbbb".to_string(), src.find_span("Bbbb")), | ||
| ], | ||
| src.find_span("[\"Aaaa\", \"Bbbb\"]"), | ||
| ), | ||
| ), | ||
| ), | ||
| ( | ||
| "num", | ||
| ( | ||
| src.find_span("test.num"), | ||
| Value::Num(1234, src.find_span("1234")), | ||
| ), | ||
| ), | ||
| ] { | ||
| let value = ast_validity.header_value_for_crate("test", key); | ||
| assert_eq!(value, Some((expected_span, &expected_value))); | ||
| } | ||
| assert_eq!( | ||
| ast_validity.unused_header_keys_for_crate(Some("test")), | ||
| vec![("test.unused".to_string(), src.find_span("test.unused"))] | ||
| ); | ||
| assert_eq!( | ||
| ast_validity.header_value_for_crate("cfgrammar", "yacckind"), | ||
| Some(( | ||
| src.find_span("yacckind"), | ||
| &Value::Namespaced("Grmtools".to_string(), src.find_span("Grmtools")) | ||
| )) | ||
| ); | ||
|
|
||
| assert!( | ||
| ast_validity | ||
| .unused_header_keys_for_crate(Some("cfgrammar")) | ||
| .is_empty() | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| ast_validity.header_value_for_crate("lrpar", "recoverer"), | ||
| Some(( | ||
| src.find_span("lrpar.recoverer"), | ||
| &Value::Namespaced("CPCTPlus".to_string(), src.find_span("CPCTPlus")) | ||
| )) | ||
| ); | ||
|
|
||
| assert!( | ||
| ast_validity | ||
| .unused_header_keys_for_crate(Some("lrpar")) | ||
| .is_empty() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_grmtools_section_values2() { | ||
| use super::*; | ||
| let src = r#" | ||
| %grmtools { | ||
| yacckind: Original(YaccOriginalActionKind::UserAction), | ||
| } | ||
| %token a | ||
| %actiontype () | ||
| %% | ||
| start: "a" { () }; | ||
| "#; | ||
| let mut ast_validity = ASTWithValidityInfo::from_str(src).unwrap(); | ||
| assert_eq!( | ||
| ast_validity.header_value_for_crate("cfgrammar", "yacckind"), | ||
| Some(( | ||
| src.find_span("yacckind"), | ||
| &Value::Namespaced( | ||
| "Original(YaccOriginalActionKind::UserAction)".to_string(), | ||
| src.find_span("Original(YaccOriginalActionKind::UserAction)"), | ||
| ), | ||
| )) | ||
| ); | ||
| assert!( | ||
| ast_validity | ||
| .unused_header_keys_for_crate(Some("cfgrammar")) | ||
| .is_empty() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_grmtools_section_values3() { | ||
| use super::*; | ||
| let src = r#" | ||
| %grmtools { | ||
| yacckind: YaccKind::Original(YaccOriginalActionKind::UserAction), | ||
| } | ||
| %token a | ||
| %actiontype () | ||
| %% | ||
| start: "a" { () }; | ||
| "#; | ||
| let mut ast_validity = ASTWithValidityInfo::from_str(src).unwrap(); | ||
| assert_eq!( | ||
| ast_validity.header_value_for_crate("cfgrammar", "yacckind"), | ||
| Some(( | ||
| src.find_span("yacckind"), | ||
| &Value::Namespaced( | ||
| "YaccKind::Original(YaccOriginalActionKind::UserAction)".to_string(), | ||
| src.find_span("YaccKind::Original(YaccOriginalActionKind::UserAction)"), | ||
| ), | ||
| )) | ||
| ); | ||
| assert!( | ||
| ast_validity | ||
| .unused_header_keys_for_crate(Some("cfgrammar")) | ||
| .is_empty() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_grmtools_section_values4() { | ||
| use super::*; | ||
| let src = r#" | ||
| %grmtools { | ||
| yacckind: YaccKind::Original(UserAction), | ||
| } | ||
| %token a | ||
| %actiontype () | ||
| %% | ||
| start: "a" { () }; | ||
| "#; | ||
| let mut ast_validity = ASTWithValidityInfo::from_str(src).unwrap(); | ||
| assert_eq!( | ||
| ast_validity.header_value_for_crate("cfgrammar", "yacckind"), | ||
| Some(( | ||
| src.find_span("yacckind"), | ||
| &Value::Namespaced( | ||
| "YaccKind::Original(UserAction)".to_string(), | ||
| src.find_span("YaccKind::Original(UserAction)"), | ||
| ), | ||
| )) | ||
| ); | ||
| assert!( | ||
| ast_validity | ||
| .unused_header_keys_for_crate(Some("cfgrammar")) | ||
| .is_empty() | ||
| ); | ||
| } | ||
|
|
||
| trait FindSpan { | ||
| fn find_span(&self, s: &str) -> Span; | ||
| } | ||
|
|
||
| impl FindSpan for &'_ str { | ||
| #[track_caller] | ||
| fn find_span(&self, s: &str) -> Span { | ||
| let start_pos = self.find(s).unwrap(); | ||
| Span::new(start_pos, start_pos + s.len()) | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question: do we even need to force two variables (
crate_nameandkey_name) on the user? Wouldkey: &strmake sense alone?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, I made the same comment in a review above too, for a different function.
I kind of flipped a coin, and did separate crate names and keys, due to the separate crate name elsewhere
like
check_unused_...where the key name isn't passed in at all.It didn't seem like a strong argument then, happy to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if we use a single string, we get a nicer API in #667 (comment) too? Warning: I might be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed in 75127c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance this does look like it improves things?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think so.
Edit: Moved my other part of the coment elsewhere.