Validate unparsed attributes and subentities in launch_xml and launch_yaml

[ivanpauno]

Fixes #338.

Adds three methods to launch.frontend.Entity:

• assert_no_children(self)
• assert_subentity_types(self, types: Iterable[str])
• assert_attribute_names(self, names: Iterable[str])

It might make the parse methods a bit more verbose, but it's not hard to use.
I have modified two parsing functions to use this, and it has already caught a preexisting error!

There might be some other ways to achieve the same, so I would like to get feedback about the approach before applying this more broadly.

[jacobperron]

I think it would be nicer to have children and attributes explicitly declared versus calling separate assert functions. For example, by extending the expose_action decorator:

@expose_action(
    'executable',
    allowed_attributes=(
        'cmd',
        'cwd',
        'name',
        'launch-prefix',
        'output',
        'respawn',
        'respawn_delay',
        'shell',
        'additional_env',
        'if',
        'unless'
    ),  
    allowed_children=('env', ))
class ExecuteProcess(Action):
    ...

If allowed_* is empty (or None), then implicitly none are allowed. This way we'll expose bugs in user launch scripts right away; the downside being that we'll break a lot of existing user code, but maybe we could tick-tock the feature, emitting a warning if they haven't explictly declared attributes/children they are accessing.

It might also be nice to have a flag to allow any attributes or children. I'm not sure what that would look like.

I don't know the details of the frontend implementation, so I'm not sure how feasible what I proposed is.

[ivanpauno]

I think it would be nicer to have children and attributes explicitly declared versus calling separate assert functions. For example, by extending the expose_action decorator:

I like that one.
The assert functions in launch.frontend.Entity are needed, but those can be called directly from the description in the decorator.

I will still need to distinguish between allow_subentities (e.g. env in ExecuteProcess), and allow children (e.g. any action in GroupAction).

[hidmic]

First of all, to put things in context, I think that, to solve #338, we have two problems at hand: (1.) to enforce an schema as we parse, and (2.) to correlate any errors with the given input. To complain about a bad tag somewhere in, say, a three levels deep launch file hierarchy is of little use.

Now, this patch addresses the first problem. I agree with @jacobperron's suggestion, but based on my observations, @ivanpauno I (personally) think it's time to go back to that old premise about frontends being standardized 1-to-1 mappings to their Python implementation.

I'd suggest we use the full power of Python's introspection machinery from within our decorators to deduce which attributes and children are valid from __init__ arguments (along with their annotated types) for the decorated class and its base classes, recursively. As a bonus, we get to enforce that 1-to-1 mapping.

[hidmic]

@ivanpauno hmm, do we have to explicitly list base classes' attributes? What's going to happen with the assertions performed by another class that derives from this one?

[ivanpauno]

That's why I used the trick if ignore is None:.
Not a beauty, but I don't know how to do better.

[ivanpauno]

I (personally) think it's time to go back to that old premise about frontends being standardized 1-to-1 mappings to their Python implementation.

I'd suggest we use the full power of Python's introspection machinery from within our decorators to deduce which attributes and children are valid from init arguments (along with their annotated types) for the decorated class and its base classes, recursively. As a bonus, we get to enforce that 1-to-1 mapping.

I don't disagree, but we aren't mapping them 1-to-1 right now, and start doing that would be a breaking change.
In the case of a class with base classes the mechanism isn't obvious (e.g. node accepts different arguments that executable, but some are directly forwarded to the base class).
In that last case, the decorator will still need some user input (a solution to this is to never use kwargs, and always forward arguments explicitly in constructors).

We could still add a more intelligent mechanism that deduce the attribute names from the constructor arguments in the future, I think this improvement wouldn't block that (specially if following @jacobperron suggestion of providing the valid attributes and sub-entities in the decorator).

[ivanpauno]

It's also not obvious how the decorator would guess what's an attribute (name, executable, plugin, etc), what are children subentities (nested actions in group_action) and what are complex attributes (env, param, etc). Maybe there could be some heuristic based on type annotations, but I'm not sure if that will always work,

[hidmic]

I don't disagree, but we aren't mapping them 1-to-1 right now, and start doing that would be a breaking change.

Hmm, aren't we? IIRC that's what we strived for. I'd suggest we don't delay in correcting the deviation. User provided aliases can aid deprecation cycles.

We could still add a more intelligent mechanism that deduce the attribute names from the constructor arguments in the future, I think this improvement wouldn't block that

I agree schema deduction is not strictly necessary. To be clear, I meant it as a proposal to solve the problem. I'm sure there are other solutions, perhaps better ones. We can discuss them. But that is not to say I agree with this patch being an improvement as long as it requires replicating information across inheritance chains like this.

In the case of a class with base classes the mechanism isn't obvious (e.g. node accepts different arguments that executable, but some are directly forwarded to the base class).

One solution to that problem (and perhaps an alternative, explicit, solution to the problem w/o using introspection) is to progressively tag input (at least for validation purposes) as it is accessed. If by the time you've processed an entire subtree, a part of it remains unacknowledged, then you know the system didn't recognize it.

the decorator will still need some user input

This is correct. The canonical example being arbitrary children mapped to an specific argument. I'm not against user input, but against introducing unnecessary coupling in the system.

It's also not obvious how the decorator would guess what's an attribute (name, executable, plugin, etc), what are children subentities (nested actions in group_action) and what are complex attributes (env, param, etc). Maybe there could be some heuristic based on type annotations, but I'm not sure if that will always work,

Once it is clear which (and if an) argument maps to all nested entities (which IIRC cannot coexist with complex attributes), you can deduce attributes at first depth from type annotations. Of course, it cannot deduce the structure of complex attributes -- whose shape may be the result of a parsing function's implementation e.g. nested dictionaries.

[ivanpauno]

One solution to that problem (and perhaps an alternative, explicit, solution to the problem w/o using introspection) is to progressively tag input (at least for validation purposes) as it is accessed. If by the time you've processed an entire subtree, a part of it remains unacknowledged, then you know the system didn't recognize it.

Yeah, I thought about that one.
I will try to implement something like that and see how it works.

[ivanpauno]

I have updated the PR with the approach suggested above, good news is that we don't need to update any parsing function.

[jacobperron]

Generally looks good. I've left some minor comments

[jacobperron]

LGTM. It'd be good to have @hidmic take a look too.

[ivanpauno]

CI, testing all downstream packages of launch:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

LGTM pending green CI!

[ivanpauno]

• Linux
• Linux-aarch64
• macOS
• Windows

[adityapande-1995]

@Mergifyio backport foxy

[mergify]

backport foxy

✅ Backports have been created

• #567 Validate unparsed attributes and subentities in launch_xml and launch_yaml (backport #468) has been created for branch foxy