Skip to content

Add Generator Expr support #232

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

Merged
merged 3 commits into from
Apr 6, 2025
Merged

Add Generator Expr support #232

merged 3 commits into from
Apr 6, 2025

Conversation

Zehen-249
Copy link
Contributor

@Zehen-249 Zehen-249 commented Apr 1, 2025

Pull Request description

Add Generator Expression Supports

Solve #199

Pull Request checklists

Note:

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more
    complexity.
  • New and old tests passed locally.

Additional information

Generator Expr Object Image representation

image

String Representation

GeneratorExpr[element=BinaryOp[+](Variable[x],Variable[x]), target=Variable[x], iterable=Identifier, conditions=['BinaryOp[<](Variable[x],LiteralInt32(8))', 'BinaryOp[%](Variable[x],LiteralInt32(2))']

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved .

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Zehen-249
Copy link
Contributor Author

Zehen-249 commented Apr 1, 2025

Hey @xmnlab
I am encountering this failure during the pre-commit checks

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

libs\astx\src\astx\flows.py:742: error: Unsupported target for indexed assignment ("Union[list[DataTypesStruct], DictDataTypesStruct, Undefined]")  [index]
libs\astx\src\astx\flows.py:742: error: No overload variant of "__setitem__" of "list" matches argument types "str", "Union[list[DataTypesStruct], DictDataTypesStruct, Undefined]"  [call-overload]
libs\astx\src\astx\flows.py:742: note: Possible overload variants:
libs\astx\src\astx\flows.py:742: note:     def __setitem__(self, SupportsIndex, DataTypesStruct, /) -> None
libs\astx\src\astx\flows.py:742: note:     def __setitem__(self, slice[Any, Any, Any], Iterable[DataTypesStruct], /) -> None
Found 2 errors in 1 file (checked 65 source files)

The source is the data type of the conditions key i am using in the get_structs method

image

If I convert the data type to List the pre-commit tests will pass but the nodes for conditions get lost in the image representation.

I was hoping if you could provide some insight on this

@Zehen-249 Zehen-249 force-pushed the generator_exp branch 5 times, most recently from 3836911 to 7c3f8b8 Compare April 2, 2025 17:23
@Zehen-249
Copy link
Contributor Author

I figured it out, is there anything left in this PR before merge into main

],
)
generated_code = transpiler.visit(gen_expr)
expected_code = "(x for (x + x) in range(10) if (x > 3) if (x < 7))"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I didn't understand it .. but it seems it has an incorrect python syntax:

>>> (x for (x + x) in range(10) if (x > 3) if (x < 7))
  File "<stdin>", line 1
    (x for (x + x) in range(10) if (x > 3) if (x < 7))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: expected 'else' after 'if' expression

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using transpiler.visit, please use translate (e.g. https://github.com/arxlang/astx/pull/232/files#diff-213fa69c2a6fc65240961486159b29cc98cffaab2968b96afbae37a47abc4624R52)

it uses the translate.visit + a function to check if the syntax is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

)

generated_code = transpiler.visit(gen_expr)
expected_code = "(x for (x + x) in range(10))"
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not a valid python syntax:

>>> (x for (x + x) in range(10))
  File "<stdin>", line 1
    (x for (x + x) in range(10))
            ^^^^^
SyntaxError: cannot assign to expression

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using transpiler.visit, please use translate (e.g. https://github.com/arxlang/astx/pull/232/files#diff-213fa69c2a6fc65240961486159b29cc98cffaab2968b96afbae37a47abc4624R52)

it uses the translate.visit + a function to check if the syntax is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I fixed the bug.

@Zehen-249 Zehen-249 force-pushed the generator_exp branch 3 times, most recently from 230055c to 3ed3171 Compare April 3, 2025 11:45
@xmnlab
Copy link
Contributor

xmnlab commented Apr 3, 2025

ast-generator

@Zehen-249 this is the current graph representation in image format

@xmnlab
Copy link
Contributor

xmnlab commented Apr 3, 2025

as you can see in the image above, the "conditions" is not properly implemented.

instead of list of expr it should be a astnodes of ifexpr
I will share a similar code here:

https://github.com/arxlang/astx/blob/main/libs/astx/src/astx/classes.py#L39

https://github.com/arxlang/astx/blob/main/libs/astx/src/astx/classes.py#L52

https://github.com/arxlang/astx/blob/main/libs/astx/src/astx/classes.py#L74

so basically in the class you will set the type as ASTNodes .. in the init you should accept both ASTNodes and Iterable .. and if it is an iterable you need to convert it to astnodes

in this way, in the get_struct, the conditions can be easily prepared such as any other AST node.

let me know if you have any question

@xmnlab
Copy link
Contributor

xmnlab commented Apr 3, 2025

please, after you fix the issue share in a new comment the graph representation in image and ascii format .. so it is easier to give you feedback

@Zehen-249
Copy link
Contributor Author

https://github.com/arxlang/astx/blob/main/libs/astx/src/astx/classes.py#L52

https://github.com/arxlang/astx/blob/main/libs/astx/src/astx/classes.py#L74

so basically in the class you will set the type as ASTNodes .. in the init you should accept both ASTNodes and Iterable .. and if it is an iterable you need to convert it to astnodes

in this way, in the get_struct, the conditions can be easily prepared such as any other AST node.

let me know if you have any question

if I use ASTNodes[IfExpr] the the transpiler for IfExpr generates 'pass if (x > 2) else None' something like this (here block node is empty if I append Variable x in it then it becomes 'x if (x > 2) else None'
But this is not the syntax to use in generator expr
I must avoid the then block and else block only the condition is used in generators ( 'if (x > 2)' )

This can be avoid if conditions are just ASTNodes[Expr] type.
should do this ??

@Zehen-249
Copy link
Contributor Author

Zehen-249 commented Apr 4, 2025

Changed the type for conditions to ASTNodes[Expr]

Object

conditions = astx.ASTNodes[astx.Expr]()
conditions.append(
    astx.BinaryOp(
        op_code="<", lhs=astx.Variable("x"), rhs=astx.LiteralInt32(8)
    )
)
conditions.append(
    astx.BinaryOp(
        op_code="%", lhs=astx.Variable("x"), rhs=astx.LiteralInt32(2)
    )
)
gen_expr = astx.GeneratorExpr(
    element=astx.BinaryOp(
        op_code="+", lhs=astx.Variable("x"), rhs=astx.Variable("x")
    ),
    target=astx.Variable("x"),
    iterable=astx.Identifier("range(10)"),
    conditions=conditions,
)

ASCII Representation

image

Image Representation

image

transpiller output

((x + x) for x in range(10) if (x < 8) if (x % 2))

String Representation

GeneratorExpr[element=BinaryOp[+](Variable[x],Variable[x]), target=Variable[x], iterable=Identifier, conditions=['BinaryOp[<](Variable[x],LiteralInt32(8))', 'BinaryOp[%](Variable[x],LiteralInt32(2))']

Copy link
Contributor

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

LGTM!! thanks for working on that @Zehen-249 !

@xmnlab xmnlab merged commit c163c4c into arxlang:main Apr 6, 2025
19 checks passed
Copy link

🎉 This PR is included in version 0.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants