[issue-89] fix: precision follow-up C5/C7/C9/C3 (#89)

src/falsegreen_robot/scanner.py

@@ -164,6 +164,15 @@ def detect_pyramid_level(model):
 # expected_status values that DISABLE the check (no oracle): the request accepts any outcome.
 _EXPECTED_STATUS_OFF = {"any", "anything"}
 SWALLOW_KEYWORDS = {"run keyword and ignore error", "run keyword and return status"}
+# BuiltIn equality assertions that are tautological when both sides are identical
+# (C7 with a ${var}, C5 with a literal). The typed "As Integers/Numbers/Strings"
+# variants are idiomatic Robot and just as vacuous as plain Should Be Equal (#89).
+SHOULD_BE_EQUAL_KEYWORDS = {
+    "should be equal",
+    "should be equal as integers",
+    "should be equal as numbers",
+    "should be equal as strings",
+}
 # Keywords that UNCONDITIONALLY end the current block: nothing after them in the
 # same body runs. A test case cannot use [Return], so the test-level terminators
 # are the keywords that always stop execution (Fail aborts, Pass Execution
@@ -190,6 +199,11 @@ def detect_pyramid_level(model):
 # e.g. `REGEXP:.*` or `REGEXP:^.*$`. A bare `.*` is NOT this: without REGEXP: it is
 # glob, where `.` is literal, so it only matches messages starting with a dot.
 _CATCH_ALL_REGEXP_RE = re.compile(r"^REGEXP:\s*\^?\(?\.[*+]\??\)?\$?$", re.IGNORECASE)
+# STARTS: / EQUALS: with an EMPTY pattern is a catch-all: every message starts with
+# "" (STARTS: matches any error) and EQUALS: on empty matches messages that are
+# themselves empty - both vacuous oracles for `Expect Error` (#89). A non-empty
+# prefix (STARTS:Boom) is specific and stays clean.
+_CATCH_ALL_PREFIX_RE = re.compile(r"^(?:STARTS|EQUALS):\s*$", re.IGNORECASE)
 
 
 def _norm(name):
@@ -321,6 +335,27 @@ def is_swallow(keyword):
     return _norm(keyword) in SWALLOW_KEYWORDS
 
 
+# Soft-assert wrappers that take `<inner_kw>  *inner_args` and still let the test
+# fail: a swallow nested under one is the same dropped-status C3, so peel one layer
+# to find the real keyword (#89 P3). Warn On Failure is excluded on purpose - it
+# already forces the test green, so nesting adds nothing to detect there.
+_SOFT_ASSERT_WRAPPERS = {"run keyword and continue on failure"}
+
+
+def _effective_swallow(call):
+    """Return `(keyword, [args])` of the swallow this call really runs, peeling one
+    soft-assert wrapper. `Run Keyword And Continue On Failure  Run Keyword And Return
+    Status  ...` -> the inner Return Status call. Returns None when it is not a
+    swallow (directly or wrapped)."""
+    kw = getattr(call, "keyword", None)
+    args = list(getattr(call, "args", None) or ())
+    if is_swallow(kw):
+        return kw, args
+    if _norm(kw) in _SOFT_ASSERT_WRAPPERS and args and is_swallow(args[0]):
+        return args[0], args[1:]
+    return None
+
+
 _VAR_NAME_RE = re.compile(r"[\$@&]\{([^{}]+)\}")
 
 
@@ -378,7 +413,7 @@ def _swallow_status_unused(calls, node=None):
     condition_names = _control_condition_names(node) if node is not None else set()
     swallows = []
     for idx, c in enumerate(calls):
-        if type(c).__name__ != "KeywordCall" or not is_swallow(c.keyword):
+        if type(c).__name__ != "KeywordCall" or _effective_swallow(c) is None:
             continue
         assigned = list(getattr(c, "assign", None) or ())
         if not assigned:
@@ -938,7 +973,8 @@ def _call_level_smells(file, owner, calls, findings, local_keywords=None, extra_
             continue
         if _norm(kw) == "run keyword and expect error" and args \
                 and (_CATCH_ALL_ERROR_RE.match((args[0] or "").strip())
-                     or _CATCH_ALL_REGEXP_RE.match((args[0] or "").strip())):
+                     or _CATCH_ALL_REGEXP_RE.match((args[0] or "").strip())
+                     or _CATCH_ALL_PREFIX_RE.match((args[0] or "").strip())):
             findings.append(Finding(file, ln, owner, "C9", "expects any error (catch-all pattern)"))
             has_verification = True
             continue
@@ -956,7 +992,7 @@ def _call_level_smells(file, owner, calls, findings, local_keywords=None, extra_
                                             "expected_status=%s accepts any HTTP status" % off))
                 has_verification = True
                 continue
-        if _norm(kw) == "should be equal" and len(args) >= 2:
+        if _norm(_strip_library_prefix(kw, local_keywords)) in SHOULD_BE_EQUAL_KEYWORDS and len(args) >= 2:
             if args[0] == args[1] and ln not in dead:
                 code = "C7" if args[0].startswith("${") else "C5"
                 findings.append(Finding(file, ln, owner, code, "both sides are identical"))

tests/test_scanner.py

@@ -247,6 +247,52 @@ def test_c7_self_compare(tmp_path):
     assert "C7" in codes(tmp_path, body)
 
 
+# #89 item 1: the typed Should Be Equal As variants are as tautological as the
+# plain form when both sides are identical - C7 with a ${var}, C5 with a literal.
+@pytest.mark.parametrize("kw", [
+    "Should Be Equal As Integers",
+    "Should Be Equal As Numbers",
+    "Should Be Equal As Strings",
+    "BuiltIn.Should Be Equal As Integers",
+])
+def test_c7_typed_should_be_equal_self_compare(tmp_path, kw):
+    body = """\
+*** Test Cases ***
+Self
+    %s    ${x}    ${x}
+""" % kw
+    assert "C7" in codes(tmp_path, body)
+
+
+@pytest.mark.parametrize("kw,val", [
+    ("Should Be Equal As Integers", "5"),
+    ("Should Be Equal As Numbers", "5"),
+    ("Should Be Equal As Strings", "ok"),
+])
+def test_c5_typed_should_be_equal_identical_literals(tmp_path, kw, val):
+    body = """\
+*** Test Cases ***
+Const
+    %s    %s    %s
+""" % (kw, val, val)
+    assert "C5" in codes(tmp_path, body)
+
+
+@pytest.mark.parametrize("step", [
+    "Should Be Equal As Integers    ${a}    ${b}",
+    "Should Be Equal As Numbers    5    6",
+    "Should Be Equal As Strings    foo    bar",
+])
+def test_no_c5_c7_for_distinct_typed_should_be_equal(tmp_path, step):
+    body = """\
+*** Test Cases ***
+Distinct
+    %s
+""" % step
+    found = codes(tmp_path, body)
+    assert "C5" not in found and "C7" not in found
+
+
 def test_c16_sleep(tmp_path):
     body = """\
 *** Test Cases ***
@@ -1065,6 +1111,30 @@ def test_c3_return_status_assigned_but_never_used(tmp_path):
     assert "C3" in codes(tmp_path, body)
 
 
+# #89 item 3: a Return Status swallow nested under a Continue On Failure wrapper is
+# the same dropped-status C3 (HIGH), not the generic C2b (LOW) it used to fall to.
+def test_c3_nested_swallow_under_continue_on_failure(tmp_path):
+    body = """\
+*** Test Cases ***
+Nested Swallow
+    ${status}=    Run Keyword And Continue On Failure    Run Keyword And Return Status    Should Be Equal    ${1}    ${2}
+    Log    done
+"""
+    found = codes(tmp_path, body)
+    assert "C3" in found and "C2b" not in found
+
+
+def test_no_c3_nested_swallow_status_read_later(tmp_path):
+    # The wrapped status IS consumed afterwards - not swallowed, so no C3.
+    body = """\
+*** Test Cases ***
+Nested Swallow Used
+    ${status}=    Run Keyword And Continue On Failure    Run Keyword And Return Status    Should Be Equal    ${1}    ${2}
+    Should Be True    ${status}
+"""
+    assert "C3" not in codes(tmp_path, body)
+
+
 def test_c3_status_form_in_keyword(tmp_path):
     body = """\
 *** Keywords ***
@@ -1195,6 +1265,28 @@ def test_no_c9_when_expect_error_equals_a_literal_star(tmp_path):
     assert "C9" not in codes(tmp_path, body)
 
 
+# #89 item 2: STARTS: / EQUALS: with an EMPTY pattern is a catch-all. Every message
+# starts with "", so STARTS: accepts any error - a vacuous oracle (C9).
+@pytest.mark.parametrize("pat", ["STARTS:", "STARTS: ", "EQUALS:", "starts:"])
+def test_c9_expect_error_empty_prefix_is_catch_all(tmp_path, pat):
+    body = """\
+*** Test Cases ***
+Expects Any Error Via Empty Prefix
+    Run Keyword And Expect Error    %s    Do Risky Thing
+""" % pat
+    assert "C9" in codes(tmp_path, body), pat
+
+
+def test_no_c9_when_expect_error_starts_prefix_is_nonempty(tmp_path):
+    # STARTS: with a real prefix stays a specific matcher - not a catch-all.
+    body = """\
+*** Test Cases ***
+Expects A Real Prefix
+    Run Keyword And Expect Error    STARTS: connection    Do Risky Thing
+"""
+    assert "C9" not in codes(tmp_path, body)
+
+
 def test_c9_expect_error_regexp_catch_all(tmp_path):
     # A REGEXP catch-all (.* / .+ / ^.*$) matches any message, so the oracle is
     # vacuous just like the glob star - C9.