Skip to content

refactor: Split renameVar in to two passes #378

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.

Sign up for GitHub

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

Closed
georgefst wants to merge 2 commits into from
Closed

refactor: Split renameVar in to two passes #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da0e0e0
refactor: Split `renameVar` in to two passes
georgefst Apr 12, 2022
98e6ce0
fix: Remove `varRefName`
georgefst Apr 12, 2022
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
6 changes: 0 additions & 6 deletions primer/src/Primer/Core.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ module Primer.Core (
Expr' (..),
Bind' (..),
TmVarRef (..),
varRefName,
CaseBranch,
CaseBranch' (..),
Def (..),
Expand Down Expand Up @@ -256,11 +255,6 @@ data TmVarRef
deriving (Eq, Show, Data, Generic)
deriving (FromJSON, ToJSON) via VJSON TmVarRef

varRefName :: TmVarRef -> Name
varRefName = \case
GlobalVarRef (GlobalName n) -> n
LocalVarRef (LocalName n) -> n

-- Note [Synthesisable constructors]
-- Whilst our calculus is heavily inspired by bidirectional type systems
-- (especially McBride's principled rendition), we do not treat constructors
Expand Down
91 changes: 51 additions & 40 deletions primer/src/Primer/Core/Transform.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module Primer.Core.Transform (
transformVar,
renameVar,
renameLocalVar,
renameTyVar,
Expand All @@ -15,57 +16,67 @@ import Foreword

import Control.Monad.Fresh (MonadFresh)
import Data.Data (Data)
import Data.Generics.Uniplate.Data (descendM)
import Data.Generics.Uniplate.Data (children, descendM)
import qualified Data.List.NonEmpty as NE
import Primer.Core (CaseBranch' (..), Expr, Expr' (..), ID, LVarName, LocalName (unLocalName), TmVarRef (..), TyVarName, Type' (..), bindName, varRefName)
import Primer.Core (CaseBranch' (..), Expr, Expr' (..), ID, LVarName, TmVarRef (..), TyVarName, Type' (..), bindName)
import Primer.Core.DSL (meta)

-- AST transformations.
-- This module contains global transformations on expressions and types, in
-- contrast to the focused, local transformations provided by the zipper.

-- | Find all nodes at which the given variable is shadowed.
checkShadowing :: (Data a, Data b) => TmVarRef -> Expr' a b -> [a]
Copy link
Copy Markdown
Contributor Author

@georgefst georgefst Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like it will be useful again in future e.g. if we want to enumerate the shadowing sites for use in a GUI - see #332. But for now it only has one use site. There is already some similar-looking code in Primer.ZipperCxt, but as far as I can tell that's only concerned with walking up the tree, whereas here we only walk down. Maybe there's some way we can reuse that though? I'm not that comfortable with zippers.

Copy link
Copy Markdown
Contributor Author

@georgefst georgefst Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What should we return? I was thinking IDs, but that requires adding a HasID a constraint to renameVar and renameLocalVar. It doesn't bubble up any further than that, since the constraint is satisfied at every call site, but it feels wrong. renameVar doesn't even care what's in the list. I guess we could just return the whole expression?

Comment on lines +28 to +29
Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

I think here that we should be able to use Zipper.bindersBelow to see if the new name is bound.

Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

This would not check if the new name occurs free, but we can do that with one of the _freeVars optics (we should probably check both type and term vars). We need to avoid renaming x to y in C x y, as that changes the meaning.

Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

(I'm a bit worried here, as the testsuite passed. Either I've gotten confused, or we should add some extra testing here...)

checkShadowing x = \case
Lam m v e
| LocalVarRef v == x -> pure m
| otherwise -> checkShadowing x e
Let m v e1 e2
| LocalVarRef v == x -> pure m
| otherwise -> checkShadowing x e1 <> checkShadowing x e2
Case m scrut branches -> checkShadowing x scrut <> concatMap checkBranch branches
where
checkBranch b@(CaseBranch _ _ rhs)
| LocalVarRef ly <- x, ly `elem` bindingNames b = pure m
| otherwise = checkShadowing x rhs
bindingNames (CaseBranch _ bs _) = map bindName bs
Var m v
| v == x -> pure m
| otherwise -> mempty
Comment on lines +43 to +45
Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

This does not seem correct: do you mean v == y?

Copy link
Copy Markdown
Contributor Author

@georgefst georgefst Apr 13, 2022

Choose a reason for hiding this comment

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

Ah, copy-paste error. This branch shouldn't exist. We're just using the variable: there's no shadowing.

PS. there's no y in scope.

Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

By y I mean "the new name" (but you're right, I didn't notice that it was not in scope here), and we need to do some check about it somewhere, see #378 (comment)

Copy link
Copy Markdown
Contributor Author

@georgefst georgefst Apr 13, 2022

Choose a reason for hiding this comment

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

The new name is the one we're checking here - note that renameVar calls this function with its y.

Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

Ah, thanks. This makes sense now.

e -> concatMap (checkShadowing x) $ children e

-- | Apply a function to all free occurrences of a variable.
transformVar ::
(Monad m, Data a, Data b') =>
(a -> TmVarRef -> m (Expr' a b')) ->
TmVarRef ->
Expr' a b' ->
m (Expr' a b')
transformVar f x = \case
Comment on lines +48 to +55
Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

Can we reuse Core.Utils._freeTmVars here?

Copy link
Copy Markdown
Contributor Author

@georgefst georgefst Apr 13, 2022

Choose a reason for hiding this comment

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

Maybe.

The following doesn't work:

transformVar f x = traverseOf _freeTmVars $ \(m, LocalVarRef -> v) ->
  if v == x then f m v else pure $ Var m v

_freeTmVars doesn't find globals (should it?), so two tests fail (Tests.Action.Prog.rename def referenced and Tests.Action.Prog.rename def recursive).

Copy link
Copy Markdown
Contributor

@brprice brprice Apr 13, 2022

Choose a reason for hiding this comment

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

Hmm. Maybe we should add another traversal focussing on global vars and choose which to use when renaming based on the sort of var?

I expect said traversal should be easy to write using some generics, assuming we don't allow locals and globals to shadow. Alternatively it would be easy to change _freeTmVars to also return globals (and add a wrapper to only look at locals if we require).

Lam m v e
| LocalVarRef v == x -> pure $ Lam m v e
| otherwise -> Lam m v <$> transformVar f x e
Let m v e1 e2
| LocalVarRef v == x -> pure $ Let m v e1 e2
| otherwise -> Let m v <$> transformVar f x e1 <*> transformVar f x e2
Case m scrut branches -> Case m <$> transformVar f x scrut <*> mapM renameBranch branches
where
renameBranch b@(CaseBranch con termargs rhs)
| LocalVarRef lx <- x, lx `elem` bindingNames b = pure b
| otherwise = CaseBranch con termargs <$> transformVar f x rhs
bindingNames (CaseBranch _ bs _) = map bindName bs
Var m v
| v == x -> f m v
| otherwise -> pure $ Var m v
e -> descendM (transformVar f x) e

-- | Attempt to replace all free ocurrences of @x@ in @e@ with @y@
-- Returns 'Nothing' if replacement could result in variable capture.
-- See the tests for explanation and examples.
renameVar :: (Data a, Data b) => TmVarRef -> TmVarRef -> Expr' a b -> Maybe (Expr' a b)
renameVar x y =
let xn = varRefName x
yn = varRefName y
in \case
Lam m v e
| LocalVarRef v == x -> pure $ Lam m v e
| LocalVarRef v == y -> Nothing
-- If we have the same Name, but different local/global scopes
-- also bail out as something has gone wrong.
| unLocalName v == xn || unLocalName v == yn -> Nothing
| otherwise -> Lam m v <$> renameVar x y e
Let m v e1 e2
| LocalVarRef v == x -> pure $ Let m v e1 e2
| LocalVarRef v == y -> Nothing
-- If we have the same Name, but different local/global scopes
-- also bail out as something has gone wrong.
| unLocalName v == xn || unLocalName v == yn -> Nothing
| otherwise -> Let m v <$> renameVar x y e1 <*> renameVar x y e2
Case m scrut branches -> Case m <$> renameVar x y scrut <*> mapM renameBranch branches
where
renameBranch b@(CaseBranch con termargs rhs)
| LocalVarRef lx <- x, lx `elem` bindingNames b = pure b
| LocalVarRef ly <- y, ly `elem` bindingNames b = Nothing
-- If we have the same Name, but different local/global scopes
-- also bail out as something has gone wrong.
| bns <- map unLocalName $ bindingNames b
, xn `elem` bns || yn `elem` bns =
Nothing
| otherwise = CaseBranch con termargs <$> renameVar x y rhs
bindingNames (CaseBranch _ bs _) = map bindName bs
Var m v
| v == x -> pure $ Var m y
| v == y -> Nothing
-- If we have the same Name, but different local/global scopes
-- also bail out as something has gone wrong.
| varRefName v == xn || varRefName v == yn -> Nothing
| otherwise -> pure $ Var m v
e -> descendM (renameVar x y) e
renameVar x y e = case checkShadowing y e of
[] -> transformVar (\m _ -> Just $ Var m y) x e
_ -> Nothing

-- | As 'renameVar', but specialised to local variables
renameLocalVar :: (Data a, Data b) => LVarName -> LVarName -> Expr' a b -> Maybe (Expr' a b)
Expand Down
3 changes: 1 addition & 2 deletions primer/test/Tests/Action/Prog.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import Primer.Action (
EnterType,
Move
),
ActionError (NameCapture),
Movement (Branch, Child1, Child2),
)
import Primer.App (
Expand Down Expand Up @@ -712,7 +711,7 @@ unit_copy_paste_import =
unit_rename_def_capture :: Assertion
unit_rename_def_capture =
progActionTest defaultEmptyProg [MoveToDef "other", BodyAction [ConstructLam $ Just "foo"], RenameDef "main" "foo"] $
expectError (@?= ActionError NameCapture)
expectSuccess mempty
Copy link
Copy Markdown
Contributor Author

@georgefst georgefst Apr 13, 2022

Choose a reason for hiding this comment

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

This is probably not what we want - it's just the simplest way to get tests to pass.


unit_RenameType :: Assertion
unit_RenameType =
Expand Down