Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: update no longer tries to put non-test watches into the namespace #4454

Merged
merged 1 commit into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/unison-util-nametree/src/Unison/Util/Nametree.hs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ data Defns terms types = Defns
types :: !types
}
deriving stock (Generic, Show)
deriving (Semigroup) via GenericSemigroupMonoid (Defns terms types)
deriving (Monoid, Semigroup) via GenericSemigroupMonoid (Defns terms types)

mapDefns :: (a -> b) -> Defns a a -> Defns b b
mapDefns f (Defns terms types) =
Expand Down
4 changes: 1 addition & 3 deletions parser-typechecker/src/Unison/Codebase.hs
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,7 @@ addDefsToCodebase c uf = do
traverse_ goTerm (UF.hashTermsId uf)
where
goTerm t | debug && trace ("Codebase.addDefsToCodebase.goTerm " ++ show t) False = undefined
goTerm (_, r, Nothing, tm, tp) = putTerm c r tm tp
goTerm (_, r, Just WK.TestWatch, tm, tp) = putTerm c r tm tp
goTerm _ = pure ()
goTerm (_, r, wk, tm, tp) = when (WK.watchKindShouldBeStoredInDatabase wk) (putTerm c r tm tp)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactor, no change in behavior

goType :: (Show t) => (t -> Decl v a) -> (Reference.Id, t) -> Sqlite.Transaction ()
goType _f pair | debug && trace ("Codebase.addDefsToCodebase.goType " ++ show pair) False = undefined
goType f (ref, decl) = putTypeDeclaration c ref (f decl)
Expand Down
36 changes: 22 additions & 14 deletions unison-cli/src/Unison/Codebase/Editor/HandleInput/Update2.hs
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,7 @@ handleUpdate2 :: Cli ()
handleUpdate2 = do
Cli.Env {codebase} <- ask
tuf <- Cli.expectLatestTypecheckedFile

-- - get add/updates from TUF
let termAndDeclNames :: Defns (Set Name) (Set Name) = getTermAndDeclNames tuf

let termAndDeclNames = getTermAndDeclNames tuf
currentPath <- Cli.getCurrentPath
currentBranch0 <- Cli.getBranch0At currentPath
let namesIncludingLibdeps = Branch.toNames currentBranch0
Expand Down Expand Up @@ -197,9 +194,10 @@ saveTuf :: (Name -> Either Output [Name]) -> TypecheckedUnisonFile Symbol Ann ->
saveTuf getConstructors tuf = do
Cli.Env {codebase} <- ask
currentPath <- Cli.getCurrentPath
branchUpdates <- Cli.runTransactionWithRollback \abort -> do
Codebase.addDefsToCodebase codebase tuf
typecheckedUnisonFileToBranchUpdates abort getConstructors tuf
branchUpdates <-
Cli.runTransactionWithRollback \abort -> do
Codebase.addDefsToCodebase codebase tuf
typecheckedUnisonFileToBranchUpdates abort getConstructors tuf
Cli.stepAt "update" (Path.unabsolute currentPath, Branch.batchUpdates branchUpdates)

-- @typecheckedUnisonFileToBranchUpdates getConstructors file@ returns a list of branch updates (suitable for passing
Expand Down Expand Up @@ -257,11 +255,14 @@ typecheckedUnisonFileToBranchUpdates abort getConstructors tuf = do
tuf
& UF.hashTermsId
& Map.toList
& foldMap \(var, (_, ref, _, _, _)) ->
let split = splitVar var
in [ BranchUtil.makeAnnihilateTermName split,
BranchUtil.makeAddTermName split (Referent.fromTermReferenceId ref) Map.empty
]
& foldMap \(var, (_, ref, wk, _, _)) ->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the bugfix. before, we didn't look at wk. now, we do.

if WK.watchKindShouldBeStoredInDatabase wk
then
let split = splitVar var
in [ BranchUtil.makeAnnihilateTermName split,
BranchUtil.makeAddTermName split (Referent.fromTermReferenceId ref) Map.empty
]
else []

splitVar :: Symbol -> Path.Split
splitVar = Path.splitFromName . Name.unsafeFromVar
Expand Down Expand Up @@ -425,10 +426,17 @@ incrementLastSegmentChar (ForwardName segments) =
else Text.init text `Text.append` Text.singleton (succ $ Text.last text)
in NameSegment incrementedText

-- @getTermAndDeclNames file@ returns the names of the terms and decls defined in a typechecked Unison file.
getTermAndDeclNames :: (Var v) => TypecheckedUnisonFile v a -> Defns (Set Name) (Set Name)
getTermAndDeclNames tuf = Defns (terms <> effectCtors <> dataCtors) (effects <> datas)
getTermAndDeclNames tuf =
Defns (terms <> effectCtors <> dataCtors) (effects <> datas)
where
terms = keysToNames $ UF.hashTermsId tuf
terms =
UF.hashTermsId tuf
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a similar issue here in just extracting type/term names; I'm not sure if it directly contributed to the crash, but we simply don't want watches that aren't "test" watches here, either.

& Map.foldMapWithKey \var (_, _, wk, _, _) ->
if WK.watchKindShouldBeStoredInDatabase wk
then Set.singleton (Name.unsafeFromVar var)
else Set.empty
effects = keysToNames $ UF.effectDeclarationsId' tuf
datas = keysToNames $ UF.dataDeclarationsId' tuf
effectCtors = foldMap ctorsToNames $ fmap (Decl.toDataDecl . snd) $ UF.effectDeclarationsId' tuf
Expand Down
9 changes: 2 additions & 7 deletions unison-cli/src/Unison/Codebase/Editor/Slurp.hs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import Unison.Util.Relation qualified as Rel
import Unison.Util.Set qualified as Set
import Unison.Var (Var)
import Unison.Var qualified as Var
import Unison.WatchKind (pattern TestWatch)
import Unison.WatchKind (watchKindShouldBeStoredInDatabase)

-- | The operation which is being performed or checked.
data SlurpOp
Expand Down Expand Up @@ -278,12 +278,7 @@ buildVarReferences uf =
terms =
UF.hashTermsId uf
-- Filter out non-test watch expressions
& Map.filter
( \case
(_, _, w, _, _)
| w == Just TestWatch || w == Nothing -> True
| otherwise -> False
)
& Map.filter (\(_, _, w, _, _) -> watchKindShouldBeStoredInDatabase w)
& Map.bimap
TermVar
(\(_, refId, _, _, _) -> LD.derivedTerm refId)
Expand Down
28 changes: 24 additions & 4 deletions unison-core/src/Unison/WatchKind.hs
Original file line number Diff line number Diff line change
@@ -1,10 +1,22 @@
{-# LANGUAGE RecordWildCards #-}

module Unison.WatchKind where
-- | A "watch kind" is the slug that comes before a ">" in a Unison file:
--
-- >
-- > foo> bar = baz
-- >
--
-- In this example, the watch kind is "foo".
module Unison.WatchKind
( WatchKind,
pattern RegularWatch,
pattern TestWatch,
watchKindShouldBeStoredInDatabase,
)
where

import Data.String (IsString)

type WatchKind = String
type WatchKind =
String

-- | A non-test watch, such as
-- @
Expand All @@ -22,3 +34,11 @@ pattern RegularWatch = ""
-- Note: currently test watches don't need to be named by the user, but that "feature" will be removed soon.
pattern TestWatch :: (Eq a, IsString a) => a
pattern TestWatch = "test"

-- Haha terrible name. Regular terms (no ">" in sight) and test watches ("test>") should be stored in the database
-- (the latter, if nameless, get a random name), but other watches (like regular ">" or even weird "oink>") shouldn't.
watchKindShouldBeStoredInDatabase :: Maybe WatchKind -> Bool
watchKindShouldBeStoredInDatabase = \case
Nothing -> True
Just TestWatch -> True
_ -> False
7 changes: 7 additions & 0 deletions unison-src/transcripts/update-watch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```unison
> 1
```

```ucm
.> update
```
27 changes: 27 additions & 0 deletions unison-src/transcripts/update-watch.output.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
```unison
> 1
```

```ucm
scratch.u changed.
Now evaluating any watch expressions (lines starting with
`>`)... Ctrl+C cancels.
1 | > 1
1
```
```ucm
.> update
Okay, I'm searching the branch for code that needs to be
updated...
Done.
```