From 8b162a15370571a420a7e5616921f0d8809b7334 Mon Sep 17 00:00:00 2001 From: Adrian Fluturel Date: Tue, 7 Jan 2025 07:22:07 +0100 Subject: [PATCH 1/4] Add check for variables that could be readonly but aren't - SC2331 --- src/ShellCheck/Analytics.hs | 49 +++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 4757e57..2a0418a 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -71,6 +71,7 @@ treeChecks = [ ,checkUseBeforeDefinition ,checkAliasUsedInSameParsingUnit ,checkArrayValueUsedAsIndex + ,checkVariableCanBeReadonly ] checker spec params = mkChecker spec params treeChecks @@ -4672,6 +4673,54 @@ checkArrayValueUsedAsIndex params _ = _ -> Nothing +prop_checkVariableCanBeReadonly = verifyTree checkVariableCanBeReadonly "x=42; echo $x" +prop_checkVariableCanBeReadonly2 = verifyTree checkVariableCanBeReadonly "x=\"a\"; echo $x" +prop_checkVariableCanBeReadonly3 = verifyNotTree checkVariableCanBeReadonly "readonly x=\"a\"; echo $x" +prop_checkVariableCanBeReadonly4 = verifyTree checkVariableCanBeReadonly "declare -i num=100; echo $num" +prop_checkVariableCanBeReadonly5 = verifyTree checkVariableCanBeReadonly "declare -r CONFIG=\"config_value\"; echo $CONFIG" +prop_checkVariableCanBeReadonly6 = verifyTree checkVariableCanBeReadonly "declare -x ENV_VAR=\"environment\"; echo $ENV_VAR" +prop_checkVariableCanBeReadonly7 = verifyTree checkVariableCanBeReadonly "local variable=\"local_val\"; echo $variable" +prop_checkVariableCanBeReadonly8 = verifyNotTree checkVariableCanBeReadonly "readonly PI=3.14; echo $PI" +prop_checkVariableCanBeReadonly9 = verifyTree checkVariableCanBeReadonly "x=$(date); echo $x" +prop_checkVariableCanBeReadonly10 = verifyTree checkVariableCanBeReadonly "x=\"hello\"; echo $x" +prop_checkVariableCanBeReadonly11 = verifyTree checkVariableCanBeReadonly "varname=\"y\"; declare $varname=10; echo $y" +prop_checkVariableCanBeReadonly12 = verifyNotTree checkVariableCanBeReadonly "source somescript.sh; echo $foo" +prop_checkVariableCanBeReadonly13 = verifyTree checkVariableCanBeReadonly "export -r PERM=\"constant\"; echo $PERM" +prop_checkVariableCanBeReadonly14 = verifyTree checkVariableCanBeReadonly "arr=(1 2 3); echo ${arr[0]}" +prop_checkVariableCanBeReadonly15 = verifyTree checkVariableCanBeReadonly "declare -a arr=(1 2 3); echo ${arr[1]}" +prop_checkVariableCanBeReadonly16 = verifyTree checkVariableCanBeReadonly "dict=([key]=\"value\"); echo ${dict[key]}" +prop_checkVariableCanBeReadonly17 = verifyNotTree checkVariableCanBeReadonly "x=3; x=4; echo $x" +prop_checkVariableCanBeReadonly18 = verifyNotTree checkVariableCanBeReadonly "name=\"Alice\"; name=\"Bob\"; echo $name" +prop_checkVariableCanBeReadonly19 = verifyNotTree checkVariableCanBeReadonly "export PATH=\"/usr/local/bin\"; export PATH=\"/usr/bin\"; echo $PATH" +prop_checkVariableCanBeReadonly20 = verifyNotTree checkVariableCanBeReadonly "declare -i num=100; num=200; echo $num" +prop_checkVariableCanBeReadonly21 = verifyNotTree checkVariableCanBeReadonly "declare -r CONFIG=\"config_value\"; CONFIG=\"new_value\"; echo $CONFIG" +prop_checkVariableCanBeReadonly22 = verifyNotTree checkVariableCanBeReadonly "declare -x ENV_VAR=\"environment\"; ENV_VAR=\"new_env\"; echo $ENV_VAR" +prop_checkVariableCanBeReadonly23 = verifyNotTree checkVariableCanBeReadonly "local variable=\"local_val\"; variable=\"new_val\"; echo $variable" +prop_checkVariableCanBeReadonly24 = verifyNotTree checkVariableCanBeReadonly "readonly PI=3.14; PI=3.1415; echo $PI" +prop_checkVariableCanBeReadonly25 = verifyNotTree checkVariableCanBeReadonly "x=$(date); x=$(date +%Y); echo $x" +prop_checkVariableCanBeReadonly26 = verifyNotTree checkVariableCanBeReadonly "x=\"hello\"; x=\"world\"; echo $x" +prop_checkVariableCanBeReadonly27 = verifyTree checkVariableCanBeReadonly "varname=\"y\"; declare $varname=10; declare $varname=20; echo $y" +prop_checkVariableCanBeReadonly28 = verifyNotTree checkVariableCanBeReadonly "source some_script.sh; source some_script.sh; echo $SOME_VARIABLE" +prop_checkVariableCanBeReadonly29 = verifyNotTree checkVariableCanBeReadonly "export -r PERM=\"constant\"; export -r PERM=\"updated\"; echo $PERM" +prop_checkVariableCanBeReadonly30 = verifyNotTree checkVariableCanBeReadonly "arr=(1 2 3); arr=(4 5 6); echo ${arr[0]}" +prop_checkVariableCanBeReadonly31 = verifyNotTree checkVariableCanBeReadonly "declare -a arr=(1 2 3); declare -a arr=(4 5 6); echo ${arr[1]}" +prop_checkVariableCanBeReadonly32 = verifyNotTree checkVariableCanBeReadonly "dict=([key]=\"value\"); dict=([key]=\"new_value\"); echo ${dict[key]}" + +checkVariableCanBeReadonly params t = execWriter (mapM_ infoFor canBeReadonly) + where + flow = variableFlow params + references = nub [stripSuffix name | Reference (base, token, name) <- flow] + assignments = [(base, name, token) | Assignment (base, token, name, _) <- flow, isVariableName name] + canBeReadonly = filter (\(base, name, token) -> name `elem` references && countAssignments name == 1 && not (isDeclaredReadonly base)) assignments + infoFor (_, name, token) = + info (getId token) 2331 $ name ++ " appears to never be assigned after initialization. Consider making it readonly." + stripSuffix = takeWhile isVariableChar + countAssignments name = length (filter (\(_, a, _) -> a == name) assignments) + + isDeclaredReadonly (OuterToken _ (Inner_T_SimpleCommand [] (token:_))) = isDeclaredReadonly token + isDeclaredReadonly (OuterToken _ (Inner_T_NormalWord [(OuterToken _ (Inner_T_Literal "readonly"))])) = True + isDeclaredReadonly (OuterToken _ _) = False + prop_checkSetESuppressed1 = verifyTree checkSetESuppressed "set -e; f(){ :; }; x=$(f)" prop_checkSetESuppressed2 = verifyNotTree checkSetESuppressed "f(){ :; }; x=$(f)" prop_checkSetESuppressed3 = verifyNotTree checkSetESuppressed "set -e; f(){ :; }; x=$(set -e; f)" From d7b3a1f71e9681c87fa1cff9b43af9974070a549 Mon Sep 17 00:00:00 2001 From: Adrian Fluturel Date: Tue, 7 Jan 2025 07:22:31 +0100 Subject: [PATCH 2/4] Change some tests to include 2331 so they don't fail anymore --- src/ShellCheck/Checker.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ShellCheck/Checker.hs b/src/ShellCheck/Checker.hs index 0cfc3ab..b598723 100644 --- a/src/ShellCheck/Checker.hs +++ b/src/ShellCheck/Checker.hs @@ -359,7 +359,7 @@ prop_optionIncludes2 = prop_optionIncludes3 = -- expect 2086, no inclusions provided, so it is reported - [2086] == checkOptionIncludes Nothing "#!/bin/sh\n var='a b'\n echo $var" + [2086, 2331] == checkOptionIncludes Nothing "#!/bin/sh\n var='a b'\n echo $var" prop_optionIncludes4 = -- expect 2086 & 2154, only 2154 included, so only that's reported @@ -520,7 +520,7 @@ prop_hereDocsAreParsedWithoutTrailingLinefeed = 1044 `elem` result prop_hereDocsWillHaveParsedIndices = null result where - result = check "#!/bin/bash\nmy_array=(a b)\ncat <> ./test\n $(( 1 + my_array[1] ))\nEOF" + result = check "#!/bin/bash\nreadonly my_array=(a b)\ncat <> ./test\n $(( 1 + my_array[1] ))\nEOF" prop_rcCanSuppressDfa = null result where From 37ff09aff5953cebb71a86247d7a92bdfe29519b Mon Sep 17 00:00:00 2001 From: Adrian Fluturel Date: Tue, 7 Jan 2025 10:12:36 +0100 Subject: [PATCH 3/4] Add check as an optional tree check --- src/ShellCheck/Analytics.hs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index 2a0418a..e4a7ffb 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -71,7 +71,6 @@ treeChecks = [ ,checkUseBeforeDefinition ,checkAliasUsedInSameParsingUnit ,checkArrayValueUsedAsIndex - ,checkVariableCanBeReadonly ] checker spec params = mkChecker spec params treeChecks @@ -280,6 +279,13 @@ optionalTreeChecks = [ cdPositive = "cat foo | grep bar", cdNegative = "grep bar foo" }, nodeChecksToTreeCheck [checkUuoc]) + + ,(newCheckDescription { + cdName = "check-variable-can-be-readonly", + cdDescription = "Check that a variable can be made readonly if it isn't assigned to.", + cdPositive = "x=3; echo $x", + cdNegative = "readonly x=3; echo $x" + }, checkVariableCanBeReadonly) ] optionalCheckMap :: Map.Map String (Parameters -> Token -> [TokenComment]) From e0fbb8326473fd4ff355771e71b5754c1fe0718d Mon Sep 17 00:00:00 2001 From: Adrian Fluturel Date: Tue, 7 Jan 2025 11:47:23 +0100 Subject: [PATCH 4/4] Fix tests after making the check optional --- src/ShellCheck/Checker.hs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/ShellCheck/Checker.hs b/src/ShellCheck/Checker.hs index b598723..ca66195 100644 --- a/src/ShellCheck/Checker.hs +++ b/src/ShellCheck/Checker.hs @@ -359,7 +359,7 @@ prop_optionIncludes2 = prop_optionIncludes3 = -- expect 2086, no inclusions provided, so it is reported - [2086, 2331] == checkOptionIncludes Nothing "#!/bin/sh\n var='a b'\n echo $var" + [2086] == checkOptionIncludes Nothing "#!/bin/sh\n var='a b'\n echo $var" prop_optionIncludes4 = -- expect 2086 & 2154, only 2154 included, so only that's reported @@ -400,6 +400,12 @@ prop_canEnableOptionalsWithRc = result == [2244] csScript = "#!/bin/sh\n[ \"$1\" ]" } +prop_canEnableCheckForReadonlyVariables = result == [2331] + where + result = checkWithRc "enable=check-variable-can-be-readonly" emptyCheckSpec { + csScript = "#!/bin/sh\na=3\necho \"$a\"" + } + prop_sourcePathRedirectsName = result == [2086] where f "dir/myscript" _ _ "lib" = return "foo/lib" @@ -520,7 +526,7 @@ prop_hereDocsAreParsedWithoutTrailingLinefeed = 1044 `elem` result prop_hereDocsWillHaveParsedIndices = null result where - result = check "#!/bin/bash\nreadonly my_array=(a b)\ncat <> ./test\n $(( 1 + my_array[1] ))\nEOF" + result = check "#!/bin/bash\nmy_array=(a b)\ncat <> ./test\n $(( 1 + my_array[1] ))\nEOF" prop_rcCanSuppressDfa = null result where