From c6190b51acb34668b2dbec9f9c9af007e9178dc4 Mon Sep 17 00:00:00 2001
From: disinvite <disinvite@users.noreply.github.com>
Date: Sun, 17 Dec 2023 17:15:00 -0500
Subject: [PATCH] Linter warning for offset reuse in codebase

---
 tools/isledecomp/isledecomp/parser/linter.py | 36 +++++++++++++++++---
 tools/isledecomp/tests/test_linter.py        | 29 ++++++++++++++++
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/tools/isledecomp/isledecomp/parser/linter.py b/tools/isledecomp/isledecomp/parser/linter.py
index 99c8e272..54406ce9 100644
--- a/tools/isledecomp/isledecomp/parser/linter.py
+++ b/tools/isledecomp/isledecomp/parser/linter.py
@@ -1,6 +1,7 @@
 from typing import List, Optional
 from .parser import DecompParser
 from .error import ParserAlert, ParserError
+from .node import ParserSymbol
 
 
 def get_checkorder_filter(module):
@@ -14,16 +15,39 @@ def __init__(self) -> None:
         self._parser = DecompParser()
         self._filename: str = ""
         self._module: Optional[str] = None
+        # Set of (str, int) tuples for each module/offset pair seen while scanning.
+        # This is _not_ reset between files and is intended to report offset reuse
+        # when scanning the entire directory.
+        self._offsets_used = set()
 
-    def reset(self):
+    def reset(self, full_reset: bool = False):
         self.alerts = []
         self._parser.reset()
         self._filename = ""
         self._module = None
 
+        if full_reset:
+            self._offsets_used.clear()
+
     def file_is_header(self):
         return self._filename.lower().endswith(".h")
 
+    def _load_offsets_from_list(self, marker_list: List[ParserSymbol]):
+        """Helper for loading (module, offset) tuples while the DecompParser
+        has them broken up into three different lists."""
+        for marker in marker_list:
+            value = (marker.module, marker.offset)
+            if value in self._offsets_used:
+                self.alerts.append(
+                    ParserAlert(
+                        code=ParserError.DUPLICATE_OFFSET,
+                        line_number=marker.line_number,
+                        line=f"0x{marker.offset:08x}",
+                    )
+                )
+            else:
+                self._offsets_used.add(value)
+
     def _check_function_order(self):
         """Rules:
         1. Only markers that are implemented in the file are considered. This means we
@@ -55,8 +79,9 @@ def _check_function_order(self):
             last_offset = fun.offset
 
     def _check_offset_uniqueness(self):
-        # TODO
-        pass
+        self._load_offsets_from_list(self._parser.functions)
+        self._load_offsets_from_list(self._parser.vtables)
+        self._load_offsets_from_list(self._parser.variables)
 
     def _check_byname_allowed(self):
         if self.file_is_header():
@@ -75,7 +100,7 @@ def check_lines(self, lines, filename, module=None):
         """`lines` is a generic iterable to allow for testing with a list of strings.
         We assume lines has the entire contents of the compilation unit."""
 
-        self.reset()
+        self.reset(False)
         self._filename = filename
         self._module = module
 
@@ -84,9 +109,10 @@ def check_lines(self, lines, filename, module=None):
         self._parser.finish()
         self.alerts = self._parser.alerts[::]
 
+        self._check_offset_uniqueness()
+
         if self._module is not None:
             self._check_byname_allowed()
-            self._check_offset_uniqueness()
 
             if not self.file_is_header():
                 self._check_function_order()
diff --git a/tools/isledecomp/tests/test_linter.py b/tools/isledecomp/tests/test_linter.py
index cb3b8f27..84ce6495 100644
--- a/tools/isledecomp/tests/test_linter.py
+++ b/tools/isledecomp/tests/test_linter.py
@@ -70,6 +70,7 @@ def test_module_isolation(linter):
     ]
 
     assert linter.check_lines(lines, "test.cpp", "TEST") is True
+    linter.reset(True)
     assert linter.check_lines(lines, "test.cpp", "ALPHA") is True
 
 
@@ -81,5 +82,33 @@ def test_byname_headers_only(linter):
     ]
 
     assert linter.check_lines(lines, "test.h", "TEST") is True
+    linter.reset(True)
     assert linter.check_lines(lines, "test.cpp", "TEST") is False
     assert linter.alerts[0].code == ParserError.BYNAME_FUNCTION_IN_CPP
+
+
+def test_duplicate_offsets(linter):
+    """The linter will retain module/offset pairs found until we do a full reset."""
+    lines = [
+        "// FUNCTION: TEST 0x1000",
+        "// FUNCTION: HELLO 0x1000",
+        "// MyClass::~MyClass",
+    ]
+
+    # Should not fail for duplicate offset 0x1000 because the modules are unique.
+    assert linter.check_lines(lines, "test.h", "TEST") is True
+
+    # Simulate a failure by reading the same file twice.
+    assert linter.check_lines(lines, "test.h", "TEST") is False
+
+    # Two errors because offsets from both modules are duplicated
+    assert len(linter.alerts) == 2
+    assert all(a.code == ParserError.DUPLICATE_OFFSET for a in linter.alerts)
+
+    # Partial reset will retain the list of seen offsets.
+    linter.reset(False)
+    assert linter.check_lines(lines, "test.h", "TEST") is False
+
+    # Full reset will forget seen offsets.
+    linter.reset(True)
+    assert linter.check_lines(lines, "test.h", "TEST") is True