Improve Redefinition Validation (#700)

src/parsers/common.rs

@@ -6,6 +6,7 @@ use crate::slice_file::Location;
 
 /// Stores a reference to a block of source code in a Slice file.
 #[derive(Clone, Copy, Debug)]
+#[allow(dead_code)] // The `end` field isn't used, but is very useful for debugging, so we keep it.
 pub struct SourceBlock<'input> {
     /// The raw text contained in the block, taken directly from the input.
     pub content: &'input str,

src/validators/identifiers.rs

@@ -42,45 +42,98 @@ fn check_for_shadowing(
 }
 
 pub fn check_for_redefinitions(ast: &Ast, diagnostics: &mut Diagnostics) {
-    // A map storing `(scoped_identifier, named_symbol)` pairs. We iterate through the AST and build up this map,
-    // and if we try to add an entry but see it's already occupied, that means we've found a redefinition.
-    let mut slice_definitions: HashMap<String, &dyn NamedSymbol> = HashMap::new();
+    RedefinitionChecker { diagnostics }.check_for_redefinitions(ast);
+}
 
-    for node in ast.as_slice() {
-        // Only check nodes that have identifiers, everything else is irrelevant.
-        if let Ok(definition) = <&dyn NamedSymbol>::try_from(node) {
-            let scoped_identifier = definition.parser_scoped_identifier();
-            match slice_definitions.get(&scoped_identifier) {
-                // If we've already seen a node with this identifier, there's a name collision.
-                // This is fine for modules (since they can be re-opened), but for any other type, we report an error.
-                Some(other_definition) => {
-                    if !(is_module(definition) && is_module(*other_definition)) {
-                        report_redefinition_error(definition, *other_definition, diagnostics);
-                    }
+struct RedefinitionChecker<'a> {
+    diagnostics: &'a mut Diagnostics,
+}
+
+impl<'a> RedefinitionChecker<'a> {
+    fn check_for_redefinitions(&mut self, ast: &'a Ast) {
+        // Stores all the _module-scoped_ Slice definitions we've seen so far.
+        // Keys are the definition's fully-scoped identifiers, and values are references to the definitions themselves.
+        let mut seen_definitions = HashMap::new();
+
+        for node in ast.as_slice() {
+            // We only check `Entity`s so as to exclude any Slice elements which don't have names (and hence cannot be
+            // redefined), and also to exclude modules (which are reopened, not redefined).
+            let Ok(definition) = <&dyn Entity>::try_from(node) else { continue };
+
+            match definition.concrete_entity() {
+                Entities::Struct(struct_def) => {
+                    self.check_if_redefined(struct_def, &mut seen_definitions);
+                    self.check_contents_for_redefinitions(struct_def.contents());
                 }
+                Entities::Class(class_def) => {
+                    self.check_if_redefined(class_def, &mut seen_definitions);
+                    self.check_contents_for_redefinitions(class_def.contents());
+                }
+                Entities::Exception(exception_def) => {
+                    self.check_if_redefined(exception_def, &mut seen_definitions);
+                    self.check_contents_for_redefinitions(exception_def.contents());
+                }
+                Entities::Interface(interface_def) => {
+                    self.check_if_redefined(interface_def, &mut seen_definitions);
+                    self.check_contents_for_redefinitions(interface_def.contents());
 
-                // If we haven't seen a node with this identifier before, add it to the map and continue checking.
-                None => {
-                    slice_definitions.insert(scoped_identifier, definition);
+                    for operation in interface_def.operations() {
+                        self.check_contents_for_redefinitions(operation.parameters());
+                        self.check_contents_for_redefinitions(operation.return_members());
+                    }
+                }
+                Entities::Enum(enum_def) => {
+                    self.check_if_redefined(enum_def, &mut seen_definitions);
+                    self.check_contents_for_redefinitions(enum_def.contents());
+                }
+                Entities::CustomType(custom_type) => {
+                    self.check_if_redefined(custom_type, &mut seen_definitions);
                 }
+                Entities::TypeAlias(type_alias) => {
+                    self.check_if_redefined(type_alias, &mut seen_definitions);
+                }
+
+                // No need to check `Field`, `Enumerator`, `Operation`, or `Parameter`; We just check their containers.
+                Entities::Field(_) | Entities::Enumerator(_) | Entities::Operation(_) | Entities::Parameter(_) => {}
             }
         }
     }
-}
 
-// TODO improve this function.
-fn is_module(definition: &dyn NamedSymbol) -> bool {
-    definition.kind() == "module"
-}
+    fn check_contents_for_redefinitions<T: NamedSymbol>(&mut self, contents: Vec<&T>) {
+        // We create a separate hashmap, so redefinitions are isolated to just the container we're checking.
+        let mut seen_definitions = HashMap::new();
+        for element in contents {
+            self.check_if_redefined(element, &mut seen_definitions);
+        }
+    }
 
-fn report_redefinition_error(new: &dyn NamedSymbol, original: &dyn NamedSymbol, diagnostics: &mut Diagnostics) {
-    Diagnostic::new(Error::Redefinition {
-        identifier: new.identifier().to_owned(),
-    })
-    .set_span(new.raw_identifier().span())
-    .add_note(
-        format!("'{}' was previously defined here", original.identifier()),
-        Some(original.raw_identifier().span()),
-    )
-    .push_into(diagnostics);
+    /// Checks if the provided `definition` already has an entry in the `already_seen` map. If it does, we report a
+    /// redefinition error, otherwise, we just add it to the map and return.
+    fn check_if_redefined<'b>(
+        &mut self,
+        definition: &'b impl NamedSymbol,
+        already_seen: &mut HashMap<String, &'b dyn NamedSymbol>,
+    ) {
+        let scoped_identifier = definition.parser_scoped_identifier();
+
+        if let Some(other_definition) = already_seen.get(&scoped_identifier) {
+            // We found a name collision; report an error.
+            self.report_redefinition_error(definition, *other_definition);
+        } else {
+            // This is the first time we've seen this identifier, so we add it to the map.
+            already_seen.insert(scoped_identifier, definition);
+        }
+    }
+
+    fn report_redefinition_error(&mut self, new: &dyn NamedSymbol, original: &dyn NamedSymbol) {
+        Diagnostic::new(Error::Redefinition {
+            identifier: new.identifier().to_owned(),
+        })
+        .set_span(new.raw_identifier().span())
+        .add_note(
+            format!("'{}' was previously defined here", original.identifier()),
+            Some(original.raw_identifier().span()),
+        )
+        .push_into(self.diagnostics);
+    }
 }

tests/module_tests.rs

@@ -71,28 +71,4 @@ mod module {
         assert!(ast.find_element::<Struct>("Foo::Test1").is_ok());
         assert!(ast.find_element::<Struct>("Foo::Test2").is_ok());
     }
-
-    #[test]
-    fn cross_module_redefinitions_are_disallowed() {
-        // Arrange
-        let slice1 = "
-            module Foo
-            struct Bar {}
-        ";
-        let slice2 = "
-            module Foo
-            custom Bar
-        ";
-
-        // Act
-        let diagnostics = parse_multiple_for_diagnostics(&[slice1, slice2]);
-
-        // Assert
-        let expected = Diagnostic::new(Error::Redefinition {
-            identifier: "Bar".to_owned(),
-        })
-        .add_note("'Bar' was previously defined here", None);
-
-        check_diagnostics(diagnostics, [expected]);
-    }
 }

tests/redefinition_tests.rs

@@ -0,0 +1,206 @@
+// Copyright (c) ZeroC, Inc.
+
+mod test_helpers;
+
+mod redefinition {
+    use crate::test_helpers::*;
+    use slicec::diagnostics::{Diagnostic, Error};
+    use slicec::slice_file::Span;
+
+    #[test]
+    fn redefinitions_of_the_same_type_are_disallowed() {
+        // Arrange
+        let slice = "
+            module Test
+
+            struct S {
+                i: int32
+            }
+
+            struct S {
+                i: int32
+            }
+        ";
+
+        // Act
+        let diagnostics = parse_for_diagnostics(slice);
+
+        // Assert
+        let expected = Diagnostic::new(Error::Redefinition {
+            identifier: "S".to_owned(),
+        })
+        .set_span(&Span::new((8, 20).into(), (8, 21).into(), "string-0"))
+        .add_note(
+            "'S' was previously defined here",
+            Some(&Span::new((4, 20).into(), (4, 21).into(), "string-0")),
+        );
+
+        check_diagnostics(diagnostics, [expected]);
+    }
+
+    #[test]
+    fn redefinitions_of_different_types_are_disallowed() {
+        // Arrange
+        let slice = "
+            module Test
+
+            enum A { i }
+
+            struct A {
+                i: int32
+            }
+        ";
+
+        // Act
+        let diagnostics = parse_for_diagnostics(slice);
+
+        // Assert
+        let expected = Diagnostic::new(Error::Redefinition {
+            identifier: "A".to_owned(),
+        })
+        .set_span(&Span::new((6, 20).into(), (6, 21).into(), "string-0"))
+        .add_note(
+            "'A' was previously defined here",
+            Some(&Span::new((4, 18).into(), (4, 19).into(), "string-0")),
+        );
+
+        check_diagnostics(diagnostics, [expected]);
+    }
+
+    #[test]
+    fn orthogonal_redefinitions_are_disallowed_separately() {
+        // Arrange
+        let slice = "
+            module Test
+
+            struct A {
+                i: int32
+                i: int64
+            }
+
+            interface A {
+                i(i: int32)
+            }
+        ";
+
+        // Act
+        let diagnostics = parse_for_diagnostics(slice);
+
+        // Assert
+        let i_error = Diagnostic::new(Error::Redefinition {
+            identifier: "i".to_owned(),
+        })
+        .set_span(&Span::new((6, 17).into(), (6, 18).into(), "string-0"))
+        .add_note(
+            "'i' was previously defined here",
+            Some(&Span::new((5, 17).into(), (5, 18).into(), "string-0")),
+        );
+
+        let s_error = Diagnostic::new(Error::Redefinition {
+            identifier: "A".to_owned(),
+        })
+        .set_span(&Span::new((9, 23).into(), (9, 24).into(), "string-0"))
+        .add_note(
+            "'A' was previously defined here",
+            Some(&Span::new((4, 20).into(), (4, 21).into(), "string-0")),
+        );
+
+        check_diagnostics(diagnostics, [i_error, s_error]);
+    }
+
+    #[test]
+    fn multiple_redefinitions_trigger_separate_errors() {
+        // Arrange
+        let slice = "
+            module Test
+
+            struct A {
+                i: int8
+
+                b: bool
+                b: bool
+            }
+
+            interface A {
+                i(b: bool, b: bool) -> (b: bool, b: bool)
+
+                b()
+                b()
+            }
+
+            enum A { i, b, b }
+        ";
+
+        // Act
+        let diagnostics = parse_for_diagnostics(slice);
+
+        // Assert
+        let expected = [
+            // The struct fields
+            Diagnostic::new(Error::Redefinition {
+                identifier: "b".to_owned(),
+            })
+            .set_span(&Span::new((8, 17).into(), (8, 18).into(), "string-0")),
+            // The interface
+            Diagnostic::new(Error::Redefinition {
+                identifier: "A".to_owned(),
+            })
+            .set_span(&Span::new((11, 23).into(), (11, 24).into(), "string-0")),
+            // The operation
+            Diagnostic::new(Error::Redefinition {
+                identifier: "b".to_owned(),
+            })
+            .set_span(&Span::new((15, 17).into(), (15, 18).into(), "string-0")),
+            // The parameter
+            Diagnostic::new(Error::Redefinition {
+                identifier: "b".to_owned(),
+            })
+            .set_span(&Span::new((12, 28).into(), (12, 29).into(), "string-0")),
+            // The return member
+            Diagnostic::new(Error::Redefinition {
+                identifier: "b".to_owned(),
+            })
+            .set_span(&Span::new((12, 50).into(), (12, 51).into(), "string-0")),
+            // The enum
+            Diagnostic::new(Error::Redefinition {
+                identifier: "A".to_owned(),
+            })
+            .set_span(&Span::new((18, 18).into(), (18, 19).into(), "string-0")),
+            // The enumerator
+            Diagnostic::new(Error::Redefinition {
+                identifier: "b".to_owned(),
+            })
+            .set_span(&Span::new((18, 28).into(), (18, 29).into(), "string-0")),
+        ];
+
+        check_diagnostics(diagnostics, expected);
+    }
+
+    #[test]
+    fn cross_module_redefinitions_are_disallowed() {
+        // Arrange
+        let slice1 = "
+            module Foo
+            struct Bar {}
+        ";
+        let slice2 = "
+            module Foo
+            custom Bar
+        ";
+
+        // Act
+        let diagnostics = parse_multiple_for_diagnostics(&[slice1, slice2]);
+
+        // Assert
+        let expected = Diagnostic::new(Error::Redefinition {
+            identifier: "Bar".to_owned(),
+        })
+        .set_span(&Span::new((3, 20).into(), (3, 23).into(), "string-1"))
+        .add_note(
+            "'Bar' was previously defined here",
+            Some(&Span::new((3, 20).into(), (3, 23).into(), "string-0")),
+        );
+
+        check_diagnostics(diagnostics, [expected]);
+    }
+}