Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions launch/launch/substitutions/python_expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"""Module for the PythonExpression substitution."""

import collections.abc
import itertools
import math
from typing import Iterable
from typing import List
Expand All @@ -25,6 +26,8 @@
from ..some_substitutions_type import SomeSubstitutionsType
from ..substitution import Substitution
from ..utilities import ensure_argument_type
from ..utilities import normalize_to_list_of_substitutions
from ..utilities import perform_substitutions


@expose_substitution('eval')
Expand All @@ -47,15 +50,17 @@ def __init__(self, expression: SomeSubstitutionsType) -> None:
'expression',
'PythonExpression')

from ..utilities import normalize_to_list_of_substitutions
self.__expression = normalize_to_list_of_substitutions(expression)

@classmethod
def parse(cls, data: Iterable[SomeSubstitutionsType]):
"""Parse `PythonExpression` substitution."""
if len(data) != 1:
raise TypeError('eval substitution expects 1 argument')
return cls, {'expression': data[0]}
if len(data) == 0:
raise TypeError('eval substitution expects 1 or more argument')
elif len(data) == 1:
return cls, {'expression': data[0]}
expression = normalize_to_list_of_substitutions(itertools.chain.from_iterable(data))
return cls, {'expression': expression}

@property
def expression(self) -> List[Substitution]:
Expand All @@ -68,5 +73,12 @@ def describe(self) -> Text:

def perform(self, context: LaunchContext) -> Text:
"""Perform the substitution by evaluating the expression."""
from ..utilities import perform_substitutions
return str(eval(perform_substitutions(context, self.expression), {}, math.__dict__))
expressions = [perform_substitutions(context, [exp]) for exp in self.expression]

if len(expressions) == 3 and (expressions[1] in ['==', '!=']):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenji-miyake I understand it's the easy path forward, but we can't merge a barely-implemented feature. IMHO, if we want $(eval) to be able to use launch configurations as variables, we need to inject launch configurations in eval()'s local scope.

CC @ivanpauno @wjwwood

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to inject launch configurations in eval()'s local scop

Yeah that sounds better, the current proposal is too ad-hoc.

I'm not sure whether I should support $(eval value == 'abc') as well, but I guess it's a bit difficult because the current system seems to automatically drop ''.

Maybe this is a problem (?).
We probably need to modify the launch substitutions grammar to make it more flexible, which will require a bit of thinking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your reviews!

we need to inject launch configurations in eval()'s local scop

Is there any sample code that does this? Unfortunately, I'm not so familiar with the design concept of launch. 😢

Anyway, I'll drop expanding LaunchConfiguration from this PR for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted by 4cc38be.
@hidmic @ivanpauno Would you please teach me what else should I change? 🙏

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hidmic @ivanpauno Friendly ping. Could you review this again? 🙇

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hidmic Thank you for your comment!

What if an expression is supposed to expand to, the name of some constant like pi?

Hmm, I didn't care about the case. But I guess just adding an eval with try-cache can support it? 🤔

IMO to have this feature we need to revisit that grammar.

Yes, I agree with that. (I've changed this to a draft for now.)
How can I build a consensus on the syntax and move things forward?
Are there any design concept documents or plans for the eval syntax?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, how about adding eq and ne substitutions like #598?

$(eq $(var foo) 'bar')
$(ne $(var foo) 'bar')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenji-miyake first of all, my apologies for the insane delay. I've been uber busy.

Now, circling back to this.

How can I build a consensus on the syntax and move things forward?

A concrete proposal is the way to go. You want to simplify notation by making $(var name) and name equivalent in $(eval ...) context. We agree with the intent, but we need a fully functional implementation i.e. not just for a few operators. One possible approach is to inject launch configurations in eval scope.

That doesn't solve the quoting issue though. In launch substitutions' grammar, arguments are delimited by spaces and so you need quotes to pass a single argument that includes a space e.g. '/my/very/unhelpful path'. Quotes must be dropped in these cases. When an $(eval) expression is not quoted, the parser will try to split expression into space delimited arguments, potentially dropping quotes from string literals, and the rest follows.

The only easy way I see we could avoid this is by instructing the substitution parser to skip the entire $(eval) expression. That means, no nested substitutions, tracking left and right parenthesis, and coming up with a special escaping mechanism for parenthesis so that something like $(anon $(eval data() + ')')) doesn't fall apart.

Are there any design concept documents or plans for the eval syntax?

IIRC there are no design documents about the syntax itself. Only the grammar.

how about adding eq and ne substitutions

That's definitely a simpler (better?) path forward.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hidmic Hi, welcome back! No problem, I know ROS maintainers are quite busy. 😢
And thank you for your detailed explanation. But it's super difficult for me... I have to learn the grammar a lot.

That's definitely a simpler (better?) path forward.

Oh, it's good to hear that. Can I send a new PR to add eq/ne?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eq/ne seem to be implemented in #649.

left = expressions[0]
right = expressions[2]
expression = f"'{left}' {expressions[1]} '{right}'"
return str(eval(expression, {}, math.__dict__))

return str(eval(''.join(expressions), {}, math.__dict__))
36 changes: 36 additions & 0 deletions launch/test/launch/frontend/test_substitutions.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from launch import SomeSubstitutionsType
from launch import Substitution
from launch.actions import ExecuteProcess
from launch.actions import SetLaunchConfiguration
from launch.frontend.expose import expose_substitution
from launch.frontend.parse_substitution import parse_if_substitutions
from launch.frontend.parse_substitution import parse_substitution
Expand Down Expand Up @@ -205,6 +206,12 @@ def test_eval_subst():
assert 'asdbsd' == expr.perform(LaunchContext())


def test_eval_subst_empty():
from lark.visitors import VisitError
with pytest.raises(VisitError):
parse_substitution(r'$(eval)')


def test_eval_subst_of_math_expr():
subst = parse_substitution(r'$(eval "ceil(1.3)")')
assert len(subst) == 1
Expand All @@ -213,6 +220,35 @@ def test_eval_subst_of_math_expr():
assert '2' == expr.perform(LaunchContext())


def test_eval_equal():
context = LaunchContext()
SetLaunchConfiguration('value', 'abc').execute(context)

subst = parse_substitution(r"$(eval $(var value) == 'abc')")
assert len(subst) == 1
expr = subst[0]
assert isinstance(expr, PythonExpression)
assert 'True' == expr.perform(context)

subst = parse_substitution(r"$(eval $(var value) == 'def')")
assert len(subst) == 1
expr = subst[0]
assert isinstance(expr, PythonExpression)
assert 'False' == expr.perform(context)

subst = parse_substitution(r"$(eval $(var value) != 'abc')")
assert len(subst) == 1
expr = subst[0]
assert isinstance(expr, PythonExpression)
assert 'False' == expr.perform(context)

subst = parse_substitution(r"$(eval $(var value) != 'def')")
assert len(subst) == 1
expr = subst[0]
assert isinstance(expr, PythonExpression)
assert 'True' == expr.perform(context)


def expand_cmd_subs(cmd_subs: List[SomeSubstitutionsType]):
return [perform_substitutions_without_context(x) for x in cmd_subs]

Expand Down