Argument should not be converted if its type is one of the accepted types

def foo(s: str = None): ... # type is Union[str, None]
def foo(s: Optional[str]): ... # type is Union[str, None]

When using these very idiomatic/pythonic type hints, it makes it impossible to pass a None from robot because robot will convert it to a string of value "None"

Other unwanted side effects include conversions between float/int:

if you have Union[int, float] and use it with ${42.9} that gets converted to an integer 42.
-pekkaklarck

The purpose of this change was to make a literal int/float/enum etc passed to a function with a union type hint be treated as a literal of that type instead of being passed as a string.

So I suggest avoiding the conversion entirely if the following conditions are met:

the value is not a string
the value is already a valid type from the union

An alternative solution would be to always attempt the string conversion last. I think this approach is a bad idea because it adds more hidden complexity for users.

PR: #3895

I agree ${None} being converted to a string is not good if you use Optional[str], but I wouldn't say that totally breaks it. There's also a simple workaround that you omit Optional and just use arg=None. Anyway, let's discuss this more in #3897.

Our codebase uses type hints, so foo(arg=None) is unacceptable and foo(arg: str=None) is the same type as foo(arg: Optional[str])

The fix is to no longer use idiomatic type hints and instead convert everything to foo(arg: Union[None, str])

This issue is about what to do if an argument accepts multiple types (i.e. typically is an Union) and the given argument already matches one of these types. This was actually discussed as the first design decision in #3738 that introduced this multi type support. This is what I wrote about that subject there:

Should we first check does the given argument match any of the possible types before doing conversion? If, for example, we have arg: Union[str, int] and the given argument is an integer, should we pass it as is (it matches one of the possible types) or should we test conversion with possible types one-by-one? In the latter case we'd convert the integer to a string because the first possible type is str. In this case checking does the argument match any of the types feels better.

On the other hand, if we have arg: Union[int, str] and call with a string 2, checking does the given argument match any of the type would mean that we don't do any conversion because one of the types is str. If we'd go through types one-by-one, we'd convert it to an integer because int is before str. In this case testing conversion one-by-one works better.

We could also make it so that we test conversions one-by-one if the given argument is a string and otherwise first check does the argument match any of the types. That feels a bit inconsistent and hard to explain, though. I got a feeling that going through types one-by-one works best. The key thing to remember is that str should be the last type to avoid converting all arguments to strings.

As you can read above, there are basically three alternatives how to handle this. We decided to go with the middle solution of not checking does the type match and just doing conversion one-by-one. This means that if you have e.g. Union[int, str] and call it with string 2, it gets converted to an integer 2. A problem is that if you have Union[int, float], float 2.9 gets converted into an integer 2.

The PR #3895 initially proposed using the first option. It would mean that if you have Union[int, float] and call it with a float, no conversion is done. That's great, but it would also mean that if you have Union[int, str] and call it with a string 2, no conversion is done either. That's in my opinion worse than the current situation.

The final option listed above would have a benefit of not converting floats if you have e.g. Union[int, float] but converting strings if you have e.g. Union[int, str]. That approach was rejected because it's not as straightforward as the above solutions but since it avoids their problems I start to believe this is the approach to use. PR #3895 has also been updated to use this approach.

The more I think about this, the more I like the option three discussed above and that PR #3895 also nowadays implements. I've initially thought this issue as a feature request changing functionality and in that case it can be earliest done in RF 4.1. I guess we could also consider the current functionality buggy in which case it could be fixed already in RF 4.0.1. The change is backwards incompatible either way, but changing functionality now when Union support has been live only since RF 4.0 could be safer.

If you think this should be a bug fix in RF 4.0.1, add 👍 reaction to this comment, and use 👎 if you think this should wait for RF 4.1. If you think the current functionality shouldn't be changed, add a comment explaining why!

Although Union conversion itself is new in RF 4.0, this bug caused a regression in cases like arg: str = None where Python automatically converts str to Union[str, None] due to the default value (#3908). With earlier releases we had extra code to remove the Union in this case, but the code was removed when Union support was added. The problem is that the Union support is buggy in this regard but once it's fixed also default values are handled correctly.

@aaltat

I've been thinking this more and also discussed the problem with @aaltat who encountered some surprises related to Union support with SeleniumLibrary. We agreed that the current behavior is not acceptable and needs to be changed. We then discussed about the alternatives:

If the given argument isn't a string, check does it have one of the accepted types. This is the solution I have proposed earlier but now I fee that it is a bit too magic, hard to explain, and can cause subtle bugs. For example, if an argument has type like Union[int, str] and its value is got from a variable having string as a value, it can be surprising that the final argument type depends on does the string look like an integer or not. This behavior would also make changes to argument conversion with some type backwards incompatible. For example, if we enhance integer conversion as proposed in Support binary, octal and hex values in argument conversion with int type #3909, string like 0x0 would be considered a hexadecimal value zero.
Always check is the argument one of the accepted types. I have been against this earlier because it means that if an argument has type Union[int, str] and it gets string 42 as a value, it will not be converted. The reason I have considered that a problem is that the common way to call keywords is like Example 42 so most argument are strings. This basically means that if you add str into a Union, in the common case no conversion is done. Considering the problems with other approaches, and the fact that you need to add str there explicitly, I believe this problem is not so bad. A big benefit of this approach is that it's easy to explain.

Based on our discussion, @aaltat and I agreed that it's best to handle Union conversion so that we do no conversion if the given argument has one of the accepted type. This should be done with all arguments without exceptions. Could you @KotlinIsland change PR #3895 so that it behaves like this? I know it originally worked like that and I asked you to change it. This is a surprisingly hard design decision and with different approaches having different pros and cons.

@aaltat

@aaltat already promised to test SeleniumLibrary after this behavior has been changed so that we learn how it affects an existing library in real life. It probably is a good idea to also create RF 4.0.1rc1 to make it easy for others to test this before the final RF 4.0.1 release.

After all this investigation, problems with SeleniumLibrary and the realization that this behavior caused a regression with None default value (#3908), I definitely consider this a pretty high priority bug rather than an enhancement. Since it's a bug, we can fix it in RF 4.0.1 even though the fix itself is backwards incompatible.

I see the need for this conversation for 'literals' ie foo 42 should be an int, but should

${bar} =  42
foo  ${bar}

Be an int or a string?

Maybe there could be a lint warning that a conversion is going to happen here, and include a syntax to disable conversion

${bar} =  42  # ok, bar is a string
foo  ${bar}  # warning: value conversion to int
foo  ${bar:no_conversion}  # special syntax to not convert something (pass as str)

What if RF has type hints like

${bar}: int =  42

I think the core design of RF where everything is a string makes this very complicated.

I think if the ide could inform where conversion are happening it would reduce confusion a lot, maybe conversions could also be included in the report.

In my opinion Robot should not start converting values automatically just because they look like something. For example, something like Foo 42 should mean passing a string to the keyword by default and conversion done only if the keyword itself has type information. The same should happen also with Foo ${bar} - the keyword should get the original value unless it has type info.

We have issue #3278 about adding variable type conversion using syntax like ${COUNT:int}. It's currently targeted for RF 5, but we don't yet have concrete plans when the development of that release will start an what it actually will contain. I agree it would be a handy feature.

Anyway, in this issue we should concentrate on Union conversion. As I wrote earlier, I nowadays think the original approach in PR #3895 is the best one.

Unionon conversion: Don't convert value if it is already a valid type. (

Navigation Menu

Repository navigation

Argument should not be converted if its type is one of the accepted types #3897

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Argument should not be converted if its type is one of the accepted types #3897

Description

Activity

KotlinIsland commented on Mar 19, 2021

pekkaklarck commented on Mar 19, 2021

pekkaklarck commented on Mar 19, 2021

pekkaklarck commented on Mar 26, 2021

pekkaklarck commented on Mar 26, 2021

pekkaklarck commented on Mar 26, 2021

pekkaklarck commented on Mar 26, 2021

KotlinIsland commented on Mar 27, 2021

KotlinIsland commented on Mar 27, 2021

pekkaklarck commented on Mar 29, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions