Merge pull request #5324 from unisonweb/fix-5323

bugfix: fix suffixification logic in upgrade

parser-typechecker/src/Unison/PrettyPrintEnvDecl/Names.hs

@@ -1,11 +1,8 @@
 module Unison.PrettyPrintEnvDecl.Names
   ( makePPED,
-    makeFilePPED,
-    makeCodebasePPED,
   )
 where
 
-import Unison.Names (Names)
 import Unison.PrettyPrintEnv.Names qualified as PPE
 import Unison.PrettyPrintEnvDecl (PrettyPrintEnvDecl (PrettyPrintEnvDecl))
 
@@ -14,23 +11,3 @@ makePPED namer suffixifier =
   PrettyPrintEnvDecl
     (PPE.makePPE namer PPE.dontSuffixify)
     (PPE.makePPE namer suffixifier)
-
--- | Make a PPED suitable for names in a Unison file.
---
--- Such names have special suffixification rules: aliases may *not* be referred to by a common suffix. For example, if
--- a file contains
---
---   one.foo = 6
---   two.foo = 6
---
--- then the suffix `foo` will *not* be accepted (currently). So, this PPE uses the "suffixify by name" strategy.
-makeFilePPED :: Names -> PrettyPrintEnvDecl
-makeFilePPED names =
-  makePPED (PPE.namer names) (PPE.suffixifyByName names)
-
--- | Make a PPED suitable for names in the codebase. These names are hash qualified and suffixified by hash.
-makeCodebasePPED :: Names -> PrettyPrintEnvDecl
-makeCodebasePPED names =
-  makePPED
-    (PPE.hqNamer 10 names)
-    (PPE.suffixifyByHash names)

unison-cli/src/Unison/Codebase/Editor/HandleInput/Upgrade.hs

@@ -59,7 +59,7 @@ import Unison.PrettyPrintEnv qualified as PPE
 import Unison.PrettyPrintEnv.Names qualified as PPE
 import Unison.PrettyPrintEnvDecl (PrettyPrintEnvDecl (..))
 import Unison.PrettyPrintEnvDecl qualified as PPED (addFallback)
-import Unison.PrettyPrintEnvDecl.Names qualified as PPED (makeCodebasePPED, makeFilePPED)
+import Unison.PrettyPrintEnvDecl.Names qualified as PPED (makePPED)
 import Unison.Project (ProjectBranchName)
 import Unison.Reference (TermReference, TermReferenceId, TypeReference, TypeReferenceId)
 import Unison.Reference qualified as Reference
@@ -162,15 +162,23 @@ handleUpgrade oldName newName = do
           UnisonFile.emptyUnisonFile
       pure
         ( unisonFile,
-          makeOldDepPPE
-            oldName
-            newName
-            currentDeepNamesSansOld
-            (Branch.toNames oldNamespace)
-            (Branch.toNames oldLocalNamespace)
-            (Branch.toNames newLocalNamespace)
-            `PPED.addFallback` PPED.makeFilePPED (Names.fromReferenceIds dependents)
-            `PPED.addFallback` PPED.makeCodebasePPED currentDeepNamesSansOld
+          let ppe1 =
+                makeOldDepPPE
+                  oldName
+                  newName
+                  currentDeepNamesSansOld
+                  (Branch.toNames oldNamespace)
+                  (Branch.toNames oldLocalNamespace)
+                  (Branch.toNames newLocalNamespace)
+              ppe2 =
+                PPED.makePPED
+                  (PPE.namer (Names.fromReferenceIds dependents))
+                  (PPE.suffixifyByName currentDeepNamesSansOld)
+              ppe3 =
+                PPED.makePPED
+                  (PPE.hqNamer 10 currentDeepNamesSansOld)
+                  (PPE.suffixifyByHash currentDeepNamesSansOld)
+           in ppe1 `PPED.addFallback` ppe2 `PPED.addFallback` ppe3
         )
 
   pp@(PP.ProjectPath project projectBranch _path) <- Cli.getCurrentProjectPath
@@ -300,12 +308,12 @@ makeUnisonFile abort codebase doFindCtorNames defns = do
                 overwriteConstructorNames name ed.toDataDecl <&> \ed' ->
                   uf
                     & #effectDeclarationsId
-                    %~ Map.insertWith (\_new old -> old) (Name.toVar name) (Reference.Id h i, Decl.EffectDeclaration ed')
+                      %~ Map.insertWith (\_new old -> old) (Name.toVar name) (Reference.Id h i, Decl.EffectDeclaration ed')
               Right dd ->
                 overwriteConstructorNames name dd <&> \dd' ->
                   uf
                     & #dataDeclarationsId
-                    %~ Map.insertWith (\_new old -> old) (Name.toVar name) (Reference.Id h i, dd')
+                      %~ Map.insertWith (\_new old -> old) (Name.toVar name) (Reference.Id h i, dd')
 
         -- Constructor names are bogus when pulled from the database, so we set them to what they should be here
         overwriteConstructorNames :: Name -> DataDeclaration Symbol Ann -> Transaction (DataDeclaration Symbol Ann)

unison-src/transcripts/fix-5276.md → unison-src/transcripts/fix-5312.md

@@ -1,3 +1,6 @@
+This transcript demonstrates that dependents of an update are suffixified properly. Previously, `c = b.y + 1` would
+render as `c = y + 1` (ambiguous).
+
 ```ucm
 scratch/main> builtins.merge lib.builtin
 ```

unison-src/transcripts/fix-5276.output.md → unison-src/transcripts/fix-5312.output.md

@@ -1,3 +1,6 @@
+This transcript demonstrates that dependents of an update are suffixified properly. Previously, `c = b.y + 1` would
+render as `c = y + 1` (ambiguous).
+
 ``` ucm
 scratch/main> builtins.merge lib.builtin
 

unison-src/transcripts/fix-5323.md

@@ -0,0 +1,24 @@
+This transcript demonstrates that dependents of an upgrade are suffixified properly. Previously, `c = b.y + 1` would
+render as `c = y + 1` (ambiguous).
+
+```ucm
+scratch/main> builtins.merge lib.builtin
+```
+
+```unison
+lib.old.x = 17
+lib.new.x = 100
+
+a.y = 18
+b.y = lib.old.x + 1
+
+c = b.y + 1
+```
+
+```ucm
+scratch/main> add
+```
+
+```ucm
+scratch/main> upgrade old new
+```

unison-src/transcripts/fix-5323.output.md

@@ -0,0 +1,54 @@
+This transcript demonstrates that dependents of an upgrade are suffixified properly. Previously, `c = b.y + 1` would
+render as `c = y + 1` (ambiguous).
+
+``` ucm
+scratch/main> builtins.merge lib.builtin
+
+  Done.
+
+```
+``` unison
+lib.old.x = 17
+lib.new.x = 100
+
+a.y = 18
+b.y = lib.old.x + 1
+
+c = b.y + 1
+```
+
+``` ucm
+
+  Loading changes detected in scratch.u.
+
+  I found and typechecked these definitions in scratch.u. If you
+  do an `add` or `update`, here's how your codebase would
+  change:
+  
+    ⍟ These new definitions are ok to `add`:
+    
+      a.y       : Nat
+      b.y       : Nat
+      c         : Nat
+      lib.new.x : Nat
+      lib.old.x : Nat
+
+```
+``` ucm
+scratch/main> add
+
+  ⍟ I've added these definitions:
+  
+    a.y       : Nat
+    b.y       : Nat
+    c         : Nat
+    lib.new.x : Nat
+    lib.old.x : Nat
+
+```
+``` ucm
+scratch/main> upgrade old new
+
+  I upgraded old to new, and removed old.
+
+```