PDL: Change the scoping of packet and group declarations
Packet, struct, and group previously lived in different scopes,
which allowed packets and structs to have the same name.
This impacts the rust code generation as packets and structs are
disambiguated by adding appropriate suffixes, which is undesirable.
Packets and groups are now declared in the same namespace
as structs and other typedef declarations. An error is raised
if a packet is declared with the same name as another struct or
group declaration.
Test: pdl_inline_tests, validate hci_packets.pdl file
Bug: 228327522
Change-Id: I8055bed118e04de7297e240ac075ca529b5a8f8a
diff --git a/tools/pdl/src/lint.rs b/tools/pdl/src/lint.rs
index 0b2d3bc..6d9e655 100644
--- a/tools/pdl/src/lint.rs
+++ b/tools/pdl/src/lint.rs
@@ -26,15 +26,7 @@
/// Gather information about the full grammar declaration.
struct Scope<'d> {
- // Collection of Group declarations.
- groups: HashMap<String, &'d Decl>,
-
- // Collection of Packet declarations.
- packets: HashMap<String, &'d Decl>,
-
- // Collection of Enum, Struct, Checksum, and CustomField declarations.
- // Packet and Group can not be referenced in a Typedef field and thus
- // do not share the same namespace.
+ // Collection of Group, Packet, Enum, Struct, Checksum, and CustomField declarations.
typedef: HashMap<String, &'d Decl>,
// Collection of Packet, Struct, and Group scope declarations.
@@ -499,10 +491,11 @@
_ => (),
}
- let (parent_id, parent_namespace, fields) = match decl {
- Decl::Packet { parent_id, fields, .. } => (parent_id, &scope.packets, fields),
- Decl::Struct { parent_id, fields, .. } => (parent_id, &scope.typedef, fields),
- Decl::Group { fields, .. } => (&None, &scope.groups, fields),
+ let (parent_id, fields) = match decl {
+ Decl::Packet { parent_id, fields, .. } | Decl::Struct { parent_id, fields, .. } => {
+ (parent_id.as_ref(), fields)
+ }
+ Decl::Group { fields, .. } => (None, fields),
_ => return None,
};
@@ -513,7 +506,7 @@
for f in fields {
match f {
Field::Group { group_id, constraints, .. } => {
- match scope.groups.get(group_id) {
+ match scope.typedef.get(group_id) {
None => result.push(
Diagnostic::error()
.with_message(format!(
@@ -522,7 +515,7 @@
))
.with_labels(vec![f.loc().primary()]),
),
- Some(group_decl) => {
+ Some(group_decl @ Decl::Group { .. }) => {
// Recurse to flatten the inserted group.
if let Some(rscope) = bfs(group_decl, context, scope, result) {
// Inline the group fields and constraints into
@@ -530,6 +523,15 @@
lscope.inline(scope, rscope, f, constraints.iter(), result)
}
}
+ Some(_) => result.push(
+ Diagnostic::error()
+ .with_message(format!(
+ "invalid group field identifier `{}`",
+ group_id
+ ))
+ .with_labels(vec![f.loc().primary()])
+ .with_notes(vec!["hint: expected group identifier".to_owned()]),
+ ),
}
}
Field::Typedef { type_id, .. } => {
@@ -554,21 +556,32 @@
}
// Iterate over parent declaration.
- for id in parent_id {
- match parent_namespace.get(id) {
- None => result.push(
- Diagnostic::error()
- .with_message(format!("undeclared parent identifier `{}`", id))
- .with_labels(vec![decl.loc().primary()])
- .with_notes(vec![format!("hint: expected {} parent", decl.kind())]),
- ),
- Some(parent_decl) => {
- if let Some(rscope) = bfs(parent_decl, context, scope, result) {
- // Import the parent fields and constraints into the current scope.
- lscope.inherit(scope, rscope, decl.constraints(), result)
- }
+ let parent = parent_id.and_then(|id| scope.typedef.get(id));
+ match (decl, parent) {
+ (Decl::Packet { parent_id: Some(_), .. }, None)
+ | (Decl::Struct { parent_id: Some(_), .. }, None) => result.push(
+ Diagnostic::error()
+ .with_message(format!(
+ "undeclared parent identifier `{}`",
+ parent_id.unwrap()
+ ))
+ .with_labels(vec![decl.loc().primary()])
+ .with_notes(vec![format!("hint: expected {} parent", decl.kind())]),
+ ),
+ (Decl::Packet { .. }, Some(Decl::Struct { .. }))
+ | (Decl::Struct { .. }, Some(Decl::Packet { .. })) => result.push(
+ Diagnostic::error()
+ .with_message(format!("invalid parent identifier `{}`", parent_id.unwrap()))
+ .with_labels(vec![decl.loc().primary()])
+ .with_notes(vec![format!("hint: expected {} parent", decl.kind())]),
+ ),
+ (_, Some(parent_decl)) => {
+ if let Some(rscope) = bfs(parent_decl, context, scope, result) {
+ // Import the parent fields and constraints into the current scope.
+ lscope.inherit(scope, rscope, decl.constraints(), result)
}
}
+ _ => (),
}
lscope.finalize(result);
@@ -581,7 +594,7 @@
let mut context =
Context::<'d> { list: vec![], visited: HashMap::new(), scopes: HashMap::new() };
- for decl in self.packets.values().chain(self.typedef.values()).chain(self.groups.values()) {
+ for decl in self.typedef.values() {
bfs(decl, &mut context, self, result);
}
@@ -1215,28 +1228,15 @@
impl Grammar {
fn scope<'d>(&'d self, result: &mut LintDiagnostics) -> Scope<'d> {
- let mut scope = Scope {
- groups: HashMap::new(),
- packets: HashMap::new(),
- typedef: HashMap::new(),
- scopes: HashMap::new(),
- };
+ let mut scope = Scope { typedef: HashMap::new(), scopes: HashMap::new() };
// Gather top-level declarations.
// Validate the top-level scopes (Group, Packet, Typedef).
//
// TODO: switch to try_insert when stable
for decl in &self.declarations {
- if let Some((id, namespace)) = match decl {
- Decl::Checksum { id, .. }
- | Decl::CustomField { id, .. }
- | Decl::Struct { id, .. }
- | Decl::Enum { id, .. } => Some((id, &mut scope.typedef)),
- Decl::Group { id, .. } => Some((id, &mut scope.groups)),
- Decl::Packet { id, .. } => Some((id, &mut scope.packets)),
- _ => None,
- } {
- if let Some(prev) = namespace.insert(id.clone(), decl) {
+ if let Some(id) = decl.id() {
+ if let Some(prev) = scope.typedef.insert(id.clone(), decl) {
result.err_redeclared(id, decl.kind(), decl.loc(), prev.loc())
}
}
@@ -1263,3 +1263,31 @@
result
}
}
+
+#[cfg(test)]
+mod test {
+ use crate::ast::*;
+ use crate::lint::Lintable;
+ use crate::parser::parse_inline;
+
+ macro_rules! grammar {
+ ($db:expr, $text:literal) => {
+ parse_inline($db, "stdin".to_owned(), $text.to_owned()).expect("parsing failure")
+ };
+ }
+
+ #[test]
+ fn test_packet_redeclared() {
+ let mut db = SourceDatabase::new();
+ let grammar = grammar!(
+ &mut db,
+ r#"
+ little_endian_packets
+ struct Name { }
+ packet Name { }
+ "#
+ );
+ let result = grammar.lint();
+ assert!(!result.diagnostics.is_empty());
+ }
+}
diff --git a/tools/pdl/src/parser.rs b/tools/pdl/src/parser.rs
index bea80e0..821207a 100644
--- a/tools/pdl/src/parser.rs
+++ b/tools/pdl/src/parser.rs
@@ -506,6 +506,26 @@
Ok(grammar)
}
+/// Parse a PDL grammar text.
+/// The grammar is added to the compilation database under the
+/// provided name.
+pub fn parse_inline(
+ sources: &mut ast::SourceDatabase,
+ name: String,
+ source: String,
+) -> Result<ast::Grammar, Diagnostic<ast::FileId>> {
+ let root = PDLParser::parse(Rule::grammar, &source)
+ .map_err(|e| {
+ Diagnostic::error()
+ .with_message(format!("failed to parse input file '{}': {}", &name, e))
+ })?
+ .next()
+ .unwrap();
+ let line_starts: Vec<_> = files::line_starts(&source).collect();
+ let file = sources.add(name, source.clone());
+ parse_grammar(root, &(file, &line_starts)).map_err(|e| Diagnostic::error().with_message(e))
+}
+
/// Parse a new source file.
/// The source file is fully read and added to the compilation database.
/// Returns the constructed AST, or a descriptive error message in case
@@ -517,14 +537,5 @@
let source = std::fs::read_to_string(&name).map_err(|e| {
Diagnostic::error().with_message(format!("failed to read input file '{}': {}", &name, e))
})?;
- let root = PDLParser::parse(Rule::grammar, &source)
- .map_err(|e| {
- Diagnostic::error()
- .with_message(format!("failed to parse input file '{}': {}", &name, e))
- })?
- .next()
- .unwrap();
- let line_starts: Vec<_> = files::line_starts(&source).collect();
- let file = sources.add(name, source.clone());
- parse_grammar(root, &(file, &line_starts)).map_err(|e| Diagnostic::error().with_message(e))
+ parse_inline(sources, name, source)
}