From 19eda496538f0ce019400c6fe675ffb1fa74dfb4 Mon Sep 17 00:00:00 2001
From: Jeroen Engels <jfm.engels@gmail.com>
Date: Tue, 9 Apr 2024 11:03:35 +0200
Subject: [PATCH] Fix module name lookup table giving a wrong result

Let destructuring variables that shadowed imports were not correctly
registered, and looking them up could lead to incorrectly point to
the imported module that was shadowed.

Thanks @matzko for discovering this.
---
 CHANGELOG.md                                 |  4 ++++
 src/Review/ModuleNameLookupTable/Compute.elm | 10 +++++-----
 tests/ModuleNameLookupTableTest.elm          | 13 +++++++++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2a4236b23..58753929a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+## [2.13.2] - 2024-04-09
+
+Fixed an issue where the result of a module name lookup for let variables was sometimes incorrect.
+
 ## [2.13.1] - 2023-06-17
 
 Fixed an issue where the module name lookup table would yield an incorrect result. [#159](https://github.com/jfmengels/elm-review/pull/159)
diff --git a/src/Review/ModuleNameLookupTable/Compute.elm b/src/Review/ModuleNameLookupTable/Compute.elm
index 4045fd34a..70f26c699 100644
--- a/src/Review/ModuleNameLookupTable/Compute.elm
+++ b/src/Review/ModuleNameLookupTable/Compute.elm
@@ -1215,7 +1215,7 @@ popScopeExit node context =
 expressionEnterVisitor : Node Expression -> Context -> Context
 expressionEnterVisitor node context =
     case Node.value node of
-        Expression.LetExpression { declarations } ->
+        Expression.LetExpression letExpression ->
             let
                 newContext : Context
                 newContext =
@@ -1246,11 +1246,11 @@ expressionEnterVisitor node context =
                                         in
                                         NonEmpty.mapHead (\scope -> { scope | cases = ( expression, names ) :: scope.cases }) withLetVariable
 
-                                Expression.LetDestructuring _ _ ->
-                                    scopes
+                                Expression.LetDestructuring pattern _ ->
+                                    NonEmpty.mapHead (\scope -> { scope | names = collectNamesFromPattern LetVariable [ pattern ] scope.names }) scopes
                         )
                         (NonEmpty.cons emptyScope context.scopes)
-                        declarations
+                        letExpression.declarations
                         |> updateScope context
 
                 lookupTable : ModuleNameLookupTable
@@ -1280,7 +1280,7 @@ expressionEnterVisitor node context =
                                     collectModuleNamesFromPattern newContext [ pattern ] acc
                         )
                         newContext.lookupTable
-                        declarations
+                        letExpression.declarations
             in
             { newContext | lookupTable = lookupTable }
 
diff --git a/tests/ModuleNameLookupTableTest.elm b/tests/ModuleNameLookupTableTest.elm
index ea1f6d65b..034a8c64d 100644
--- a/tests/ModuleNameLookupTableTest.elm
+++ b/tests/ModuleNameLookupTableTest.elm
@@ -103,6 +103,7 @@ a = localValue
  ("x" </> "y")
  (|=)
  ("pars" |= "er")
+ (let (exposedElement, _) = 1 in exposedElement)
 b = case () of
   VariantA -> ()
   (ExposesEverything.VariantA as foo) -> foo
@@ -167,6 +168,7 @@ Cmd.none -> Platform.Cmd.none
 <nothing>.</> -> Url.Parser.</>
 <nothing>.|= -> Parser.Advanced.|=
 <nothing>.|= -> Parser.Advanced.|=
+<nothing>.exposedElement -> <nothing>.exposedElement
 <nothing>.VariantA -> ExposesEverything.VariantA
 ExposesEverything.VariantA -> ExposesEverything.VariantA
 ExposesEverythingAlias.VariantA -> ExposesEverything.VariantA
@@ -316,6 +318,7 @@ a = localValue
  ("x" </> "y")
  (|=)
  ("pars" |= "er")
+ (let (exposedElement, _) = 1 in exposedElement)
 b = case () of
   VariantA -> ()
   (ExposesEverything.VariantA as foo) -> foo
@@ -378,6 +381,7 @@ Cmd.none -> Platform.Cmd.none
 <nothing>.</> -> Url.Parser.</>
 <nothing>.|= -> Parser.Advanced.|=
 <nothing>.|= -> Parser.Advanced.|=
+<nothing>.exposedElement -> Abc.Xyz.exposedElement
 <nothing>.VariantA -> ExposesEverything.VariantA
 ExposesEverything.VariantA -> ExposesEverything.VariantA
 ExposesEverythingAlias.VariantA -> ExposesEverything.VariantA
@@ -698,6 +702,15 @@ collectPatterns lookupFunction context node =
         Pattern.VarPattern _ ->
             []
 
+        Pattern.TuplePattern subPatterns ->
+            List.concatMap (collectPatterns lookupFunction context) subPatterns
+
+        Pattern.RecordPattern subPatterns ->
+            List.map Node.value subPatterns
+
+        Pattern.AllPattern ->
+            []
+
         _ ->
             Debug.todo ("Other patterns in case expressions are not handled: " ++ Debug.toString node)