refactor: Migrated UserDefinedNamesCollector to the new Visitor pattern

Author: ngeor

rusty_linter/src/core/visitor.rs

@@ -61,9 +61,12 @@ where
     }
 }
 
-/// Creates a delegate visitor for the given type T.
-pub trait DelegateVisitor<T> {
-    fn delegate(&mut self) -> impl Visitor<T>;
+/// A visitor that delegates some functionality to another visitor.
+pub trait DelegateVisitor<P> {
+    /// Returns the delegate visitor.
+    /// Typically called after the visit is done, in order to extract
+    /// information from the underlying visitor.
+    fn delegate(self) -> P;
 }
 
 delegate_visitor!(
@@ -257,12 +260,6 @@ macro_rules! delegate_visitor {
             pub fn new(delegate: P) -> Self {
                 Self { delegate }
             }
-
-            /// Returns the delegate back.
-            #[allow(unused)]
-            pub fn delegate(self) -> P {
-                self.delegate
-            }
         }
 
         impl<P> SetPosition for $name<P>
@@ -273,5 +270,11 @@ macro_rules! delegate_visitor {
                 self.delegate.set_position(pos);
             }
         }
+
+        impl<P> DelegateVisitor<P> for $name<P> {
+            fn delegate(self) -> P {
+                self.delegate
+            }
+        }
     };
 }

rusty_linter/src/post_linter/dots_linter.rs

@@ -2,10 +2,21 @@ use super::post_conversion_linter::PostConversionLinter;
 use rusty_common::*;
 use rusty_parser::*;
 
-use crate::core::LintResult;
-use crate::core::{LintError, LintErrorPos};
+use crate::core::*;
+use crate::{no_op_visitor, no_pos_visitor};
 use std::collections::HashSet;
 
+/// In QBasic, if you have anywhere in the program a variable of user defined type,
+/// e.g. `DIM A AS Person`,
+/// then you can't have anywhere else in the program a variable that starts with
+/// that name followed by a dot,
+/// e.g. `DIM A.B$` is illegal.
+///
+/// The QBasic behavior is probably done at the parsing level,
+/// but here we catch it at the linter.
+/// QBasic's behavior is to throw the error:
+/// "Expected: , or end-of-statement"
+/// and the error location is reported at the dot.
 #[derive(Default)]
 pub struct DotsLinter {
     user_defined_names: HashSet<CaseInsensitiveString>,
@@ -128,9 +139,9 @@ impl NoDotNamesCheck<(&Expression, Position), LintErrorPos> for DotsLinter {
 
 impl PostConversionLinter for DotsLinter {
     fn visit_program(&mut self, p: &Program) -> Result<(), LintErrorPos> {
-        let mut collector = UserDefinedNamesCollector::default();
-        collector.visit_program(p)?;
-        self.user_defined_names = collector.user_defined_names;
+        let mut collector = UserDefinedNamesCollector::visitor();
+        collector.visit(p)?;
+        self.user_defined_names = collector.delegate().user_defined_names;
         self.visit_global_statements(p)
     }
 
@@ -177,43 +188,46 @@ impl PostConversionLinter for DotsLinter {
     }
 }
 
+/// Collects names of parameters or variables that are of a user defined type.
 #[derive(Default)]
 struct UserDefinedNamesCollector {
     user_defined_names: HashSet<CaseInsensitiveString>,
 }
 
+no_op_visitor!(UserDefinedNamesCollector: DefType, FunctionDeclaration, SubDeclaration, UserDefinedType);
+no_pos_visitor!(UserDefinedNamesCollector);
+
 impl UserDefinedNamesCollector {
-    fn visit_names<T>(&mut self, params: &Vec<Positioned<TypedName<T>>>)
-    where
-        T: VarTypeToUserDefinedRecursively,
-    {
-        self.user_defined_names.extend(
-            params
-                .iter()
-                .map(|dim_var_pos| &dim_var_pos.element)
-                .filter(|dim_name| dim_name.var_type.as_user_defined_recursively().is_some())
-                .map(|dim_name| &dim_name.bare_name)
-                .cloned(),
-        );
-    }
-}
-
-impl PostConversionLinter for UserDefinedNamesCollector {
-    fn visit_function_implementation(
-        &mut self,
-        f: &FunctionImplementation,
-    ) -> Result<(), LintErrorPos> {
-        self.visit_names(&f.params);
-        self.visit_statements(&f.body)
+    pub fn visitor() -> impl Visitor<Program> + DelegateVisitor<Self> + SetPosition {
+        DeepStatementVisitor::new(Self::default())
     }
+}
 
-    fn visit_sub_implementation(&mut self, s: &SubImplementation) -> Result<(), LintErrorPos> {
-        self.visit_names(&s.params);
-        self.visit_statements(&s.body)
+impl<T> Visitor<SubprogramImplementation<T>> for UserDefinedNamesCollector {
+    fn visit(&mut self, element: &SubprogramImplementation<T>) -> VisitResult {
+        self.visit(&element.params)
     }
+}
 
-    fn visit_dim(&mut self, dim_list: &DimList) -> Result<(), LintErrorPos> {
-        self.visit_names(&dim_list.variables);
+impl Visitor<Statement> for UserDefinedNamesCollector {
+    fn visit(&mut self, element: &Statement) -> VisitResult {
+        match element {
+            Statement::Dim(dim_list) | Statement::Redim(dim_list) => {
+                self.visit(&dim_list.variables)
+            }
+            _ => Ok(()),
+        }
+    }
+}
+
+impl<T> Visitor<TypedName<T>> for UserDefinedNamesCollector
+where
+    T: VarTypeToUserDefinedRecursively,
+{
+    fn visit(&mut self, element: &TypedName<T>) -> VisitResult {
+        if element.var_type.as_user_defined_recursively().is_some() {
+            self.user_defined_names.insert(element.bare_name.clone());
+        }
         Ok(())
     }
 }

rusty_linter/src/pre_linter/main.rs

@@ -118,21 +118,22 @@ impl Visitor<Statement> for MainContext {
 
 impl Visitor<UserDefinedType> for MainContext {
     fn visit(&mut self, user_defined_type: &UserDefinedType) -> VisitResult {
-        self.delegate().visit(user_defined_type)
+        self.user_defined_types_visitor().visit(user_defined_type)
     }
 }
 
-impl DelegateVisitor<UserDefinedType> for MainContext {
-    fn delegate(&mut self) -> impl Visitor<UserDefinedType> {
+impl MainContext {
+    fn user_defined_types_visitor(&mut self) -> impl Visitor<UserDefinedType> + use<'_> {
         super::user_defined_type_visitor::UserDefinedTypeVisitor::new(
             &mut self.user_defined_types,
             self.declaration_pos,
             &self.global_constants,
         )
     }
-}
 
-impl MainContext {
+    // TODO the remaining logic regarding parameters is a different kind of Visitor
+    // with a possible signature visit(&mut self, element: &T) -> Result<U, LintErrorPos>
+
     fn on_parameters(&self, parameters: &Parameters) -> Result<ResolvedParamTypes, LintErrorPos> {
         parameters
             .iter()