Send an error for unused .proc's

We cannot safely detect all scenarios in which there is dead code, but
we can safely do it for .proc's. Even if they are not called directly,
they might be referenced via jump tables and shenanigans like that. Long
story short, if you are not referencing a .proc in any meaningful way,
then we have dead code.

A pattern could also be given in which a function that was too long has
been splitted into smaller functions which are not called
directly. This is, in my opinion, an anti-pattern (as we generally
expect an rts or a jmp from .proc's), and anyways can be avoided via the
use of __fallthrough__ if the programmer is really set to write this
kind of code.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>

2 files changed, 104 insertions, 42 deletions

diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs
index e36136e..cfc1de2 100644
--- a/lib/xixanta/src/assembler.rs
+++ b/lib/xixanta/src/assembler.rs
@@ -466,21 +466,17 @@ impl<'a> Assembler<'a> {
         }
     }
 
-    // Define a new variable by taking the given `id`. This variable will only
-    // be created if `id` is not empty. The function will error out if the given
-    // name is already taken.
-    fn define_variable(&mut self, node: &PNode) -> Result<(), Error> {
+    // Define a new variable by taking the value on 'node'. This variable will
+    // be of type 'object_type' and will only be created if 'id' is not
+    // empty. The function will error out if the given name is already taken.
+    fn define_variable(&mut self, node: &PNode, object_type: ObjectType) -> Result<(), Error> {
         if node.value.is_empty() {
             return Ok(());
         }
 
         if let Err(message) = self.context.set_variable(
             &node.value,
-            &Object::new(
-                self.current_mapping,
-                self.current_segment,
-                ObjectType::Address,
-            ),
+            &Object::new(self.current_mapping, self.current_segment, object_type),
             false,
         ) {
             return Err(Error {
@@ -526,7 +522,7 @@ impl<'a> Assembler<'a> {
                         });
                         continue;
                     }
-                    if let Err(err) = self.define_variable(node) {
+                    if let Err(err) = self.define_variable(node, ObjectType::Address) {
                         errors.push(err);
                     }
                 }
@@ -701,7 +697,7 @@ impl<'a> Assembler<'a> {
                                     source: self.source_for(node),
                                 });
                                 continue;
-                            } else if let Err(err) = self.define_variable(proc_name) {
+                            } else if let Err(err) = self.define_variable(proc_name, ObjectType::Proc) {
                                 errors.push(err);
                             }
                         }
@@ -799,11 +795,16 @@ impl<'a> Assembler<'a> {
         }
     }
 
-    // Apply the current segment offset to the label identified by `id` unless
-    // it's empty (i.e. anonymous label). In either case, the computed label
-    // will be pushed into the context's list of known labels with the current
-    // segment offset.
-    fn apply_segment_offset_to_label(&mut self, node: &PNode) -> Result<(), Error> {
+    // Apply the current segment offset to the label identified by the given
+    // 'node' and taking 'object_type' as its type, unless the node value is
+    // empty (i.e. anonymous label). In either case, the computed label will be
+    // pushed into the context's list of known labels with the current segment
+    // offset.
+    fn apply_segment_offset_to_label(
+        &mut self,
+        node: &PNode,
+        object_type: ObjectType,
+    ) -> Result<(), Error> {
         let segment = &self.mappings[self.current_mapping].segments[self.current_segment];
         let value = segment.offset.to_le_bytes();
         let object = Object {
@@ -819,7 +820,7 @@ impl<'a> Assembler<'a> {
             node: None,
             mapping: self.current_mapping,
             segment: self.current_segment,
-            object_type: ObjectType::Address,
+            object_type,
             asan_ignore: false,
             asan_reserve: 1,
             accessed: 0,
@@ -854,7 +855,7 @@ impl<'a> Assembler<'a> {
                 // of the offset, the effective address will only be available
                 // after calling `Context::get_variable`
                 NodeType::Label => {
-                    if let Err(e) = self.apply_segment_offset_to_label(node) {
+                    if let Err(e) = self.apply_segment_offset_to_label(node, ObjectType::Address) {
                         errors.push(e);
                     }
                 }
@@ -863,7 +864,8 @@ impl<'a> Assembler<'a> {
                 // then open up its inner context.
                 NodeType::Control(ControlType::StartProc) => {
                     let proc_name = &node.left.as_ref().unwrap();
-                    if let Err(e) = self.apply_segment_offset_to_label(proc_name) {
+                    if let Err(e) = self.apply_segment_offset_to_label(proc_name, ObjectType::Proc)
+                    {
                         errors.push(e);
                     }
                     if let Err(message) = self.context.change_context(node) {
@@ -1168,7 +1170,7 @@ impl<'a> Assembler<'a> {
                 }
 
                 // If it doesn't have the proper prefix, skip as well.
-                if !is_asan_friendly_name(name) {
+                if !is_asan_friendly_name(name) && !matches!(bundle.object_type, ObjectType::Proc) {
                     continue;
                 }
                 let actual_name = name.split("::").last().unwrap_or("");
@@ -1184,19 +1186,44 @@ impl<'a> Assembler<'a> {
                     },
                 };
 
-                // Check if this variable was ever accessed and warn about
-                // it. Even if later it conflicts with another memory range, the
-                // fact that it's not used is not a danger and hence a conflict
-                // does not have to be reported. Hence, skip after issueing the
-                // warning.
+                // Check if this variable/proc was ever accessed.
                 if bundle.accessed == 0 {
-                    self.warnings.push(Error {
-                        line: 0,
-                        message: format!("variable {range} is unused"),
-                        source: self.sources[0].clone(),
-                        expanded_from: self.macro_context.clone(),
-                        global: true,
-                    });
+                    // If this was a .proc definition then we have dead code
+                    // which is a really crappy situation.
+                    if matches!(bundle.object_type, ObjectType::Proc) {
+                        let full_name = if context_name == GLOBAL_CONTEXT {
+                            name.clone()
+                        } else {
+                            format!("{context_name}::{name}")
+                        };
+
+                        errors.push(Error {
+                            line: 0,
+                            message: format!("proc '{full_name}' is unused"),
+                            source: self.sources[0].clone(),
+                            expanded_from: self.macro_context.clone(),
+                            global: true,
+                        });
+                    } else {
+                        // If this was a variable, then warn about it. Even if
+                        // later it conflicts with another memory range, the
+                        // fact that it's not used is not a danger and hence a
+                        // conflict does not have to be reported. Hence, skip
+                        // after issueing the warning.
+                        self.warnings.push(Error {
+                            line: 0,
+                            message: format!("variable {range} is unused"),
+                            source: self.sources[0].clone(),
+                            expanded_from: self.macro_context.clone(),
+                            global: true,
+                        });
+                    }
+                    continue;
+                }
+
+                // We have done everything there was for proc's. Go to the next
+                // iteration if that was the case.
+                if matches!(bundle.object_type, ObjectType::Proc) {
                     continue;
                 }
 
@@ -2798,7 +2825,7 @@ impl<'a> Assembler<'a> {
                     // determine a ZeroPageX or an IndirectX instruction, with a
                     // byte in size of difference).
                     let mut bundle = value.bundle;
-                    if matches!(value.object_type, ObjectType::Address) {
+                    if matches!(value.object_type, ObjectType::Address | ObjectType::Proc) {
                         bundle.size = 2;
                     }
                     Ok(bundle)
@@ -3135,11 +3162,14 @@ impl<'a> Assembler<'a> {
             NodeType::Value => {
                 let name = &node.value.value;
                 if !is_asan_friendly_name(name) {
-                    // If it's referencing an actual ObjectType::Address, then
-                    // let it be (e.g. 'lda palettes, x'; where 'palettes' is a
-                    // legitimate name even if not 'is_asan_friendly_name').
+                    // If it's referencing an actual address, then let it be
+                    // (e.g. 'lda palettes, x'; where 'palettes' is a legitimate
+                    // name even if not 'is_asan_friendly_name').
                     if let Ok(var) = self.context.get_variable(&node.value, &self.mappings) {
-                        if matches!(var.object_type, ObjectType::Address | ObjectType::Argument) {
+                        if matches!(
+                            var.object_type,
+                            ObjectType::Address | ObjectType::Argument | ObjectType::Proc
+                        ) {
                             return Ok(());
                         }
                     }
@@ -3170,12 +3200,18 @@ impl<'a> Assembler<'a> {
                     // If everything failed but because it was an address
                     // (e.g. 'lda palettes + 1, x'), then return early.
                     if let Ok(var) = self.context.get_variable(left_name, &self.mappings) {
-                        if matches!(var.object_type, ObjectType::Address | ObjectType::Argument) {
+                        if matches!(
+                            var.object_type,
+                            ObjectType::Address | ObjectType::Argument | ObjectType::Proc
+                        ) {
                             return Ok(());
                         }
                     }
                     if let Ok(var) = self.context.get_variable(right_name, &self.mappings) {
-                        if matches!(var.object_type, ObjectType::Address | ObjectType::Argument) {
+                        if matches!(
+                            var.object_type,
+                            ObjectType::Address | ObjectType::Argument | ObjectType::Proc
+                        ) {
                             return Ok(());
                         }
                     }
diff --git a/lib/xixanta/src/object.rs b/lib/xixanta/src/object.rs
index cd1db1f..a05c315 100644
--- a/lib/xixanta/src/object.rs
+++ b/lib/xixanta/src/object.rs
@@ -118,10 +118,22 @@ impl Bundle {
 #[derive(Debug, Clone)]
 pub enum ObjectType {
     Address,
+    Proc,
     Value,
     Argument,
 }
 
+impl std::fmt::Display for ObjectType {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        match self {
+            ObjectType::Address => write!(f, "raw address"),
+            ObjectType::Proc => write!(f, "proc"),
+            ObjectType::Value => write!(f, "variable"),
+            ObjectType::Argument => write!(f, "macro argument"),
+        }
+    }
+}
+
 /// Bundle and metadata which is stored on the context table for a given
 /// variable or label.
 #[derive(Debug, Clone)]
@@ -214,7 +226,7 @@ impl Context {
     /// Returns the value of the object represented by the given `id`. Note that
     /// this `id` can be scoped or not, and this function will try to pick the
     /// variable from the right scope. The value itself will be resolved if the
-    /// type is ObjectType::Address.
+    /// type is ObjectType::Address or ObjectType::Proc.
     pub fn get_variable(&mut self, id: &PString, mappings: &[Mapping]) -> Result<Object, String> {
         // First of all, figure out the name of the scope and the real name of
         // the variable. If this was not scoped at all (None case when trying to
@@ -242,14 +254,24 @@ impl Context {
         match self.map.get_mut(scope_name) {
             Some(scope) => match scope.get_mut(var_name) {
                 Some(var) => match var.object_type {
+                    // If it's a value or an argument, then we just account for
+                    // the number of times it was accessed and return it as is.
                     ObjectType::Value | ObjectType::Argument => {
                         var.accessed += 1;
                         Ok(var.clone())
                     }
+                    // If it's a raw address, then we return the resolved value.
                     ObjectType::Address => {
                         let var_to_resolve = var.clone();
                         Ok(self.resolve_label(mappings, &var_to_resolve)?)
                     }
+                    // If it's a .proc, then we account for the number of times
+                    // it was accessed, and we return the resolved value.
+                    ObjectType::Proc => {
+                        var.accessed += 1;
+                        let var_to_resolve = var.clone();
+                        Ok(self.resolve_label(mappings, &var_to_resolve)?)
+                    }
                 },
                 None => {
                     // If it cannot be found, then we have to move up through
@@ -290,9 +312,13 @@ impl Context {
     /// address.
     ///
     /// NOTE: this function asserts that the given `object` is of type
-    /// ObjectType::Address, otherwise it doesn't make sense to call it.
+    /// ObjectType::Address or ObjectType::Proc, otherwise it doesn't make sense
+    /// to call it.
     pub fn resolve_label(&self, mappings: &[Mapping], object: &Object) -> Result<Object, String> {
-        assert!(matches!(object.object_type, ObjectType::Address));
+        assert!(matches!(
+            object.object_type,
+            ObjectType::Address | ObjectType::Proc
+        ));
 
         let mut ret = object.clone();