From e51dbac52bf91ffd8f45c60ded7d18121ea6f28c Mon Sep 17 00:00:00 2001 From: Taeer Bar-Yam Date: Thu, 11 Nov 2021 10:57:24 -0500 Subject: [PATCH 1/3] fix UnusedLetBind: account for `inherit` --- src/Nix/Linter/Checks.hs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/Nix/Linter/Checks.hs b/src/Nix/Linter/Checks.hs index 4cacae6..95f2369 100644 --- a/src/Nix/Linter/Checks.hs +++ b/src/Nix/Linter/Checks.hs @@ -35,11 +35,15 @@ checkUnusedLetBinding :: CheckBase checkUnusedLetBinding warn e = [ (warn UnusedLetBind) & setLoc loc & note' varName name - | NLet_ _ binds usedIn <- [unFix e] + | NLet_ srcSpan binds usedIn <- [unFix e] , (bind, others) <- choose binds - , NamedVar (StaticKey name :| []) _ loc <- [bind] - , all (noRef name) (values others) - , name `noRef` usedIn + , (name, loc) <- case bind of + NamedVar (StaticKey name :| []) _ loc -> [(name, loc)] + Inherit _ keys loc -> do + StaticKey name <- keys + [(name, loc)] + _ -> [] + , noRef name $ Fix $ NLet_ srcSpan others usedIn ] checkUnusedArg :: CheckBase From 299affb0b7235b82e4cdd43b5ed61b8d8fc79cdd Mon Sep 17 00:00:00 2001 From: Taeer Bar-Yam Date: Mon, 15 Nov 2021 07:07:01 -0500 Subject: [PATCH 2/3] UnusedLetBind: speed up --- src/Nix/Linter/Checks.hs | 8 ++++--- src/Nix/Linter/Tools/FreeVars.hs | 41 +++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/Nix/Linter/Checks.hs b/src/Nix/Linter/Checks.hs index 95f2369..30fdda8 100644 --- a/src/Nix/Linter/Checks.hs +++ b/src/Nix/Linter/Checks.hs @@ -25,6 +25,7 @@ import Nix.Expr.Types import Nix.Expr.Types.Annotated import Nix.Linter.Tools +import Nix.Linter.Tools.FreeVars import Nix.Linter.Types import Nix.Linter.Utils (choose, sorted, (<$$>), (<&>)) @@ -35,15 +36,16 @@ checkUnusedLetBinding :: CheckBase checkUnusedLetBinding warn e = [ (warn UnusedLetBind) & setLoc loc & note' varName name - | NLet_ srcSpan binds usedIn <- [unFix e] - , (bind, others) <- choose binds + | NLet_ _ binds _ <- [unFix e] + , free <- [freeVarsIgnoreTopBinds' e] + , bind <- binds , (name, loc) <- case bind of NamedVar (StaticKey name :| []) _ loc -> [(name, loc)] Inherit _ keys loc -> do StaticKey name <- keys [(name, loc)] _ -> [] - , noRef name $ Fix $ NLet_ srcSpan others usedIn + , not $ Set.member name free ] checkUnusedArg :: CheckBase diff --git a/src/Nix/Linter/Tools/FreeVars.hs b/src/Nix/Linter/Tools/FreeVars.hs index a078991..8671edd 100644 --- a/src/Nix/Linter/Tools/FreeVars.hs +++ b/src/Nix/Linter/Tools/FreeVars.hs @@ -1,9 +1,48 @@ -module Nix.Linter.Tools.FreeVars (freeVars, freeVars') where +module Nix.Linter.Tools.FreeVars (freeVars, freeVars', freeVarsIgnoreTopBinds + , freeVarsIgnoreTopBinds') where import Data.Set (Set) import Nix.Expr.Types import Nix.Expr.Types.Annotated import Nix.TH (freeVars) +import qualified Data.Set as Set +import Data.Maybe (mapMaybe) +import Data.Fix (unFix, Fix (..)) + freeVars' :: NExprLoc -> Set VarName freeVars' = freeVars . stripAnnotation + +freeVarsIgnoreTopBinds' :: NExprLoc -> Set VarName +freeVarsIgnoreTopBinds' = freeVarsIgnoreTopBinds . stripAnnotation + +-- gets the free variables of the expression, assuming the top level bindings +-- were not bound. Note that this function is *not* recursive, since we want to +-- count bindings deeper in the tree +freeVarsIgnoreTopBinds :: NExpr -> Set VarName +freeVarsIgnoreTopBinds e = + case unFix e of + (NSet NRecursive bindings) -> bindFreeVars bindings + (NAbs (Param _) expr) -> freeVars expr + (NAbs (ParamSet set _ _) expr) -> + freeVars expr <> (Set.unions $ freeVars <$> mapMaybe snd set) + (NLet bindings expr) -> freeVars expr <> bindFreeVars bindings + noTopBinds -> freeVars $ Fix noTopBinds + where + bindFreeVars :: Foldable t => t (Binding NExpr) -> Set VarName + bindFreeVars = foldMap bind1Free + where + bind1Free :: Binding NExpr -> Set VarName + bind1Free (Inherit Nothing keys _) = Set.fromList $ mapMaybe staticKey keys + bind1Free (Inherit (Just scope) _ _) = freeVars scope + bind1Free (NamedVar path expr _) = pathFree path <> freeVars expr + + staticKey :: NKeyName r -> Maybe VarName + staticKey (StaticKey varname) = pure varname + staticKey (DynamicKey _ ) = mempty + + pathFree :: NAttrPath NExpr -> Set VarName + pathFree = foldMap mapFreeVars + + mapFreeVars :: Foldable t => t NExpr -> Set VarName + mapFreeVars = foldMap freeVars From fd7fcaff9462dc737e8d9dbdeb6c721c6c1b1bcf Mon Sep 17 00:00:00 2001 From: Taeer Bar-Yam Date: Mon, 22 Nov 2021 19:18:03 -0500 Subject: [PATCH 3/3] fix UnneededRec's treatment of `inherit`s --- src/Nix/Linter/Checks.hs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/Nix/Linter/Checks.hs b/src/Nix/Linter/Checks.hs index 30fdda8..7d73a23 100644 --- a/src/Nix/Linter/Checks.hs +++ b/src/Nix/Linter/Checks.hs @@ -27,7 +27,7 @@ import Nix.Expr.Types.Annotated import Nix.Linter.Tools import Nix.Linter.Tools.FreeVars import Nix.Linter.Types -import Nix.Linter.Utils (choose, sorted, (<$$>), (<&>)) +import Nix.Linter.Utils (choose, sorted, (<$$>)) varName :: Text varName = "varName" @@ -72,10 +72,19 @@ checkEmptyInherit warn e = [ (warn EmptyInherit) {pos=singletonSpan loc} checkUnneededRec :: CheckBase checkUnneededRec warn e = [ warn UnneededRec | NSet_ _ann NRecursive binds <- [unFix e] - , not $ or $ choose binds <&> \case - (bind, others) -> case bind of - NamedVar (StaticKey name :| []) _ _ -> all (noRef name) (values others) - _ -> False + , free <- [freeVarsIgnoreTopBinds' e] + , not . or $ do + bind <- binds + name <- case bind of + NamedVar (StaticKey name :| []) _ _ -> [name] + Inherit _ keys _ -> do + StaticKey name <- keys + [name] + -- References to dynamic keys on the LHS are not allowed + -- e.g. rec { x = "hi"; ${x} = 5; y = hi; } + -- will fail to evaluate, so we don't need to account for it + _ -> [] + pure $ Set.member name free ] checkOpBase :: OffenseCategory -> Pair (UnwrappedNExprLoc -> Bool) -> NBinaryOp -> Bool -> CheckBase