From 372c0b667e7b6f36a5f1a42a9802eb0246ee3e95 Mon Sep 17 00:00:00 2001
From: Vidar Holen <vidar@vidarholen.net>
Date: Sun, 30 Jul 2023 13:47:00 -0700
Subject: [PATCH] SC2324: Warn when x+=1 appends.

---
 CHANGELOG.md                  |  1 +
 src/ShellCheck/ASTLib.hs      |  9 +++++++++
 src/ShellCheck/Analytics.hs   | 38 +++++++++++++++++++++++++++++++++++
 src/ShellCheck/CFGAnalysis.hs | 16 +++++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fc1c0ce..8f4426e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ## Git
 ### Added
+- SC2324: Warn when x+=1 appends instead of increments.
 
 ### Fixed
 
diff --git a/src/ShellCheck/ASTLib.hs b/src/ShellCheck/ASTLib.hs
index 56903ee..64fa762 100644
--- a/src/ShellCheck/ASTLib.hs
+++ b/src/ShellCheck/ASTLib.hs
@@ -886,6 +886,15 @@ isUnmodifiedParameterExpansion t =
             in getBracedReference str == str
         _ -> False
 
+-- Return the referenced variable if (and only if) it's an unmodified parameter expansion.
+getUnmodifiedParameterExpansion t =
+    case t of
+        T_DollarBraced _ _ list -> do
+            let str = concat $ oversimplify list
+            guard $ getBracedReference str == str
+            return str
+        _ -> Nothing
+
 --- A list of the element and all its parents up to the root node.
 getPath tree t = t :
     case Map.lookup (getId t) tree of
diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs
index ecc170d..dbad0f5 100644
--- a/src/ShellCheck/Analytics.hs
+++ b/src/ShellCheck/Analytics.hs
@@ -201,6 +201,7 @@ nodeChecks = [
     ,checkOverwrittenExitCode
     ,checkUnnecessaryArithmeticExpansionIndex
     ,checkUnnecessaryParens
+    ,checkPlusEqualsNumber
     ]
 
 optionalChecks = map fst optionalTreeChecks
@@ -5007,5 +5008,42 @@ checkUnnecessaryParens params t =
         ]
 
 
+prop_checkPlusEqualsNumber1 = verify checkPlusEqualsNumber "x+=1"
+prop_checkPlusEqualsNumber2 = verify checkPlusEqualsNumber "x+=42"
+prop_checkPlusEqualsNumber3 = verifyNot checkPlusEqualsNumber "(( x += 1 ))"
+prop_checkPlusEqualsNumber4 = verifyNot checkPlusEqualsNumber "declare -i x=0; x+=1"
+prop_checkPlusEqualsNumber5 = verifyNot checkPlusEqualsNumber "x+='1'"
+prop_checkPlusEqualsNumber6 = verifyNot checkPlusEqualsNumber "n=foo; x+=n"
+prop_checkPlusEqualsNumber7 = verify checkPlusEqualsNumber "n=4; x+=n"
+prop_checkPlusEqualsNumber8 = verify checkPlusEqualsNumber "n=4; x+=$n"
+prop_checkPlusEqualsNumber9 = verifyNot checkPlusEqualsNumber "declare -ia var; var[x]+=1"
+checkPlusEqualsNumber params t =
+    case t of
+        T_Assignment id Append var _ word -> sequence_ $ do
+            state <- CF.getIncomingState (cfgAnalysis params) id
+            guard $ isNumber state word
+            guard . not $ fromMaybe False $ CF.variableMayBeDeclaredInteger state var
+            return $ warn id 2324 "var+=1 will append, not increment. Use (( var += 1 )), declare -i var, or quote number to silence."
+        _ -> return ()
+
+  where
+    isNumber state word =
+        let
+            unquotedLiteral = getUnquotedLiteral word
+            isEmpty = unquotedLiteral == Just ""
+            isUnquotedNumber = not isEmpty && fromMaybe False (all isDigit <$> unquotedLiteral)
+            isNumericalVariableName = fromMaybe False $ do
+                str <- unquotedLiteral
+                CF.variableMayBeAssignedInteger state str
+            isNumericalVariableExpansion =
+                case word of
+                    T_NormalWord _ [part] -> fromMaybe False $ do
+                        str <- getUnmodifiedParameterExpansion part
+                        CF.variableMayBeAssignedInteger state str
+                    _ -> False
+        in
+            isUnquotedNumber || isNumericalVariableName || isNumericalVariableExpansion
+
+
 return []
 runTests =  $( [| $(forAllProperties) (quickCheckWithResult (stdArgs { maxSuccess = 1 }) ) |])
diff --git a/src/ShellCheck/CFGAnalysis.hs b/src/ShellCheck/CFGAnalysis.hs
index 4e36cf5..3b4f957 100644
--- a/src/ShellCheck/CFGAnalysis.hs
+++ b/src/ShellCheck/CFGAnalysis.hs
@@ -59,6 +59,8 @@ module ShellCheck.CFGAnalysis (
     ,getIncomingState
     ,getOutgoingState
     ,doesPostDominate
+    ,variableMayBeDeclaredInteger
+    ,variableMayBeAssignedInteger
     ,ShellCheck.CFGAnalysis.runTests -- STRIP
     ) where
 
@@ -153,6 +155,20 @@ doesPostDominate analysis target base = fromMaybe False $ do
     (targetStart, _) <- M.lookup target $ tokenToRange analysis
     return $ targetStart `elem` (postDominators analysis ! baseEnd)
 
+-- See if any execution path results in the variable containing a state
+variableMayHaveState :: ProgramState -> String -> CFVariableProp -> Maybe Bool
+variableMayHaveState state var property = do
+    value <- M.lookup var $ variablesInScope state
+    return $ any (S.member property) $ variableProperties value
+
+-- See if any execution path declares the variable an integer (declare -i).
+variableMayBeDeclaredInteger state var = variableMayHaveState state var CFVPInteger
+
+-- See if any execution path suggests the variable may contain an integer value
+variableMayBeAssignedInteger state var = do
+    value <- M.lookup var $ variablesInScope state
+    return $ (numericalStatus $ variableValue value) >= NumericalStatusMaybe
+
 getDataForNode analysis node = M.lookup node $ nodeToData analysis
 
 -- The current state of data flow at a point in the program, potentially as a diff