-
Notifications
You must be signed in to change notification settings - Fork 35
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
Conversation
a93c097
to
06d4285
Compare
Running tests with full node makes more sense because its information is superset of not cfg |
c736d5b
to
72d53a0
Compare
21d6cb5
to
db2f3ca
Compare
fe279f2
to
03a2949
Compare
excluded range from FunctionArg not to make overwrapping ranges like #49 |
is hiding unused struct with a feature is better? |
a9586f7
to
7af8dc4
Compare
8281890
to
35c1649
Compare
@MichaReiser could you review it again? |
There was a problem hiding this 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.
parser/src/function.rs
Outdated
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())), | ||
) |
There was a problem hiding this comment.
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) } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this 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) } |
There was a problem hiding this comment.
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)
8cfe1a6
to
ff4187a
Compare
ast/src/gen/generic.rs
Outdated
/// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
4d1bdf1
to
6efec83
Compare
21e88f2
to
9ddc5e1
Compare
Fix #57