Refer to the original file for unused objects

When warning users on unused objects (e.g. variables, proc's), we have
to pick up the source of origin for this warning. In places where the
original PNode is not available, then we go with the last source we have
at hand, as that's the usual way to go (i.e. the error occurred at the
current source/context).

That being said, for unused objects that's not desirable, because we
might otherwise claim the error to happen on the file we first
targetted, but it's way more useful to understand where the object was
defined.

Hence, when defining a variable, address or proc, let's actually store
the PNode associated with it as well. This way the checker can hopefully
get the source from this PNode and be more clear to the user.

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

6 files changed, 49 insertions, 22 deletions

diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs
index c53f143..e8ecd06 100644
--- a/lib/xixanta/src/assembler.rs
+++ b/lib/xixanta/src/assembler.rs
@@ -495,7 +495,16 @@ impl<'a> Assembler<'a> {
 
         if let Err(message) = self.context.set_variable(
             &node.value,
-            &Object::new(self.current_mapping, self.current_segment, object_type),
+            &Object {
+                bundle: Bundle::default(),
+                node: Some(node.clone()),
+                mapping: self.current_mapping,
+                segment: self.current_segment,
+                object_type,
+                asan_ignore: false,
+                asan_reserve: 1,
+                accessed: 0,
+            },
             false,
         ) {
             return Err(Error {
@@ -619,7 +628,7 @@ impl<'a> Assembler<'a> {
                                 &node.value,
                                 &Object {
                                     bundle: value,
-                                    node: None,
+                                    node: Some(node.clone()),
                                     mapping: self.current_mapping,
                                     segment: self.current_segment,
                                     object_type: ObjectType::Value,
@@ -1199,13 +1208,24 @@ impl<'a> Assembler<'a> {
                 // (e.g. nasm's '--allow-unused' flag), or the magic
                 // 'asan:ignore' comment is given.
                 if !self.allow_unused && !bundle.asan_ignore && bundle.accessed == 0 {
+                    // For unused stuff we need to point to the source of the
+                    // definition, not the current context. This is usually
+                    // provided on the 'bundle.node' member, but we can always
+                    // pick the first source if that's not provided for some
+                    // unknown reason.
+                    let source = self.sources[match &bundle.node {
+                        Some(node) => node.source,
+                        None => 0,
+                    }]
+                    .clone();
+
                     // If this was a .proc definition then we are certain that
                     // this is dead code, which is a really crappy situation.
                     if matches!(bundle.object_type, ObjectType::Proc) {
                         errors.push(Error {
                             line: 0,
                             message: format!("proc '{full_name}' is unused"),
-                            source: self.sources[0].clone(),
+                            source,
                             expanded_from: self.macro_context.clone(),
                             global: true,
                         });
@@ -1216,7 +1236,7 @@ impl<'a> Assembler<'a> {
                         self.warnings.push(Error {
                             line: 0,
                             message: format!("{} '{full_name}' is unused", bundle.object_type),
-                            source: self.sources[0].clone(),
+                            source,
                             expanded_from: self.macro_context.clone(),
                             global: true,
                         });
diff --git a/lib/xixanta/src/object.rs b/lib/xixanta/src/object.rs
index 1887b77..36b15dc 100644
--- a/lib/xixanta/src/object.rs
+++ b/lib/xixanta/src/object.rs
@@ -142,8 +142,10 @@ pub struct Object {
     pub bundle: Bundle,
 
     /// Node which marks the source of the computed `bundle` attribute. This is
-    /// only provided in cases like macro calls where at the crunching stage we
-    /// might need to fetch previous context for the current `bundle` value.
+    /// provided in cases like macro calls where at the crunching stage we might
+    /// need to fetch previous context for the current `bundle` value. It is
+    /// also provided in variable definitions, as an indirect way to fetch the
+    /// original source containing the definition.
     pub node: Option<PNode>,
 
     /// The mapping index where the object was found. Note that this index
@@ -169,22 +171,6 @@ pub struct Object {
     pub accessed: usize,
 }
 
-impl Object {
-    /// Create a default bundle with the given metadata parameters.
-    pub fn new(mapping: usize, segment: usize, object_type: ObjectType) -> Self {
-        Self {
-            bundle: Bundle::default(),
-            node: None,
-            mapping,
-            segment,
-            object_type,
-            asan_ignore: false,
-            asan_reserve: 1,
-            accessed: 0,
-        }
-    }
-}
-
 /// Context holds information about the different scopes being defined, the
 /// current scope, and has a map of all the variables defined for each scope.
 #[derive(Debug)]
diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh
index ae06e72..524c75e 100755
--- a/scripts/test-e2e.sh
+++ b/scripts/test-e2e.sh
@@ -114,6 +114,11 @@ exit_code=$((exit_code + $?))
 diff tests/out/delayed_macro_arguments.nes tests/expected/delayed_macro_arguments.nes
 exit_code=$((exit_code + $?))
 
+echo "test: custom => unused_definition.nes"
+./target/debug/nasm -c empty -Werror --asan tests/unused_definition.s -o /dev/null 2>tests/out/unused_definition.txt
+diff tests/out/unused_definition.txt tests/expected/unused_definition.txt
+exit_code=$((exit_code + $?))
+
 ##
 # code.nes
 
diff --git a/tests/def.s b/tests/def.s
new file mode 100644
index 0000000..a9327d4
--- /dev/null
+++ b/tests/def.s
@@ -0,0 +1,2 @@
+
+Var = 1
diff --git a/tests/expected/unused_definition.txt b/tests/expected/unused_definition.txt
new file mode 100644
index 0000000..8e851b0
--- /dev/null
+++ b/tests/expected/unused_definition.txt
@@ -0,0 +1 @@
+error: variable 'Var' is unused (def.s)
diff --git a/tests/unused_definition.s b/tests/unused_definition.s
new file mode 100644
index 0000000..144d3dd
--- /dev/null
+++ b/tests/unused_definition.s
@@ -0,0 +1,13 @@
+.segment "HEADER"
+    .byte 'N', 'E', 'S', $1A
+    .byte $02, $01
+    .byte $00
+    .byte $00
+
+.segment "CODE"
+
+;;; asan:stack full
+
+.include "def.s"
+
+nop