From e7c89749b99115c78b7d7362ea3ced1a1421b0b6 Mon Sep 17 00:00:00 2001 From: Paul Chiusano Date: Wed, 18 Mar 2020 14:23:38 -0400 Subject: [PATCH] Fixed issue with Metadata.delete, transcript now passes! Issue was that it was unilaterally deleting the D2 dimension for whatever type was being unlinked (this is the dimension that supports efficient lookup by metadata type - "find me all the docs"). Correct behavior is to only delete from this dimension when there are no more metadata values for a metadata type - for instance, if you delete the last `Doc`, you can delete the index that lets you find all the docs. --- .../src/Unison/Codebase/Editor/HandleInput.hs | 4 - .../src/Unison/Codebase/Metadata.hs | 11 +- .../src/Unison/CommandLine/InputPatterns.hs | 2 +- .../src/Unison/CommandLine/OutputMessages.hs | 2 +- parser-typechecker/src/Unison/Util/Star3.hs | 14 +- unison-src/transcripts/fix1356.md | 13 +- unison-src/transcripts/fix1356.output.md | 140 ++++++++++++++++++ 7 files changed, 170 insertions(+), 16 deletions(-) create mode 100644 unison-src/transcripts/fix1356.output.md diff --git a/parser-typechecker/src/Unison/Codebase/Editor/HandleInput.hs b/parser-typechecker/src/Unison/Codebase/Editor/HandleInput.hs index 90bd3b892..0970cffbe 100644 --- a/parser-typechecker/src/Unison/Codebase/Editor/HandleInput.hs +++ b/parser-typechecker/src/Unison/Codebase/Editor/HandleInput.hs @@ -435,18 +435,14 @@ loop = do -> Branch.Star r NameSegment) -> MaybeT (StateT (LoopState m v) (F m (Either Event Input) v)) () manageLinks srcs mdValue2 op = do - traceM "In manage links, before srcle" let !srcle = toList . getHQ'Terms =<< srcs !srclt = toList . getHQ'Types =<< srcs - traceM "In manage links, AFTER srcle" mdValuel <- toList <$> getHQTerms mdValue2 - traceM "Got mdValuel" names0 <- basicPrettyPrintNames0 ppe <- PPE.suffixifiedPPE <$> prettyPrintEnvDecl (Names names0 mempty) case (srcle, srclt, mdValuel) of (srcle, srclt, [r@(Referent.Ref mdValue)]) -> do mdType <- eval $ LoadTypeOfTerm mdValue - traceM $ "Loaded type for " <> show mdValue case mdType of Nothing -> respond $ MetadataMissingType ppe r Just ty -> do diff --git a/parser-typechecker/src/Unison/Codebase/Metadata.hs b/parser-typechecker/src/Unison/Codebase/Metadata.hs index 5349bf474..a005a1be8 100644 --- a/parser-typechecker/src/Unison/Codebase/Metadata.hs +++ b/parser-typechecker/src/Unison/Codebase/Metadata.hs @@ -11,6 +11,7 @@ import Unison.Util.Relation (Relation) import qualified Unison.Util.Relation as R import Unison.Util.Relation4 (Relation4) import qualified Unison.Util.Relation4 as R4 +import qualified Unison.Util.List as List type Type = Reference type Value = Reference @@ -43,7 +44,15 @@ insert :: (Ord a, Ord n) => (a, Type, Value) -> Star a n -> Star a n insert (a, ty, v) = Star3.insertD23 (a, ty, (ty,v)) delete :: (Ord a, Ord n) => (a, Type, Value) -> Star a n -> Star a n -delete (a, ty, v) = Star3.deleteD23 (a, ty, (ty,v)) +delete (a, ty, v) s = let + s' = Star3.deleteD3 (a, (ty,v)) s + -- if (ty,v) is the last metadata of type ty + -- we also delete (a, ty) from the d2 index + metadataByType = List.multimap (toList (R.lookupDom a (Star3.d3 s))) + in + case Map.lookup ty metadataByType of + Just vs | all (== v) vs -> Star3.deleteD2 (a, ty) s' + _ -> s' -- parallel composition - commutative and associative merge :: Metadata -> Metadata -> Metadata diff --git a/parser-typechecker/src/Unison/CommandLine/InputPatterns.hs b/parser-typechecker/src/Unison/CommandLine/InputPatterns.hs index 74c7ceaba..dac9c1fa8 100644 --- a/parser-typechecker/src/Unison/CommandLine/InputPatterns.hs +++ b/parser-typechecker/src/Unison/CommandLine/InputPatterns.hs @@ -1077,7 +1077,7 @@ unlink = InputPattern md <- case HQ.fromString md of Nothing -> Left "Invalid hash qualified identifier for metadata." Just hq -> pure hq - Right $ traceShowId $ Input.UnlinkI defs md + Right $ Input.UnlinkI defs md _ -> Left (I.help unlink) ) diff --git a/parser-typechecker/src/Unison/CommandLine/OutputMessages.hs b/parser-typechecker/src/Unison/CommandLine/OutputMessages.hs index 9cb3da330..1ae2ed5ad 100644 --- a/parser-typechecker/src/Unison/CommandLine/OutputMessages.hs +++ b/parser-typechecker/src/Unison/CommandLine/OutputMessages.hs @@ -1301,7 +1301,7 @@ listOfLinks :: listOfLinks _ [] = pure . P.callout "😶" . P.wrap $ "No results. Try using the " <> IP.makeExample IP.link [] <> - "command to add outgoing links to a definition." + "command to add metadata to a definition." listOfLinks ppe results = pure $ P.lines [ P.numberedColumn2 num [ (P.syntaxToColor $ prettyHashQualified hq, ": " <> prettyType typ) | (hq,typ) <- results diff --git a/parser-typechecker/src/Unison/Util/Star3.hs b/parser-typechecker/src/Unison/Util/Star3.hs index 295f8e615..7f7e5303a 100644 --- a/parser-typechecker/src/Unison/Util/Star3.hs +++ b/parser-typechecker/src/Unison/Util/Star3.hs @@ -160,13 +160,19 @@ insertD23 (f, x, y) s = Star3 fact' (d1 s) d2' d3' where d2' = R.insert f x (d2 s) d3' = R.insert f y (d3 s) -deleteD23 :: (Ord fact, Ord d1, Ord d2, Ord d3) - => (fact, d2, d3) +deleteD3 :: (Ord fact, Ord d1, Ord d2, Ord d3) + => (fact, d3) -> Star3 fact d1 d2 d3 -> Star3 fact d1 d2 d3 -deleteD23 (f, x, y) s = Star3 (fact s) (d1 s) d2' d3' where +deleteD3 (f, x) s = Star3 (fact s) (d1 s) (d2 s) d3' where + d3' = R.delete f x (d3 s) + +deleteD2 :: (Ord fact, Ord d1, Ord d2, Ord d3) + => (fact, d2) + -> Star3 fact d1 d2 d3 + -> Star3 fact d1 d2 d3 +deleteD2 (f, x) s = Star3 (fact s) (d1 s) d2' (d3 s) where d2' = R.delete f x (d2 s) - d3' = R.delete f y (d3 s) deleteFact :: (Ord fact, Ord d1, Ord d2, Ord d3) => Set fact -> Star3 fact d1 d2 d3 -> Star3 fact d1 d2 d3 diff --git a/unison-src/transcripts/fix1356.md b/unison-src/transcripts/fix1356.md index b72aedca5..a26af42eb 100644 --- a/unison-src/transcripts/fix1356.md +++ b/unison-src/transcripts/fix1356.md @@ -37,13 +37,16 @@ Step 6: Great that seems to have worked but let me check just in case .> display 1 ``` -Step 7: I have been guided to check `display 1`, so now let's try and unlink -to get rid of the second link. +Step 7: I have been guided to check `display 1`, so now let's try and unlink to get rid of the second link. We can remove by number, or by hash: ```ucm -.> unlink x.doc x +.> unlink 2 x +.> undo +.> unlink #v8f1hhvs57 x ``` -Issuing the `unlink x.doc x` command here breaks ucm, and no transcript output is generated. -The captured output is `ucm: Maybe.fromJust: Nothing` +We expect that after the `unlink`, the `docs` command now works as there's a single `Doc` again: +```ucm +.> docs x +``` diff --git a/unison-src/transcripts/fix1356.output.md b/unison-src/transcripts/fix1356.output.md new file mode 100644 index 000000000..f0b4cef46 --- /dev/null +++ b/unison-src/transcripts/fix1356.output.md @@ -0,0 +1,140 @@ +##### This transcript reproduces the failure to unlink documentation + +Step 1: code a term and documentation for it +```unison +x = 1 +x.doc = [: I am the documentation for x:] +``` + +```ucm + + 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`: + + x : Nat + x.doc : Doc + + Now evaluating any watch expressions (lines starting with + `>`)... Ctrl+C cancels. + +``` +Step 2: add term and documentation, link, and check the documentation +```ucm +.> add + + ⍟ I've added these definitions: + + x : Nat + x.doc : Doc + +.> link x.doc x + + Updates: + + 1. x : Nat + + 2. x.doc : Doc + +.> docs x + + I am the documentation for x + +``` +Step 3: Oops I don't like the doc, so I will re-code it! +```unison +x.doc = [: I am the documentation for x, and I now look better:] +``` + +```ucm + + I found and typechecked these definitions in scratch.u. If you + do an `add` or `update`, here's how your codebase would + change: + + ⍟ These names already exist. You can `update` them to your + new definition: + + x.doc : Doc + + Now evaluating any watch expressions (lines starting with + `>`)... Ctrl+C cancels. + +``` +Step 4: I add it and expect to see it +```ucm +.> update + + ⍟ I've updated these names to your new definition: + + x.doc : base.Doc + +.> docs x + + I am the documentation for x + +``` +But I don't see an update, so from here on I am in panic mode and have no clue what I am doing! + +Step 5: maybe re-link it? +```ucm +.> link x.doc x + + Updates: + + 1. x : Nat + + 2. x.doc : Doc + +``` +Step 6: Great that seems to have worked but let me check just in case + +```ucm +.> docs x + + 1. x.doc : Doc + 2. x.doc#v8f1hhvs57 : Doc + + Tip: Try using `display 1` to display the first result or + `view 1` to view its source. + +.> display 1 + + I am the documentation for x, and I now look better + +``` +Step 7: I have been guided to check `display 1`, so now let's try and unlink to get rid of the second link. We can remove by number, or by hash: + +```ucm +.> unlink 2 x + + Updates: + + 1. x : Nat + - 2. (unnamed metadata) : Doc + +.> undo + + Here's the changes I undid + + Updates: + + 1. x : Nat + - 2. (unnamed metadata) : Doc + +.> unlink #v8f1hhvs57 x + + Updates: + + 1. x : Nat + - 2. (unnamed metadata) : Doc + +``` +We expect that after the `unlink`, the `docs` command now works as there's a single `Doc` again: + +```ucm +.> docs x + + I am the documentation for x, and I now look better + +```