Skip to content

New Arguments and Arg/ArgWithDefault AST representation #59

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 2 commits into from
May 31, 2023

Conversation

youknowone
Copy link
Member

Fix #57

@youknowone youknowone requested a review from MichaReiser May 18, 2023 18:04
@youknowone youknowone force-pushed the raw-function branch 5 times, most recently from a93c097 to 06d4285 Compare May 18, 2023 19:42
@youknowone
Copy link
Member Author

Running tests with full node makes more sense because its information is superset of not cfg

@youknowone youknowone force-pushed the raw-function branch 3 times, most recently from 21d6cb5 to db2f3ca Compare May 18, 2023 22:00
@youknowone youknowone marked this pull request as draft May 18, 2023 22:12
@youknowone youknowone marked this pull request as ready for review May 19, 2023 05:56
@youknowone youknowone force-pushed the raw-function branch 2 times, most recently from fe279f2 to 03a2949 Compare May 19, 2023 06:41
@youknowone
Copy link
Member Author

excluded range from FunctionArg not to make overwrapping ranges like #49

@youknowone
Copy link
Member Author

is hiding unused struct with a feature is better?

@youknowone youknowone force-pushed the raw-function branch 2 times, most recently from a9586f7 to 7af8dc4 Compare May 23, 2023 07:37
@youknowone youknowone marked this pull request as draft May 23, 2023 07:37
@youknowone youknowone force-pushed the raw-function branch 3 times, most recently from 8281890 to 35c1649 Compare May 24, 2023 14:39
@youknowone youknowone marked this pull request as ready for review May 24, 2023 14:39
@youknowone youknowone requested a review from MichaReiser May 24, 2023 14:39
@youknowone
Copy link
Member Author

@MichaReiser could you review it again?

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I'm still struggling to understand how this PR changes the AST and how it is intended to be used. Could you add a summary to your PR explaining what new nodes get introduced and removed, their structure, and how they are different from existing nodes.

Comment on lines 21 to 38
for (range, arg_name) in arguments
.posonlyargs
.iter()
.chain(arguments.args.iter())
.chain(arguments.kwonlyargs.iter())
.map(|arg| (arg.def.range, arg.def.arg.as_str()))
.chain(
arguments
.vararg
.as_ref()
.map(|arg| (arg.range, arg.arg.as_str())),
)
.chain(
arguments
.kwarg
.as_ref()
.map(|arg| (arg.range, arg.arg.as_str())),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This inline in arguments... is a lot to parse. Maybe assign the result to a temporary variable. This has the advantage that it gives the "thing" a name that guides readers.

UntypedParameter: ast::ArgWithDefault = {
<location:@L> <arg:Identifier> <end_location:@R> => {
let def = ast::Arg { arg, annotation: None, type_comment: None, range: (location..end_location).into() };
ast::ArgWithDefault { def, default: None, range: optional_range(location, end_location) }
Copy link
Contributor

Choose a reason for hiding this comment

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

My intention was that we would use ArgWithDefault in cases where the argument has a default. I think it's a bit odd that we now have ArgWithDefault that has no default.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. ArgWithDefault is only created for arg with potential default value, in common functions.

This type UntypedParameter is used for lambda expression, which disallow type hints. default is always None for lambda. If we want to give Arg for this types, we have to create Arguments variant like LambdaArguments.

Before this change, the default field of old Arguments was always empty Vec for lambda expression. I think the current change matches to it. (though we don't have to)

Copy link
Contributor

Choose a reason for hiding this comment

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

So do I understand this correctly that the ArgWithDefault is only a temporal data structure and we convert this into an Arg inside the lambda handling?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is always ArgWithDefault.

I had confusion for the upper comment. It is always not annotated, but it can have a default.

default is possibly filled up later when it exists.

A lambda with default value currently looks like

echo "lambda a=10: a" | python -m ast
Module(
   body=[
      Expr(
         value=Lambda(
            args=arguments(
               posonlyargs=[],
               args=[
                  arg(arg='a')],
               kwonlyargs=[],
               kw_defaults=[],
               defaults=[
                  Constant(value=10)]),
            body=Name(id='a', ctx=Load())))],
   type_ignores=[])

it will look like this with this patch

Module(
   body=[
      Expr(
         value=Lambda(
            args=arguments(
               posonlyargs=[],
               args=[
                  arg_with_default(arg='a', default=Constant(value=10))],
               kwonlyargs=[],
            body=Name(id='a', ctx=Load())))],
   type_ignores=[])

When it doesn't have a default, it is arg_with_default with default=None

Module(
   body=[
      Expr(
         value=Lambda(
            args=arguments(
               posonlyargs=[],
               args=[
                  arg_with_default(arg='a')],
               kwonlyargs=[],
            body=Name(id='a', ctx=Load())))],
   type_ignores=[])

args is a sequence type. it is not possible to give it a mix of arg and arg_with_default elements without introducing a new enum type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would then just go with Arg rather than naming it ArgWithDefault.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you distinguish it from arg without default? (Which is currently Arg)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I still don't understand how the final AST structure looks. What I understood from your last comment is that we use ArgWithDefault in all places where we used to use Arg. Is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's here https://github.com/RustPython/Parser/pull/59/files#diff-9f23fa4f43702fa8ce10a83956426c5dd647568d6d60cd59e3dff7f78b493981R3000-R3008

ArgWithDefault is used for args, posonlyargs and kwonlyargs. Arg is used for vararg and kwarg. Becasue this is illegal in Python.

def f(*args=[something]):
    pass

Copy link
Member Author

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

I will add a note about current structure as a crate doc

UntypedParameter: ast::ArgWithDefault = {
<location:@L> <arg:Identifier> <end_location:@R> => {
let def = ast::Arg { arg, annotation: None, type_comment: None, range: (location..end_location).into() };
ast::ArgWithDefault { def, default: None, range: optional_range(location, end_location) }
Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. ArgWithDefault is only created for arg with potential default value, in common functions.

This type UntypedParameter is used for lambda expression, which disallow type hints. default is always None for lambda. If we want to give Arg for this types, we have to create Arguments variant like LambdaArguments.

Before this change, the default field of old Arguments was always empty Vec for lambda expression. I think the current change matches to it. (though we don't have to)

@youknowone youknowone force-pushed the raw-function branch 2 times, most recently from 8cfe1a6 to ff4187a Compare May 27, 2023 16:20
Comment on lines 2991 to 2999
/// An alternative type of AST `arguments`. This is parser-friendly definition of function arguments.
/// `defaults` and `kw_defaults` fields are removed and the default values are placed under each `arg_with_default` typed argument.
/// `vararg` and `kwarg` are still typed as `arg` because they never can have a default value.
///
/// The matching Python style AST type is [PythonArguments]. While [PythonArguments] has ordered `kwonlyargs` fields by
/// default existence, [Arguments] has location-ordered kwonlyargs fields.
///
/// NOTE: This type is different from original Python AST.
Copy link
Member Author

Choose a reason for hiding this comment

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

@MichaReiser Is this descriptive enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also more human friendly (and implementing pre-order traversal becomes easier too)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! I added that too.

@youknowone youknowone force-pushed the raw-function branch 2 times, most recently from 4d1bdf1 to 6efec83 Compare May 27, 2023 17:18
@youknowone youknowone marked this pull request as draft May 31, 2023 11:27
@youknowone youknowone force-pushed the raw-function branch 2 times, most recently from 21e88f2 to 9ddc5e1 Compare May 31, 2023 15:51
@youknowone youknowone marked this pull request as ready for review May 31, 2023 16:03
@youknowone youknowone changed the title FunctionArguments and FunctionArg as raw AST representation New Arguments and Arg/ArgWithDefault AST representation May 31, 2023
@youknowone youknowone merged commit fdec727 into RustPython:main May 31, 2023
@youknowone youknowone deleted the raw-function branch May 31, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New layout for Arguments node to enhance performance. (and more so if possible)
2 participants