Repository: MarketSquare/robotframework-robocop

Issue:
[Bug] Rule [E] 0933 Undefined argument value falsely reports strings
### What happened?

After updating robocop and running it on my testset I found some situations in which the rule **0933** does not work as expected. A string, ending with **=** seems to always be regarded as setting a variable.
E.g. the **Replace String** keyword leads to an unjustly error. but also keywords from my own library had that result.

### What command/code did you try to run?

```
${Element}    Replace String    ${Element}    xpath=    xpath=(

Push Buttons    C${expression}=

```
### What is the full error message?

[E] 0933 Undefined argument value, use xpath=${EMPTY} instead (undefined-argument-value)

[E] 0933 Undefined argument value, use C${expression}=${EMPTY} instead (undefined-argument-value)

### What did you expect to happen instead?

No error

### Operating System

Windows

### Robocop version

5.7.0

Hints from issue comments:
@bhirsz: This error makes sense, though it may be difficult to get it fixed. Robot Framework doesn't know if argument is named argument or not until it doesn't resolve final keyword signature. In our rule we simply check for existence of '='. 

On other side, if I read your code I potentially also don't know if it's "xpath" argument receiving "", "(", or simply argument with such string. Would it work with escaped ``\=`` sign? @bhirsz: @Lakitna Tagging 

Also molsky added another example:

```
Error: ${test}= Get Text xpath=(//h4)[5] *= min
No error: ${test}= Get Text xpath=(//h4)[5] *=min
```@emilezwiggelaar: I didn't realise that the mark-up removed the spacing. I changed it now in my original report...

What I need in that case is that the xpath which I defined in ${Element} needs to be altered and one of the steps is adding a ( between **xpath=** and the actual locator for the element. I have tried escaping it with an \ :

```
    VAR    ${Element}    xpath=dummy
    ${Element}    Replace String    ${Element}    xpath\=    xpath=(
    Log    ${Element}    console
```
And that works without error 0933@Lakitna: There are indeed limitations in what we can check for. This is caused by not knowing which argument you're trying to set. In this case, you're using `Replace String` with positional arguments, so `xpath=` is the `search_for` argument. But the code does not know that you're setting the `search_for` argument so it assumes that you're trying to set the `xpath` argument.

There are 2 ways of giving the rule more context so it can see that this is not an empty argument:

- `xpath\=` Escaping the `=`
- `search_for=xpath=` Make it a named argument instead

`C${expression}=` may be fixable, if we're ok with ignoring empty arguments for argument names defined in a variable:

```robotframework
${arg_name} =    Set Variable    replace_with
${element}    Replace String    ${element}    search_for=xpath=    ${arg_name}=
```

This example is technically equivalent to:

```robotframework
${element}    Replace String    ${element}    search_for=xpath=    replace_with=
```

`C${expression}=` falls into the same category.

If we don't care about detecting this pattern, I can check if every character before the `=` is a valid argument name character (from memory: `a-zA-Z0-9_-`). Ignoring argument names from variables will make the rule less correct, but probably more useful. What do you think @bhirsz?

---

Finally, molsky's case:

Looking at the keyword docs, your whitespace looks like:

```robotframework
${test}=    Get Text    xpath=(//h4)[5]    *=    min
```

I did not consider this situation.. For now, you should be able to use `*\=`, but this one needs a code fix. For it to be a named argument, it should have at least one valid argument name character before the first `=`.@bhirsz: Looking for valid arguments characters should help to filter out false positives so I'm in favour.

Also, we may want to add it to rule documentation - example where the rule may trip and how to avoid (like you wrote, either escape or introduce named argument)

Solution patch:
diff --git a/robocop/checkers/misc.py b/robocop/checkers/misc.py
index 95f5fdb..10a18f7 100644
--- a/robocop/checkers/misc.py
+++ b/robocop/checkers/misc.py
@@ -1,6 +1,7 @@
 """Miscellaneous checkers"""
 
 import ast
+import re
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Optional
@@ -882,12 +883,20 @@ rules = {
         misreading your keyword arguments, explicitly state that the value is empty using the
         built-in ``${EMPTY}`` variable.
 
-        If your argument is falsly flagged by this rule, escape the ``=`` character in your argument
-        value by like so: ``\\=``.
-
         Example of a rule violation::
 
             My Amazing Keyword    argument_name=
+
+        Positional arguments that end with a ``=`` character can be falsly flagged by this rule. You
+        can fix this by making the situation more explicit:
+
+        1. Escape the ``=`` character::
+
+            Log    value\\=
+
+        2. Make it a named argument instead::
+
+            Log    message=value=
         """,
     ),
 }
@@ -1997,6 +2006,7 @@ class UndefinedArgumentDefaultChecker(VisitorChecker):
         "undefined-argument-default",
         "undefined-argument-value",
     )
+    valid_argument_name = re.compile(r"[a-zA-Z0-9-_ ]+")
 
     def visit_Arguments(self, node: Arguments):  # noqa: N802
         for token in node.get_tokens(Token.ARGUMENT):
@@ -2034,10 +2044,19 @@ class UndefinedArgumentDefaultChecker(VisitorChecker):
                 # `=` is escaped
                 continue
 
+            if arg_name.endswith(" "):
+                # Space before `=` is not a named arg
+                continue
+
             if default_val != "":
                 # Has a value
                 continue
 
+            is_plain_var_name = self.valid_argument_name.fullmatch(arg_name)
+            if is_plain_var_name is None:
+                # Argument name includes invalid chars
+                continue
+
             # Falsly triggers if a positional argument ends with `=`
             # The language server has the same behavior
             self.report(


Test patch:
diff --git a/tests/atest/rules/misc/undefined_argument_value/expected_output.txt b/tests/atest/rules/misc/undefined_argument_value/expected_output.txt
index 30d3a5b..a4a6a7d 100644
--- a/tests/atest/rules/misc/undefined_argument_value/expected_output.txt
+++ b/tests/atest/rules/misc/undefined_argument_value/expected_output.txt
@@ -2,5 +2,4 @@ test.robot:3:12 [E] 0933 Undefined argument value, use message=${EMPTY} instead
 test.robot:4:12 [E] 0933 Undefined argument value, use message=${EMPTY} instead
 test.robot:4:24 [E] 0933 Undefined argument value, use level=${EMPTY} instead
 test.robot:5:27 [E] 0933 Undefined argument value, use level=${EMPTY} instead
-test.robot:6:12 [E] 0933 Undefined argument value, use A great log message=${EMPTY} instead
-test.robot:7:12 [E] 0933 Undefined argument value, use A great log message =${EMPTY} instead
\ No newline at end of file
+test.robot:6:12 [E] 0933 Undefined argument value, use A great log message=${EMPTY} instead
\ No newline at end of file