Warn on pointless (un)conditional branching

Sometimes performing some 'jmp'/'jsr' can be quite pointless, and the
programmer might not be fully aware of this because of the layout of the
code. Imagine:

      .proc foo
            ;; code
            jmp bar
      .endproc

      ;; Documentation, comments, extra space, etc.

      .proc bar
            ;; whatever
      .endproc

The 'jmp' in the code above tries to perform a call stack optimization,
but it's actually not needed because the next instruction after 'jmp' is
the one inside of 'bar', but that's obfuscated because of the layout.

In these sort of cases (and also for 'jsr' and branches) warn the
programmer about it so it can remove that instruction.

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

2 files changed, 88 insertions, 0 deletions

diff --git a/lib/xixanta/src/assembler.rs b/lib/xixanta/src/assembler.rs
index 0d4e132..d7e036f 100644
--- a/lib/xixanta/src/assembler.rs
+++ b/lib/xixanta/src/assembler.rs
@@ -889,11 +889,50 @@ impl<'a> Assembler<'a> {
                     let current = &self.mappings[pn.mapping].segments[pn.segment];
                     bundle.address = current.bundles[pn.bundle_index].address;
 
+                    // If we are trying to 'jmp'/'jsr' right into the next
+                    // instruction, then warn the programmer about it. This
+                    // looks silly but in practice it might happen inside of a
+                    // .proc where the code layout might not make this as
+                    // obvious as it sounds.
+                    //
+                    // NOTE: this is only done for absolute addressing as the
+                    // indirect case for 'jmp' is harder to follow and just not
+                    // worth it. If programmers do fancy indirect jumps, let
+                    // them shoot themselves in the foot if that's what they
+                    // want.
+                    if (bundle.bytes[0] == 0x4C || bundle.bytes[0] == 0x20)
+                        && bundle.arg() as usize == bundle.next_address()
+                    {
+                        self.warnings.push(Error {
+                            line: pn.node.value.line,
+                            message: String::from(
+                                "unconditional jump that points to the next instruction",
+                            ),
+                            source: self.source_for(&pn.node),
+                            global: false,
+                            expanded_from: pn.macro_context.clone(),
+                        });
+                    }
+
                     if pn.node.is_branch() {
                         bundle.resolved = true;
                         if let Err(e) = self.to_relative_address(&pn.node, &mut bundle) {
                             errors.push(e);
                         }
+
+                        // Silly mistake coming from using a label directly
+                        // after the current branch instruction.
+                        if bundle.bytes[1] == 0 {
+                            self.warnings.push(Error {
+                                line: pn.node.value.line,
+                                message: String::from(
+                                    "conditional jump that points to the next instruction",
+                                ),
+                                source: self.source_for(&pn.node),
+                                expanded_from: pn.macro_context.clone(),
+                                global: false,
+                            });
+                        }
                     }
 
                     let current_mut = &mut self.mappings[pn.mapping].segments[pn.segment];
@@ -4066,6 +4105,43 @@ JAL procedure
         assert_eq!(res[1].bytes[0], 0x60);
     }
 
+    #[test]
+    fn jump_to_next() {
+        let res = just_assemble(
+            r#"
+.proc foo
+  jmp bar
+.endproc
+
+.proc bar
+  jsr another
+.endproc
+
+.proc another
+  lda #0
+  beq @next
+@next:
+  rts
+.endproc
+"#,
+        );
+
+        assert_eq!(res.warnings.len(), 3);
+        // NOTE: +3 lines for the implicit header.
+        assert_eq!(
+            res.warnings[0].to_string(),
+            "unconditional jump that points to the next instruction (line 6)"
+        );
+        assert_eq!(
+            res.warnings[1].to_string(),
+            "unconditional jump that points to the next instruction (line 10)"
+        );
+        assert_eq!(
+            res.warnings[2].to_string(),
+            "conditional jump that points to the next instruction (line 15)"
+        );
+    }
+
     // Control statements
 
     #[test]
diff --git a/lib/xixanta/src/object.rs b/lib/xixanta/src/object.rs
index 5904b56..170d872 100644
--- a/lib/xixanta/src/object.rs
+++ b/lib/xixanta/src/object.rs
@@ -98,6 +98,18 @@ impl Bundle {
             ])
         }
     }
+
+    /// Considering that the first element of the 'bytes' property is the
+    /// instruction identifier, returns the two last bytes as if they were a
+    /// 16-bit value.
+    pub fn arg(&self) -> u16 {
+        self.bytes[1] as u16 + ((self.bytes[2] as u16) << 8)
+    }
+
+    /// Returns the address to the next instruction after this bundle.
+    pub fn next_address(&self) -> usize {
+        self.address + self.size as usize
+    }
 }
 
 /// The type of object being referenced, which is either a value as-is, or an