force logical operator expr which in if-orelse stmtto boolean

[wcsjtu]

type of logical operator stmt in python, such as a and b, a or b is ambiguous. For example

a = ""
b = False
c = a and b     # type(c) is str

a = "1"
c = a and b    # type(c) is bool

In current implementation of pythonic::builtins::pythran::and_,

template <class T0, class T1>
types::lazy_combined_t<T0, T1> and_(T0 &&v0, T1 &&v1)
{
    auto &&val0 = std::forward<T0>(v0)();
    if (val0)
      return (types::lazy_combined_t<T0, T1>)std::forward<T1>(v1)();
    else
      return (types::lazy_combined_t<T0, T1>)val0;
}

when a == "1", variable b will be converted to types::str tmp_str_b implicitly via

template <class T>
str::str(T const &s)
{
    std::ostringstream oss;
    oss << s;
    *data = oss.str();
}

then converted bool tmp_bool_b via

str::operator bool() const
{
    return !data->empty();
}

which always tmp_bool_b === true。so, expr if "1" and False in python is if False, in c++ is if (true)

This pull request force expr in 'if-orelse' stmt to bool(expr) to avoid implicit type coversion in pythonic::builtins::pythran::and_

here is the test code

# pythran export test_and_op(str, str, str)
def test_and_op(s: str, s1: str, s2: str):
    expr = " '%s' and '%s' == '%s'  is "  % (s, s1, s2)
    if s and s1 == s2:
        return expr + "True"
    else:
        return expr + "False"


# pythran export test_or_op(str, str, str)
def test_or_op(s: str, s1: str, s2: str):
    expr = " '%s' or '%s' == '%s'  is "  % (s, s1, s2)
    if s or s1 == s2:
        return expr + "True"
    else:
        return expr + "False"

# test_and_op("a", "2", "1")
# test_or_op("", "2","1")

[serge-sans-paille]

Thanks for the patch. I like the approach, but there are a few things to change to make your code more robust 👍

1. handle IfExp
2. your code shold call builtins.bool and not bool, as bool may refer to any value. See how it's done in

   pythran/pythran/optimizations/comprehension_patterns.py

   Line 52 in 17d9fdb

   ifilterName = ast.Attribute(

3. You're transforming any expression within the tested expression and in the orelse branch. I don't see any reason to do that in the test body / else branch, and even for condition, you should make sure the expression is only evaluated in boolean context, and not in, say, a function argument.

What I propose:

1. have your transformation insert an explicit call to builtins.bool for if and ifexpr test condition
2. design an optimisation pass that folds this calls to builtin.bool when it meets And and Or nodes, using the property:
   bool(x and y) <> bool(x) and bool(y)

Does that sounds valid to you?

[wcsjtu]

@serge-sans-paille thanks for your review !

1. handle IfExp

I will fix it soon

2. your code shold call builtins.bool and not bool, as bool may refer to any value. See how it's done in

This transformation LogicOperateToBool works before ExpandBuiltins, I think bool(v) is equivalate to builtins.bool(v) in such case.

3. You're transforming any expression within the tested expression and in the orelse branch. I don't see any reason to do that in the test body / else branch, and even for condition, you should make sure the expression is only evaluated in boolean context, and not in, say, a function argument.

sorry! It's buggy, flag _if_stmt should be shift to False after visit(node.test). I will fix soon

design an optimisation pass that folds this calls to builtin.bool when it meets And and Or nodes, using the property:
bool(x and y) <> bool(x) and bool(y)

Do you mean convert explicit expression bool(x and y) in function body(outside if-orelse stmt) to bool(x) and bool(y) ?

def foo():
    flag = bool(x and y)  # => flag = bool(x) and bool(y)

Is it necessary to infer an Assign node with And or Or value is only used in if-orelse stmt, and converted it to boolean ?

def foo():
    flag = x and y        # flag should be converted to flag = bool(x) and bool(y) to avoid compile error
    if flag:
        do_something()
    return 0

[serge-sans-paille]

If I'm not mistaken your code will choke on something like

if a and len(b or c):

where a, b and c are strings.

I suggest to transform the above in

if builtins.bool(a and len(b or c)):

Then optimize it into

if builtins.bool(a) and builtins.bool(len(b or c))):

note: it's important to use an explicit path to builtins.bool to avoid situation like

bool = 1
if a and b:
   ...

Does that make more sense now?

[wcsjtu]

If I'm not mistaken your code will choke on something like

if a and len(b or c):

where a, b and c are strings.

I suggest to transform the above in

if builtins.bool(a and len(b or c)):

Then optimize it into

if builtins.bool(a) and builtins.bool(len(b or c))):

note: it's important to use an explicit path to builtins.bool to avoid situation like

bool = 1
if a and b:
   ...

Does that make more sense now?

I got it. Redefinition of bool is indeed rare case. This bug has been fixed. I will design two optimizers expand and combine bool() like this

# expand bool into logical stmt
bool(x and y)   # => bool(x) and bool(y)

# combine continuous bool call
bool(bool(bool(x)))  # => bool(x)

[wcsjtu]

I wrote ExpandBooleanOperator to expand expr like bool(x and y) into bool(x) and bool(y), and CombineBoolCall to combine continious bool() to single, just like bool(bool(bool(x))) => bool(x)。

special case like bool( x and ( y + (z or w) ) ) was also handled, expr like that will be converted into bool(x) and bool( y + (z or w) )

[serge-sans-paille]

I think these two (expand and combine) should be in the optimization pipeline, not in the transformation pipeline

[serge-sans-paille]

note: You should set self.update =True when you make any change to the AST.

[serge-sans-paille]

Use stateful transformation like you do makes the code harder to follow. I think you could (non recursively) detect if a call is a call to builtin.bool, then swicth on the argument: if it's a BoolOp node just distribute builtins.bool over the arguments.

But maybe I'm missing something?

[serge-sans-paille]

This should be a pattern in optimizations/pattern_transform.py

[serge-sans-paille]

@wcsjtu any news on thatone? I like the way this PR is going, happy to provide help if needed

[wcsjtu]

@wcsjtu any news on thatone? I like the way this PR is going, happy to provide help if needed

I' m learning your code these day. I found its hard to pass information between visit_AstNode without stateful impl. For example, pythran/analyses/imported_ids.py#57

def visit_AugAssign(self, node):
    self.in_augassign = True
    self.generic_visit(node)
    self.in_augassign = False

ExpandBooleanOperator will create new ast.Call, it is seemly conflict with current design, If it works in loop optimization stage.