Skip to content

Parsing crashes if user keyword has invalid argument specification with type information #5443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
d-biehl opened this issue Jun 1, 2025 · 5 comments

Comments

@d-biehl
Copy link
Contributor

d-biehl commented Jun 1, 2025

Hey,

I've been trying out the new Variable Type Conversion feature in RF 7.3 and ran into a parsing issue that I think isn't handled correctly.

Here's a minimal example:

*** Test Cases ***
first
    a simple keyword with args    2    a string


*** Keywords ***
a simple keyword with args
    [Arguments]    ${a: int}=1    ${b: str}
    Log    ${a}
    Log    ${b}

In this case, I would expect a proper error like:

Non-default argument after default arguments.

Instead, parsing fails completely with the following error:

[ ERROR ] Parsing '.../variable_conversion copy.robot' failed: Type information given to non-existing argument 'b'.

If I remove the type from ${b}, the correct error is shown, so the issue seems to be triggered specifically by the type info.

[ ERROR ] Error in file  '.../variable_conversion.robot' on line 8: Creating keyword 'a simple keyword with args' failed: Invalid argument specification: Non-default argument after default arguments.

Cheers!
Daniel

@d-biehl d-biehl changed the title Parsing fails completely when using type info/conversion with non-default arg after default Parsing fails completely when using type info/conversion with non-default arg after default arg Jun 1, 2025
@pekkaklarck pekkaklarck self-assigned this Jun 2, 2025
@pekkaklarck pekkaklarck added this to the v7.3.1 milestone Jun 2, 2025
@pekkaklarck
Copy link
Member

Thanks for the report. The bug occurs in user keyword argument validation during parsing time. The reason is that when there's an argument without a default value after arguments with default values, the error is just reported and the argument itself is not stored anywhere. The earlier parsed type is, however, preserved, and there's later a a conflict when there's a type for non-existing argument. This particular bug is easy to fix, I've already done that locally, but I want to also go through the argument parsing logic to see are there other similar cases.

Although crashes are always very severe, this bug isn't a regression and it only occurs in an error situation, so I don't think an immediate bug fix release is required.

@pekkaklarck
Copy link
Member

Here's another keyword crashing the parsing even after my fix:

Varargs
    [Arguments]    @{v: int}    @{w}
    Whatever

This can also be fixed easily. If there are even more cases, I'll consider changing the parsing logic to handle them all in a generic manner.

@pekkaklarck pekkaklarck changed the title Parsing fails completely when using type info/conversion with non-default arg after default arg Parsing crashes if user keyword has invalid argument specification with type information Jun 2, 2025
@pekkaklarck
Copy link
Member

Also this one causes a crash:

Kwargs
    [Arguments]    &{k: int}    &{w}
    Whatever

I got all these fixed pretty cleanly. There's room for refactoring, though, and I'll do that separately. My plan is to avoid using search_variable multiple times per argument (use it only once and then operate with the resulting VariableMatch object) which ought to speed up the code a bit.

@d-biehl
Copy link
Contributor Author

d-biehl commented Jun 2, 2025

It looks like it's actually a bit more complex ;-)

I think it's a bit annoying during development when, due to the parser error, the entire file suddenly turns red and I get a confusing error message—even though I'm not finished defining the arguments yet. And based on the error message, I might end up looking in the wrong direction. I don't think it's critical for execution, but it is during development.

Image

I haven't looked at the code yet, but why are the types already being checked at parse time? Isn't that a bit early?

@pekkaklarck
Copy link
Member

It's a very good point that this is a bigger problem during development when you have incomplete data. I still don't think an immediate bug fix release is needed, but we certainly need to create one soon. Perhaps next week or latest the week after, depending on are other severe issues, especially regressions, reported.

This problem isn't directly related to checking types The check that fails is there to detect bugs like this in user code:

@keyword(types={'b': int})
def kw(a):
    ...

The reason this check is executed with user keyword is that and ArgSpec object is created when user keyword arguments are validated and it validates types it gets against the arguments it gets. In this particular case types and arguments weren't in sync.

There is also type validation during parsing time to report bugs like ${var: invalid}. If we support for globally defined custom type converters, or support custom converter registered by libraries also with variables, this validation probably needs to be removed, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants