Rules no-global-variable, no-suite-variable, and no-test-variable

robocop/checkers/misc.py

@@ -699,6 +699,151 @@
                 Keyword1
         """,
     ),
+    "0929": Rule(
+        rule_id="0929",
+        name="no-global-variable",
+        msg="Don't set global variables outside the variables section",
+        severity=RuleSeverity.WARNING,
+        added_in_version="5.6.0",
+        docs="""
+        Setting or updating global variables in a test/keyword often leads to hard-to-understand
+        code. In most cases, you're better off using local variables.
+
+        Changes in global variables during a test are hard to track because you must remember what's
+        happening in multiple pieces of code at once. A line in a seemingly unrelated file can mess
+        up your understanding of what the code should be doing.
+
+        Local variables don't suffer from this issue because they are always created in the
+        keyword/test you're looking at.
+
+        In this example, the keyword changes the global variable. This will cause the test to fail.
+        Looking at just the test, it's unclear why the test fails. It only becomes clear if you also
+        remember the seemingly unrelated keyword.
+
+            *** Variables ***
+            ${hello}    Hello, world!
+
+            *** Test Cases ***
+            My Amazing Test
+                Do A Thing
+                Should Be Equal    ${hello}    Hello, world!
+
+            *** Keywords ***
+            Do A Thing
+                Set Global Variable    ${hello}    Goodnight, moon!
+
+        Using the VAR-syntax:
+
+            *** Variables ***
+            ${hello}    Hello, world!
+
+            *** Test Cases ***
+            My Amazing Test
+                Do A Thing
+                Should Be Equal    ${hello}    Hello, world!
+
+            *** Keywords ***
+            Do A Thing
+                VAR    ${hello}    Goodnight, moon!    scope=GLOBAL
+
+        In some specific situations, global variables are a great tool. But most of the time, it
+        makes code needlessly hard to understand.
+        """,
+    ),
+    "0930": Rule(
+        rule_id="0930",
+        name="no-suite-variable",
+        msg="Don't use suite variables",
+        severity=RuleSeverity.WARNING,
+        added_in_version="5.6.0",
+        docs="""
+        Using suite variables in a test/keyword often leads to hard-to-understand code. In most
+        cases, you're better off using local variables.
+
+        Changes in suite variables during a test are hard to track because you must remember what's
+        happening in multiple pieces of code at once. A line in a seemingly unrelated file can mess
+        up your understanding of what the code should be doing.
+
+        Local variables don't suffer from this issue because they are always created in the
+        keyword/test you're looking at.
+
+        In this example, the keyword changes the suite variable. This will cause the test to fail.
+        Looking at just the test, it's unclear why the test fails. It only becomes clear if you also
+        remember the seemingly unrelated keyword.
+
+            *** Test Cases ***
+            My Amazing Test
+                Set Suite Variable    ${hello}    Hello, world!
+                Do A Thing
+                Should Be Equal    ${hello}    Hello, world!
+
+            *** Keywords ***
+            Do A Thing
+                Set Suite Variable    ${hello}    Goodnight, moon!
+
+        Using the VAR-syntax:
+
+            *** Test Cases ***
+            My Amazing Test
+                VAR    ${hello}    Hello, world!    scope=SUITE
+                Do A Thing
+                Should Be Equal    ${hello}    Hello, world!
+
+            *** Keywords ***
+            Do A Thing
+                VAR    ${hello}    Goodnight, moon!    scope=SUITE
+
+        In some specific situations, suite variables are a great tool. But most of the time, it
+        makes code needlessly hard to understand.
+        """,
+    ),
+    "0931": Rule(
+        rule_id="0931",
+        name="no-test-variable",
+        msg="Don't use test/task variables",
+        severity=RuleSeverity.WARNING,
+        added_in_version="5.6.0",
+        docs="""
+        Using test/task variables in a test/keyword often leads to hard-to-understand code. In most
+        cases, you're better off using local variables.
+
+        Changes in test/task variables during a test are hard to track because you must remember what's
+        happening in multiple pieces of code at once. A line in a seemingly unrelated file can mess
+        up your understanding of what the code should be doing.
+
+        Local variables don't suffer from this issue because they are always created in the
+        keyword/test you're looking at.
+
+        In this example, the keyword changes the test/task variable. This will cause the test to fail.
+        Looking at just the test, it's unclear why the test fails. It only becomes clear if you also
+        remember the seemingly unrelated keyword.
+
+            *** Test Cases ***
+            My Amazing Test
+                Set Test Variable    ${hello}    Hello, world!
+                Do A Thing
+                Should Be Equal    ${hello}    Hello, world!
+
+            *** Keywords ***
+            Do A Thing
+                Set Test Variable    ${hello}    Goodnight, moon!
+
+        Using the VAR-syntax:
+
+            *** Test Cases ***
+            My Amazing Test
+                VAR    ${hello}    Hello, world!    scope=TEST
+                Do A Thing
+                Should Be Equal    ${hello}    Hello, world!
+
+            *** Keywords ***
+            Do A Thing
+                VAR    ${hello}    Goodnight, moon!    scope=TEST
+
+        In some specific situations, test/task variables are a great tool. But most of the time, it
+        makes code needlessly hard to understand.
+        """,
+    ),
 }
 
 
@@ -1725,3 +1870,72 @@
                 max_order_indicator = this_node_expected_order
 
     visit_Keyword = visit_TestCase = check_order  # noqa: N815
+
+
+class NonLocalVariableChecker(VisitorChecker):
+    reports = (
+        "no-global-variable",
+        "no-suite-variable",
+        "no-test-variable",
+    )
+    non_local_variable_keywords = {
+        "setglobalvariable",
+        "setsuitevariable",
+        "settestvariable",
+        "settaskvariable",
+    }
+
+    def visit_KeywordCall(self, node: KeywordCall):  # noqa: N802
+        keyword_token = node.get_token(Token.KEYWORD)
+        if not keyword_token:
+            return
+
+        keyword_name = normalize_robot_name(keyword_token.value, remove_prefix="builtin.")
+        if keyword_name not in self.non_local_variable_keywords:
+            return
+
+        if keyword_name == "setglobalvariable":
+            self._report("no-global-variable", keyword_token)
+            return
+
+        if keyword_name == "setsuitevariable":
+            self._report("no-suite-variable", keyword_token)
+            return
+
+        if keyword_name in ["settestvariable", "settaskvariable"]:
+            self._report("no-test-variable", keyword_token)
+            return
+
+    def visit_Var(self, node):  # noqa: N802
+        """Visit VAR syntax introduced in Robot Framework 7. Is ignored in Robot < 7"""
+        if not node.scope:
+            return
+
+        scope = node.scope.upper()
+        if scope == "LOCAL":
+            return
+
+        option_token = node.get_token(Token.OPTION)
+
+        if scope == "GLOBAL":
+            self._report("no-global-variable", option_token)
+            return
+
+        if scope in ["SUITE", "SUITES"]:
+            self._report("no-suite-variable", option_token)
+            return
+
+        if scope in ["TEST", "TASK"]:
+            self._report("no-test-variable", option_token)
+            return
+
+        # Unexpected scope, or variable-defined scope
+
+    def _report(self, rule_name: str, node):
+        self.report(
+            rule_name,
+            node=node,
+            lineno=node.lineno,
+            col=node.col_offset + 1,
+            end_col=node.col_offset + len(node.value) + 1,
+        )

tests/atest/rules/misc/no_global_variable/expected_output_builtin_syntax.txt

@@ -0,0 +1,4 @@
+test_builtin_syntax.robot:7:5 [W] 0929 Don't set global variables outside the variables section
+test_builtin_syntax.robot:11:5 [W] 0929 Don't set global variables outside the variables section
+test_builtin_syntax.robot:15:5 [W] 0929 Don't set global variables outside the variables section
+test_builtin_syntax.robot:20:5 [W] 0929 Don't set global variables outside the variables section

tests/atest/rules/misc/no_global_variable/expected_output_var_syntax.txt

@@ -0,0 +1,6 @@
+test_var_syntax.robot:9:44 [W] 0929 Don't set global variables outside the variables section
+test_var_syntax.robot:10:44 [W] 0929 Don't set global variables outside the variables section
+test_var_syntax.robot:14:12 [W] 0929 Don't set global variables outside the variables section
+test_var_syntax.robot:16:31 [W] 0929 Don't set global variables outside the variables section
+test_var_syntax.robot:18:27 [W] 0929 Don't set global variables outside the variables section
+test_var_syntax.robot:24:44 [W] 0929 Don't set global variables outside the variables section

tests/atest/rules/misc/no_global_variable/test_builtin_syntax.robot

@@ -0,0 +1,20 @@
+*** Variables ***
+${amazing}    Hello world!
+
+*** Test Cases ***
+Test
+    ${amazing} =    Set Variable    Hello universe
+    Set Global Variable    ${amazing}
+
+Using BuiltIn library prefix
+    ${amazing} =    Set Variable    Hello universe
+    BuiltIn.Set Global Variable    ${amazing}
+
+Using underscores
+    ${amazing} =    Set Variable    Hello universe
+    Set_Global_Variable    ${amazing}
+
+*** Keywords ***
+Keyword
+    ${amazing} =    Set Variable    Hello universe
+    Set Global Variable    ${amazing}

tests/atest/rules/misc/no_global_variable/test_rule.py

@@ -0,0 +1,11 @@
+from tests.atest.utils import RuleAcceptance
+
+
+class TestRuleAcceptance(RuleAcceptance):
+    def test_rule_builtin_syntax(self):
+        self.check_rule(src_files=["test_builtin_syntax.robot"], expected_file="expected_output_builtin_syntax.txt")
+
+    def test_rule_var_syntax(self):
+        self.check_rule(
+            src_files=["test_var_syntax.robot"], expected_file="expected_output_var_syntax.txt", target_version=">=7"
+        )

tests/atest/rules/misc/no_global_variable/test_var_syntax.robot

@@ -0,0 +1,24 @@
+*** Variables ***
+${amazing}    Hello world!
+${scope}    GLOBAL
+
+*** Test Cases ***
+Test
+    VAR    ${foo}    bar
+    VAR    ${lorum}    ipsum    scope=LOCAL
+    VAR    ${amazing}    Hello universe    scope=GLOBAL
+    VAR    ${amazing}    Hello universe    scope=global
+
+Using misc ways of writing weird things
+    VAR
+    VAR    scope=GLOBAL
+    VAR    $without_braces
+    VAR    $without_braces    scope=GLOBAL
+    VAR    ${no_value}    scope=invalid_scope
+    VAR    ${no_value}    scope=GLOBAL
+    VAR    ${no_value}    scope=${scope}
+    VAR    ${no_value}    scope=${{ 'GLOBAL' }}
+
+*** Keywords ***
+Keyword
+    VAR    ${amazing}    Hello universe    scope=GLOBAL

tests/atest/rules/misc/no_suite_variable/expected_output_builtin_syntax.txt

@@ -0,0 +1,4 @@
+test_builtin_syntax.robot:7:5 [W] 0930 Don't use suite variables
+test_builtin_syntax.robot:11:5 [W] 0930 Don't use suite variables
+test_builtin_syntax.robot:15:5 [W] 0930 Don't use suite variables
+test_builtin_syntax.robot:20:5 [W] 0930 Don't use suite variables

tests/atest/rules/misc/no_suite_variable/expected_output_var_syntax.txt

@@ -0,0 +1,7 @@
+test_var_syntax.robot:9:44 [W] 0930 Don't use suite variables
+test_var_syntax.robot:10:44 [W] 0930 Don't use suite variables
+test_var_syntax.robot:13:44 [W] 0930 Don't use suite variables
+test_var_syntax.robot:17:12 [W] 0930 Don't use suite variables
+test_var_syntax.robot:19:31 [W] 0930 Don't use suite variables
+test_var_syntax.robot:21:27 [W] 0930 Don't use suite variables
+test_var_syntax.robot:27:44 [W] 0930 Don't use suite variables

tests/atest/rules/misc/no_suite_variable/test_builtin_syntax.robot

@@ -0,0 +1,20 @@
+*** Variables ***
+${amazing}    Hello world!
+
+*** Test Cases ***
+Test
+    ${amazing} =    Set Variable    Hello universe
+    Set Suite Variable    ${amazing}
+
+Using BuiltIn library prefix
+    ${amazing} =    Set Variable    Hello universe
+    BuiltIn.Set Suite Variable    ${amazing}
+
+Using underscores
+    ${amazing} =    Set Variable    Hello universe
+    Set_Suite_Variable    ${amazing}
+
+*** Keywords ***
+Keyword
+    ${amazing} =    Set Variable    Hello universe
+    Set Suite Variable    ${amazing}

tests/atest/rules/misc/no_suite_variable/test_rule.py

@@ -0,0 +1,11 @@
+from tests.atest.utils import RuleAcceptance
+
+
+class TestRuleAcceptance(RuleAcceptance):
+    def test_rule_builtin_syntax(self):
+        self.check_rule(src_files=["test_builtin_syntax.robot"], expected_file="expected_output_builtin_syntax.txt")
+
+    def test_rule_var_syntax(self):
+        self.check_rule(
+            src_files=["test_var_syntax.robot"], expected_file="expected_output_var_syntax.txt", target_version=">=7.1"
+        )

tests/atest/rules/misc/no_suite_variable/test_var_syntax.robot

@@ -0,0 +1,27 @@
+*** Variables ***
+${amazing}    Hello world!
+${scope}    SUITE
+
+*** Test Cases ***
+Test
+    VAR    ${foo}    bar
+    VAR    ${lorum}    ipsum    scope=LOCAL
+    VAR    ${amazing}    Hello universe    scope=SUITE
+    VAR    ${amazing}    Hello universe    scope=suite
+
+Test plural
+    VAR    ${amazing}    Hello universe    scope=SUITES
+
+Using misc ways of writing weird things
+    VAR
+    VAR    scope=SUITE
+    VAR    $without_braces
+    VAR    $without_braces    scope=SUITE
+    VAR    ${no_value}    scope=invalid_scope
+    VAR    ${no_value}    scope=SUITE
+    VAR    ${no_value}    scope=${scope}
+    VAR    ${no_value}    scope=${{ 'SUITE' }}
+
+*** Keywords ***
+Keyword
+    VAR    ${amazing}    Hello universe    scope=SUITE

tests/atest/rules/misc/no_test_variable/expected_output_builtin_syntax.txt

@@ -0,0 +1,5 @@
+test_builtin_syntax.robot:7:5 [W] 0931 Don't use test/task variables
+test_builtin_syntax.robot:11:5 [W] 0931 Don't use test/task variables
+test_builtin_syntax.robot:15:5 [W] 0931 Don't use test/task variables
+test_builtin_syntax.robot:19:5 [W] 0931 Don't use test/task variables
+test_builtin_syntax.robot:24:5 [W] 0931 Don't use test/task variables

tests/atest/rules/misc/no_test_variable/expected_output_var_syntax.txt

@@ -0,0 +1,7 @@
+test_var_syntax.robot:9:44 [W] 0931 Don't use test/task variables
+test_var_syntax.robot:10:44 [W] 0931 Don't use test/task variables
+test_var_syntax.robot:13:44 [W] 0931 Don't use test/task variables
+test_var_syntax.robot:17:12 [W] 0931 Don't use test/task variables
+test_var_syntax.robot:19:31 [W] 0931 Don't use test/task variables
+test_var_syntax.robot:21:27 [W] 0931 Don't use test/task variables
+test_var_syntax.robot:27:44 [W] 0931 Don't use test/task variables