Repository: MarketSquare/robotframework-robocop (python)

Issue:
[Bug] Rule unnecessary-string-conversion is inherently bad
### What happened?

Hi,

`"${status}" == "200"` and `$status == 200` give different results in many cases and you can never know in which case they are equivalent. The situation is even worse when there is a less/greater-than operation instead of the equivalence (the rule applies even to these cases).



### What command/code did you try to run?

See the following test:
```
*** Keywords ***
Check Equal
    [Arguments]  ${value}
    ${match}  Set Variable  True
    IF  "${value}" == "1"
        ${match}  Set Variable  not ${match}
    END
    IF  $value == 1
        ${match}  Set Variable  not ${match}
    END
    Should Be True  ${match}  msg=Expressions do not match for value "${value}"

Check Greater Than
    [Arguments]  ${value}
    ${match}  Set Variable  True
    IF  "${value}" > "5"
        ${match}  Set Variable  not ${match}
    END
    IF  $value > 5
        ${match}  Set Variable  not ${match}
    END
    Should Be True  ${match}  msg=Expressions do not match for value "${value}"

*** Test Cases ***
Test 1
    Check Equal  1

Test 2
    Check Greater Than  6

Test 3
    Check Greater Than  40

Test 4
    Check Greater Than  ${40}
```

All the test cases fail, and that is for various causes. 
- When comparing two strings, you can be sure that they are comparable: they are of the same type. When you remove the string conversion, possibly different types of the two expressions come into play and the comparison can fail because of their mismatching types (as in Test 2 and Test 3). To be sure the two expressions are of compatibile types, you should employ dynamic analysis, I guess.
- Even if the two expressions are comparable without string conversion, their comparison with and without it do not need to yield the same results. In Test 1, `"1" == "1"`, but `"1" != 1` (because of different types, although values are equal). In Test 4, `40 > 5` but `"40" < "5"` (because of the different ways the things are ordered for various data types).

I am afraid it is very tedious or impossible to fix the rule or restrict it to cases when it is always correct, so that I suggest to delete it.

### Operating System

WIndows

### Robocop version

4.2.0

Pull request:
Deprecate unnecessary-string-conversion rule
Closes #986 

Patch:
diff --git a/docs/releasenotes/unreleased/rules.3.rst b/docs/releasenotes/unreleased/rules.3.rst
new file mode 100644
index 0000000..92d3f00
--- /dev/null
+++ b/docs/releasenotes/unreleased/rules.3.rst
@@ -0,0 +1,6 @@
+Deprecate unnecessary-string-conversion rule (#986)
+----------------------------------------------------
+
+Rule I0923 ``unnecessary-string-conversion`` is now deprecated. It reported multiple false positive issues that lead
+to users breaking the code. If the rule was useful in your project and you know its limitations (coming from not knowing
+the exact type of the variable in the IF condition) you can reimplement this rule as custom rule in your project.
diff --git a/docs/rules_list.rst b/docs/rules_list.rst
index 5fbd473..9517047 100644
--- a/docs/rules_list.rst
+++ b/docs/rules_list.rst
@@ -34,13 +34,14 @@ Below is the list of all built-in Robocop rules. Enjoy |:sunglasses:|
 {% for rule_doc in checker_group[1] %}
 .. _{{ rule_doc.name }}:
 
+{{ rule_doc.name }} / {{ rule_doc.severity }}{{ rule_doc.id }}
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 {% if rule_doc.deprecated %}
 .. warning::
 
       Rule is deprecated.
 {% endif %}
-{{ rule_doc.name }} / {{ rule_doc.severity }}{{ rule_doc.id }}
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 *Added in* ``v{{ rule_doc.robocop_version }}`` ⦁ *Supported RF versions*: ``{{ rule_doc.version }}``
 
diff --git a/robocop/checkers/misc.py b/robocop/checkers/misc.py
index 4f9dfeb..3915f15 100644
--- a/robocop/checkers/misc.py
+++ b/robocop/checkers/misc.py
@@ -464,6 +464,7 @@ rules = {
         name="unnecessary-string-conversion",
         msg="Variable '{{ name }}' in '{{ block_name }}' condition has unnecessary string conversion",
         severity=RuleSeverity.INFO,
+        deprecated=True,
         version=">=4.0",
         docs="""
         Expressions in Robot Framework are evaluated using Python's eval function. When a variable is used
@@ -1344,7 +1345,7 @@ class UnusedVariablesChecker(VisitorChecker):
 
 
 class ExpressionsChecker(VisitorChecker):
-    reports = ("unnecessary-string-conversion", "expression-can-be-simplified", "misplaced-negative-condition")
+    reports = ("expression-can-be-simplified", "misplaced-negative-condition")
     QUOTE_CHARS = {"'", '"'}
     CONDITION_KEYWORDS = {"passexecutionif", "setvariableif", "shouldbetrue", "shouldnotbetrue", "skipif"}
     COMPARISON_SIGNS = {"==", "!="}
@@ -1384,21 +1385,6 @@ class ExpressionsChecker(VisitorChecker):
             self.check_for_complex_condition(
                 condition_token, node_name, match.before, match.match, match.after, position
             )
-            if not match.before or not match.after:
-                continue
-            if (
-                match.before[-1] in self.QUOTE_CHARS
-                and match.before[-1] == match.after[0]
-                and not match.match.startswith("%")
-            ):
-                self.report(
-                    "unnecessary-string-conversion",
-                    name=match.match,
-                    block_name=node_name,
-                    node=condition_token,
-                    col=position,
-                    end_col=position + len(match.match),
-                )
 
     def check_for_misplaced_not(self, condition_token, node_name, left_side, variable, right_side):
         """Check if the condition contains misplaced not.
diff --git a/robocop/config.py b/robocop/config.py
index ffc10af..f726e8c 100644
--- a/robocop/config.py
+++ b/robocop/config.py
@@ -526,7 +526,7 @@ class Config:
                 raise exceptions.RuleDoesNotExist(rule, rules) from None
             rule_def = rules[rule]
             if rule_def.deprecated:
-                rule_def.deprecation_warning()
+                print(rule_def.deprecation_warning)
 
     def is_rule_enabled(self, rule: "Rule") -> bool:
         if self.is_rule_disabled(rule):
diff --git a/robocop/rules.py b/robocop/rules.py
index 3ad6b7a..435c3f6 100644
--- a/robocop/rules.py
+++ b/robocop/rules.py
@@ -354,9 +354,10 @@ class Rule:
             desc += self.docs
         return desc
 
+    @property
     def deprecation_warning(self):
         """Used when rule is deprecated and used in configuration."""
-        print(f"Rule {self.severity}{self.rule_id} {self.name} is deprecated. " f"Remove it from your configuration.")
+        return f"Rule {self.severity}{self.rule_id} {self.name} is deprecated. Remove it from your configuration."
 
     def get_severity_with_threshold(self, threshold_value):
         if threshold_value is None:
diff --git a/robocop/run.py b/robocop/run.py
index be21aea..716ac67 100644
--- a/robocop/run.py
+++ b/robocop/run.py
@@ -257,7 +257,7 @@ class Robocop:
             if rule_or_report in self.rules:
                 rule = self.rules[rule_or_report]
                 if rule.deprecated:
-                    rule.deprecation_warning()
+                    print(rule.deprecation_warning)
                 else:
                     rule.configure(param, value)
             elif rule_or_report in self.reports:


Test patch:
diff --git a/tests/atest/rules/misc/unnecessary_string_conversion/expected_output.txt b/tests/atest/rules/misc/unnecessary_string_conversion/expected_output.txt
deleted file mode 100644
index ce02497..0000000
--- a/tests/atest/rules/misc/unnecessary_string_conversion/expected_output.txt
+++ /dev/null
@@ -1,16 +0,0 @@
-test.robot:3:12 [I] 0923 Variable '${variable}' in 'IF' condition has unnecessary string conversion
-test.robot:5:20 [I] 0923 Variable '${variable}' in 'ELSE IF' condition has unnecessary string conversion
-test.robot:7:16 [I] 0923 Variable '${variable}' in 'IF' condition has unnecessary string conversion
-test.robot:17:25 [I] 0923 Variable '${variable}' in 'INLINE IF' condition has unnecessary string conversion
-test.robot:17:75 [I] 0923 Variable '${string_var}' in 'ELSE IF' condition has unnecessary string conversion
-test.robot:20:15 [I] 0923 Variable '${variable}' in 'WHILE' condition has unnecessary string conversion
-test.robot:21:19 [I] 0923 Variable '${variable}' in 'WHILE' condition has unnecessary string conversion
-test.robot:32:27 [I] 0923 Variable '${variable}' in 'Pass Execution If' condition has unnecessary string conversion
-test.robot:33:38 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:34:25 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:35:25 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:36:25 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:36:61 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:39:30 [I] 0923 Variable '${variable}' in 'Should Be True' condition has unnecessary string conversion
-test.robot:42:30 [I] 0923 Variable '${variable}' in 'Should Not Be True' condition has unnecessary string conversion
-test.robot:43:17 [I] 0923 Variable '${variable}' in 'Skip If' condition has unnecessary string conversion
\ No newline at end of file
diff --git a/tests/atest/rules/misc/unnecessary_string_conversion/expected_output_rf4.txt b/tests/atest/rules/misc/unnecessary_string_conversion/expected_output_rf4.txt
deleted file mode 100644
index 1b0647d..0000000
--- a/tests/atest/rules/misc/unnecessary_string_conversion/expected_output_rf4.txt
+++ /dev/null
@@ -1,12 +0,0 @@
-test.robot:3:12 [I] 0923 Variable '${variable}' in 'IF' condition has unnecessary string conversion
-test.robot:5:20 [I] 0923 Variable '${variable}' in 'ELSE IF' condition has unnecessary string conversion
-test.robot:7:16 [I] 0923 Variable '${variable}' in 'IF' condition has unnecessary string conversion
-test.robot:32:27 [I] 0923 Variable '${variable}' in 'Pass Execution If' condition has unnecessary string conversion
-test.robot:33:38 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:34:25 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:35:25 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:36:25 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:36:61 [I] 0923 Variable '${variable}' in 'Set Variable If' condition has unnecessary string conversion
-test.robot:39:30 [I] 0923 Variable '${variable}' in 'Should Be True' condition has unnecessary string conversion
-test.robot:42:30 [I] 0923 Variable '${variable}' in 'Should Not Be True' condition has unnecessary string conversion
-test.robot:43:17 [I] 0923 Variable '${variable}' in 'Skip If' condition has unnecessary string conversion
\ No newline at end of file
diff --git a/tests/atest/rules/misc/unnecessary_string_conversion/test_rule.py b/tests/atest/rules/misc/unnecessary_string_conversion/test_rule.py
index 36da8be..d86874e 100644
--- a/tests/atest/rules/misc/unnecessary_string_conversion/test_rule.py
+++ b/tests/atest/rules/misc/unnecessary_string_conversion/test_rule.py
@@ -2,8 +2,5 @@ from tests.atest.utils import RuleAcceptance
 
 
 class TestRuleAcceptance(RuleAcceptance):
-    def test_rule_rf4(self):
-        self.check_rule(src_files=["test.robot"], expected_file="expected_output_rf4.txt", target_version="==4.*")
-
     def test_rule(self):
-        self.check_rule(src_files=["test.robot"], expected_file="expected_output.txt", target_version=">=5")
+        self.check_rule(src_files=["test.robot"], deprecated=True)
diff --git a/tests/atest/utils/__init__.py b/tests/atest/utils/__init__.py
index 31618cf..a91c788 100644
--- a/tests/atest/utils/__init__.py
+++ b/tests/atest/utils/__init__.py
@@ -13,8 +13,6 @@ from robocop.config import Config
 from robocop.utils.misc import ROBOT_VERSION
 from robocop.utils.version_matching import VersionSpecifier
 
-DEPRECATION_PERIOD = False
-
 
 @contextlib.contextmanager
 def isolated_output():
@@ -43,14 +41,6 @@ def normalize_result(result, test_data):
     return sorted([line.replace(test_data_str, "") for line in result])
 
 
-def remove_deprecation_warning(result):
-    """Remove deprecation warning from the results"""
-    deprecation_regex = (
-        r"(?s)(\#\#\# DEPRECATION WARNING \#\#\#)(.*?)(This information will disappear in the next version\.\n+)"
-    )
-    return re.sub(deprecation_regex, "", result)
-
-
 def load_expected_file(test_data, expected_file):
     if expected_file is None:
         return []
@@ -94,12 +84,13 @@ class RuleAcceptance:
 
     def check_rule(
         self,
-        expected_file,
-        config=None,
-        rule=None,
+        expected_file: Optional[str] = None,
+        config: Optional[str] = None,
+        rule: Optional[str] = None,
         src_files: Optional[List] = None,
-        target_version=None,
-        issue_format="default",
+        target_version: Optional[str] = None,
+        issue_format: str = "default",
+        deprecated: bool = False,
     ):
         if not self.enabled_in_version(target_version):
             pytest.skip(f"Test enabled only for RF {target_version}")
@@ -115,12 +106,11 @@ class RuleAcceptance:
             finally:
                 sys.stdout.flush()
                 result = get_result(output)
-                if DEPRECATION_PERIOD:
-                    parsed_results = remove_deprecation_warning(result).splitlines()
-                else:
-                    parsed_results = result.splitlines()
+                parsed_results = result.splitlines()
         actual = normalize_result(parsed_results, test_data)
-        if actual != expected:
+        if deprecated:
+            assert robocop_instance.rules[self.rule_name].deprecation_warning in actual
+        elif actual != expected:
             missing_expected = sorted(set(actual) - set(expected))
             missing_actual = sorted(set(expected) - set(actual))
             error = "Actual issues are different than expected.\n"
diff --git a/tests/e2e/test_e2e.py b/tests/e2e/test_e2e.py
index e9ce511..fe97355 100644
--- a/tests/e2e/test_e2e.py
+++ b/tests/e2e/test_e2e.py
@@ -372,7 +372,9 @@ class TestTranslatedRobot:
 
     def test_all_builtin_rules_should_be_enabled_by_default(self, robocop_instance):
         robocop_instance.reload_config()
-        builtin_rules = [rule for rule in robocop_instance.rules.values() if not rule.community_rule]
+        builtin_rules = [
+            rule for rule in robocop_instance.rules.values() if not rule.community_rule and not rule.deprecated
+        ]
         assert builtin_rules != []
         disabled_builtin = {rule.name for rule in builtin_rules if not rule.enabled and rule.enabled_in_version}
         assert disabled_builtin == set()
@@ -400,7 +402,9 @@ class TestTranslatedRobot:
 
     def test_builtin_rules_have_matching_category_id(self, robocop_instance):
         robocop_instance.reload_config()
-        builtin_rules = [rule for rule in robocop_instance.rules.values() if not rule.community_rule]
+        builtin_rules = [
+            rule for rule in robocop_instance.rules.values() if not rule.community_rule and not rule.deprecated
+        ]
         assert builtin_rules != []
         errors = []
         for rule in builtin_rules: