refactor: Simplify converter for statements

The converter of statements would convert a `Statement` to an
`Option<Statement>`. The only case where it would return `None`
was the `CONST` statement, as the `CONST` statement is not needed
after the linter is done.

Keeping the `CONST` in there and ignoring it is not harmful either
though, so it's better to do that, at the benefit of having
a simplified mapping function.

Author: ngeor

rusty_basic/src/instruction_generator/statement.rs

@@ -26,7 +26,11 @@ impl Visitor<StatementPos> for InstructionGenerator {
             Statement::Assignment(left_side, right_side) => {
                 self.generate_assignment_instructions(left_side, right_side, pos)
             }
-            Statement::Const(_, _) => panic!("Constants should have been reduced by const_reducer"),
+            Statement::Const(_, _) => {
+                // The CONST statement does not generate any instructions,
+                // because the linter has replaced expressions that reference constants
+                // with their actual value.
+            }
             Statement::SubCall(n, args) => self.generate_sub_call_instructions(n.at_pos(pos), args),
             Statement::BuiltInSubCall(n, args) => {
                 self.generate_built_in_sub_call_instructions(n, args, pos)

rusty_linter/src/converter/common/traits.rs

@@ -1,5 +1,7 @@
+use rusty_common::{AtPos, Position, Positioned};
+
 use crate::converter::common::Context;
-use crate::core::LintErrorPos;
+use crate::core::{LintErrorPos, LintPosResult};
 
 /// Convert from the current type into the target type O.
 /// By default, O is the same as the current type.
@@ -32,6 +34,24 @@ where
     }
 }
 
+// Blanket implementation for Positioned in combination with the next trait
+
+impl<T, O> Convertible<Positioned<O>> for Positioned<T>
+where
+    T: ConvertibleIn<Position, O>,
+{
+    fn convert(self, ctx: &mut Context) -> Result<Positioned<O>, LintErrorPos> {
+        let Self {
+            element: statement,
+            pos,
+        } = self;
+        statement
+            .convert_in(ctx, pos)
+            .map(|converted| converted.at_pos(pos))
+            .patch_err_pos(&pos)
+    }
+}
+
 /// Convert from the current type into the target type O,
 /// using additional information in the value U.
 /// By default, O is the same as the current type.

rusty_linter/src/converter/dim_rules/param_rules.rs

@@ -1,23 +1,10 @@
 use crate::converter::common::Context;
-use crate::converter::common::Convertible;
 use crate::converter::common::ConvertibleIn;
 use crate::converter::dim_rules::param_type_rules::on_param_type;
 use crate::converter::dim_rules::validation;
 use crate::core::LintErrorPos;
-use crate::core::LintPosResult;
-use rusty_common::AtPos;
 use rusty_common::Position;
-use rusty_parser::{Parameter, ParameterPos};
-
-impl Convertible for ParameterPos {
-    fn convert(self, ctx: &mut Context) -> Result<Self, LintErrorPos> {
-        let Self { element, pos } = self;
-        element
-            .convert_in(ctx, pos)
-            .map(|p| p.at_pos(pos))
-            .patch_err_pos(&pos)
-    }
-}
+use rusty_parser::Parameter;
 
 impl ConvertibleIn<Position> for Parameter {
     fn convert_in(self, ctx: &mut Context, pos: Position) -> Result<Self, LintErrorPos> {

rusty_linter/src/converter/statement/const_rules.rs

@@ -9,7 +9,7 @@ pub fn on_const(
     ctx: &mut Context,
     left_side: NamePos,
     right_side: ExpressionPos,
-) -> Result<(), LintErrorPos> {
+) -> Result<Statement, LintErrorPos> {
     const_cannot_clash_with_existing_names(ctx, &left_side)?;
     new_const(ctx, left_side, right_side)
 }
@@ -38,10 +38,10 @@ fn new_const(
     ctx: &mut Context,
     left_side: NamePos,
     right_side: ExpressionPos,
-) -> Result<(), LintErrorPos> {
+) -> Result<Statement, LintErrorPos> {
     let Positioned {
         element: const_name,
-        ..
+        pos,
     } = left_side;
     let value_before_casting = ctx.names.resolve_const(&right_side)?;
     let value_qualifier = qualifier_of_const_variant(&value_before_casting);
@@ -54,6 +54,12 @@ fn new_const(
     };
     ctx.names
         .names_mut()
-        .insert_const(const_name.into(), final_value);
-    Ok(())
+        .insert_const(const_name.bare_name().clone(), final_value);
+
+    // here we could return the simplified resolved value of the constant
+    // instead of keeping `right_side`.
+    // However the `CONST` statement gets ignored anyway. It stored the resolved
+    // value in the context, so all expressions that reference it
+    // will use it.
+    Ok(Statement::Const(const_name.at_pos(pos), right_side))
 }

rusty_linter/src/converter/statement/main.rs

@@ -3,102 +3,71 @@ use crate::converter::common::Convertible;
 use crate::converter::common::ConvertibleIn;
 use crate::converter::common::{DimContext, ExprContext};
 use crate::converter::statement::{assignment, const_rules};
+use crate::core::NameContext;
 use crate::core::{LintError, LintErrorPos};
-use crate::core::{LintPosResult, NameContext};
 use rusty_common::*;
-use rusty_parser::{ExitObject, Statement, StatementPos, Statements};
+use rusty_parser::{ExitObject, Statement};
 
-impl Convertible<Option<Self>> for StatementPos {
-    fn convert(self, ctx: &mut Context) -> Result<Option<Self>, LintErrorPos> {
-        let Self {
-            element: statement,
-            pos,
-        } = self;
-        statement
-            .convert_in(ctx, pos)
-            .map(|opt_statement| opt_statement.map(|s| s.at_pos(pos)))
-            .patch_err_pos(&pos)
-    }
-}
-
-impl ConvertibleIn<Position, Option<Self>> for Statement {
-    fn convert_in(self, ctx: &mut Context, pos: Position) -> Result<Option<Self>, LintErrorPos> {
+impl ConvertibleIn<Position> for Statement {
+    fn convert_in(self, ctx: &mut Context, pos: Position) -> Result<Self, LintErrorPos> {
         match self {
-            Self::Assignment(n, e) => assignment::on_assignment(n, e, ctx, pos).map(Some),
+            Self::Assignment(n, e) => assignment::on_assignment(n, e, ctx, pos),
             // CONST is mapped to None and is filtered out
-            Self::Const(n, e) => const_rules::on_const(ctx, n, e).map(|_| None),
-            Self::SubCall(n, args) => ctx.sub_call(n, args).map(Some),
+            Self::Const(n, e) => const_rules::on_const(ctx, n, e),
+            Self::SubCall(n, args) => ctx.sub_call(n, args),
             Self::BuiltInSubCall(built_in_sub, args) => {
                 let converted_args = args.convert_in(ctx, ExprContext::Argument)?;
-                Ok(Self::BuiltInSubCall(built_in_sub, converted_args)).map(Some)
+                Ok(Self::BuiltInSubCall(built_in_sub, converted_args))
             }
-            Self::IfBlock(i) => i.convert(ctx).map(Statement::IfBlock).map(Some),
-            Self::SelectCase(s) => s.convert(ctx).map(Statement::SelectCase).map(Some),
-            Self::ForLoop(f) => f.convert(ctx).map(Statement::ForLoop).map(Some),
-            Self::While(c) => c.convert(ctx).map(Statement::While).map(Some),
-            Self::DoLoop(do_loop) => do_loop.convert(ctx).map(Statement::DoLoop).map(Some),
+            Self::IfBlock(i) => i.convert(ctx).map(Statement::IfBlock),
+            Self::SelectCase(s) => s.convert(ctx).map(Statement::SelectCase),
+            Self::ForLoop(f) => f.convert(ctx).map(Statement::ForLoop),
+            Self::While(c) => c.convert(ctx).map(Statement::While),
+            Self::DoLoop(do_loop) => do_loop.convert(ctx).map(Statement::DoLoop),
             Self::Return(opt_label) => {
                 if opt_label.is_some() && ctx.is_in_subprogram() {
                     // cannot have RETURN with explicit label inside subprogram
                     Err(LintError::IllegalInSubFunction.at_no_pos())
                 } else {
-                    Ok(Self::Return(opt_label)).map(Some)
+                    Ok(Self::Return(opt_label))
                 }
             }
             Self::Resume(resume_option) => {
                 if ctx.is_in_subprogram() {
                     Err(LintError::IllegalInSubFunction.at_no_pos())
                 } else {
-                    Ok(Self::Resume(resume_option)).map(Some)
+                    Ok(Self::Resume(resume_option))
                 }
             }
             Self::Exit(exit_object) => match ctx.names.get_name_context() {
                 NameContext::Global => Err(LintError::IllegalOutsideSubFunction.at_no_pos()),
                 NameContext::Sub => {
                     if exit_object == ExitObject::Sub {
-                        Ok(Self::Exit(exit_object)).map(Some)
+                        Ok(Self::Exit(exit_object))
                     } else {
                         Err(LintError::IllegalInSubFunction.at_no_pos())
                     }
                 }
                 NameContext::Function => {
                     if exit_object == ExitObject::Function {
-                        Ok(Self::Exit(exit_object)).map(Some)
+                        Ok(Self::Exit(exit_object))
                     } else {
                         Err(LintError::IllegalInSubFunction.at_no_pos())
                     }
                 }
             },
-            Self::Dim(dim_list) => dim_list
-                .convert_in_default(ctx)
-                .map(Statement::Dim)
-                .map(Some),
+            Self::Dim(dim_list) => dim_list.convert_in_default(ctx).map(Statement::Dim),
             Self::Redim(dim_list) => dim_list
                 .convert_in(ctx, DimContext::Redim)
-                .map(Statement::Redim)
-                .map(Some),
-            Self::Print(print) => print.convert(ctx).map(Statement::Print).map(Some),
+                .map(Statement::Redim),
+            Self::Print(print) => print.convert(ctx).map(Statement::Print),
             Self::OnError(_)
             | Self::Label(_)
             | Self::GoTo(_)
             | Self::GoSub(_)
             | Self::Comment(_)
             | Self::End
-            | Self::System => Ok(self).map(Some),
+            | Self::System => Ok(self),
         }
     }
 }
-
-impl Convertible for Statements {
-    fn convert(self, ctx: &mut Context) -> Result<Self, LintErrorPos> {
-        self.into_iter()
-            .map(|s| s.convert(ctx))
-            .map(|res| match res {
-                Ok(Some(s)) => Some(Ok(s)),
-                Err(e) => Some(Err(e)),
-                Ok(None) => None,
-            })
-            .flat_map(|opt| opt.into_iter())
-            .collect()
-    }
-}

rusty_linter/src/post_linter/expression_reducer.rs

@@ -110,7 +110,12 @@ pub trait ExpressionReducer {
                         Statement::Assignment(reduced_left, reduced_right)
                     })
             }
-            Statement::Const(left, _) => panic!("Linter should have removed Const {:?}", left),
+            Statement::Const(left, right) => {
+                // The converter is smart enough to replace Expressions that reference
+                // constants with their actual value, so the `CONST` statement isn't used
+                // any further.
+                Ok(Statement::Const(left, right))
+            }
             Statement::SubCall(b, e) => self
                 .visit_sub_call(b, e)
                 .map(|(reduced_name, reduced_expr)| Statement::SubCall(reduced_name, reduced_expr)),

rusty_linter/src/post_linter/post_conversion_linter.rs

@@ -65,9 +65,6 @@ pub trait PostConversionLinter {
     fn visit_statement(&mut self, s: &Statement) -> Result<(), LintErrorPos> {
         match s {
             Statement::Assignment(left, right) => self.visit_assignment(left, right),
-            Statement::Const(left, _) => {
-                panic!("Linter should have removed Const statements {:?}", left)
-            }
             Statement::SubCall(b, e) => self.visit_sub_call(b, e),
             Statement::BuiltInSubCall(b, e) => self.visit_built_in_sub_call(b, e),
             Statement::IfBlock(i) => self.visit_if_block(i),
@@ -85,7 +82,7 @@ pub trait PostConversionLinter {
             Statement::Resume(resume_option) => self.visit_resume(resume_option),
             Statement::Return(opt_label) => self.visit_return(opt_label.as_ref()),
             Statement::Exit(exit_object) => self.visit_exit(*exit_object),
-            Statement::End | Statement::System => Ok(()),
+            Statement::Const(_, _) | Statement::End | Statement::System => Ok(()),
         }
     }
 

rusty_linter/src/tests/constant.rs

@@ -3,10 +3,7 @@ use crate::assert_linter_ok_global_statements;
 use crate::core::LintError;
 use crate::tests::test_utils::linter_ok;
 use rusty_common::*;
-use rusty_parser::{
-    BareName, BuiltInStyle, Expression, GlobalStatement, ParamType, Parameter, Print, Statement,
-    SubImplementation, TypeQualifier,
-};
+use rusty_parser::*;
 
 #[test]
 fn function_call_not_allowed() {
@@ -109,6 +106,10 @@ fn test_constant_definition_and_usage_in_print() {
     "#;
     assert_linter_ok_global_statements!(
         program,
+        Statement::Const(
+            Name::from("X").at_rc(2, 11),
+            Expression::StringLiteral("hello".to_owned()).at_rc(2, 15)
+        ),
         Statement::Print(Print::one(
             Expression::StringLiteral("hello".to_owned()).at_rc(3, 11)
         ))
@@ -127,6 +128,11 @@ fn test_constant_definition_and_usage_in_sub_call_arg() {
     assert_eq!(
         linter_ok(program),
         vec![
+            GlobalStatement::Statement(Statement::Const(
+                Name::from("X").at_rc(2, 11),
+                Expression::StringLiteral("hello".to_owned()).at_rc(2, 15)
+            ),)
+            .at_rc(2, 5),
             GlobalStatement::Statement(Statement::SubCall(
                 "MySub".into(),
                 vec![Expression::StringLiteral("hello".to_owned()).at_rc(3, 11)]