From 22e3a7b8df864fd1e03b2e3dfefb30cb2d187aad Mon Sep 17 00:00:00 2001
From: Varun Gandhi <varun.gandhi@sourcegraph.com>
Date: Fri, 5 Aug 2022 08:28:29 +0800
Subject: [PATCH] fix: Fix ENFORCE hit due to cache reuse across functions.

For some reason, I haven't been able to reproduce this
by creating similar files but with slightly different
function and variable names (a, b, c etc.). I don't
quite understand why that didn't work, but this reliably
fixes some crashes when indexing Homebrew/brew.
---
 scip_indexer/SCIPIndexer.cc | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/scip_indexer/SCIPIndexer.cc b/scip_indexer/SCIPIndexer.cc
index 18d8fe05d4..b30d6f0cd4 100644
--- a/scip_indexer/SCIPIndexer.cc
+++ b/scip_indexer/SCIPIndexer.cc
@@ -437,10 +437,23 @@ enum class Emitted {
     Earlier,
 };
 
+using OccurrenceCache = UnorderedMap<std::pair<core::LocOffsets, /*SymbolRole*/ int32_t>, uint32_t>;
+
 class SCIPState {
     string symbolScratchBuffer;
     UnorderedMap<NamedSymbolRef, string> symbolStringCache;
-    UnorderedMap<uint32_t, core::TypePtr> localTypes;
+
+    // Cache of occurrences for locals that have been emitted in this function.
+    //
+    // Note that the SymbolRole is a part of the key too, because we can
+    // have a read-reference and write-reference at the same location
+    // (we don't merge those for now).
+    //
+    // The 'value' in the map is purely for sanity-checking. It's a bit
+    // cumbersome to conditionalize the type to be a set in non-debug and
+    // map in debug, so keeping it a map.
+    UnorderedMap<std::pair<core::LocOffsets, /*SymbolRole*/ int32_t>, uint32_t> localOccurrenceCache;
+
     GemMetadata gemMetadata;
 
 public:
@@ -454,16 +467,6 @@ class SCIPState {
     UnorderedSet<std::pair<core::FileRef, std::string>> emittedSymbols;
     UnorderedMap<core::FileRef, vector<scip::SymbolInformation>> symbolMap;
 
-    // Cache of occurrences for locals that have been emitted in this function.
-    //
-    // Note that the SymbolRole is a part of the key too, because we can
-    // have a read-reference and write-reference at the same location
-    // (we don't merge those for now).
-    //
-    // The 'value' in the map is purely for sanity-checking. It's a bit
-    // cumbersome to conditionalize the type to be a set in non-debug and
-    // map in debug, so keeping it a map.
-    UnorderedMap<std::pair<core::LocOffsets, /*SymbolRole*/ int32_t>, uint32_t> localOccurrenceCache;
     vector<scip::Document> documents;
     vector<scip::SymbolInformation> externalSymbols;
 
@@ -476,6 +479,10 @@ class SCIPState {
     SCIPState(const SCIPState &) = delete;
     SCIPState &operator=(const SCIPState &other) = delete;
 
+    void clearFunctionLocalCaches() {
+        this->localOccurrenceCache.clear();
+    }
+
     // If the returned value is as success, the pointer is non-null.
     //
     // The argument symbol is used instead of recomputing from scratch if it is non-null.
@@ -1348,8 +1355,10 @@ class SCIPSemanticExtension : public SemanticExtension {
         // specific to a range, so directly using that would lead to recomputing local variable
         // information repeatedly for each occurrence.
 
-        sorbet::scip_indexer::CFGTraversal traversal(*scipState.get(), core::Context(gs, methodDef.symbol, file));
+        auto &scipStateRef = *scipState.get();
+        sorbet::scip_indexer::CFGTraversal traversal(scipStateRef, core::Context(gs, methodDef.symbol, file));
         traversal.traverse(cfg);
+        scipStateRef.clearFunctionLocalCaches();
     }
 
     virtual unique_ptr<SemanticExtension> deepCopy(const core::GlobalState &from, core::GlobalState &to) override {