-
Notifications
You must be signed in to change notification settings - Fork 1.3k
ast module #1318
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
ast module #1318
Conversation
Lib/ast.py
Outdated
|
||
|
||
# XXX: Rustpython _ast includes parse() | ||
#def parse(source, filename='<unknown>', mode='exec'): |
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.
Our ast.parse
doesn't include a mode argument, could you maybe modify builtin_compile
to accept PyCF_ONLY_AST
? You could match on the mode and use rustpython_parser::parser::parse_program
for exec and single or rustpython_parser::parser::parse_expression
for eval. Maybe also enable builtin_compile
even when the feature is off but just throw a runtime error if the caller doesn't pass PyCF_ONLY_AST
so that ast.parse
still works.
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 would like to add the mode optional argument to the rust parse function. Note that the mode can be retrieve because mode implements FromStr
now. Please refer to the builtin compile function how this is done.
9aa51b5
to
d521d54
Compare
parser/src/parser.rs
Outdated
|
||
// compile mode but defined here | ||
#[derive(Clone, Copy)] | ||
pub enum Mode { |
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 not too sure that this Mode
enum should be contained in the parser crate. I feel it has its place in the compiler crate (where it was). The new parse
function can be place in near the compile function in the builtins.rs in the vm crate.
Rationale: the mode is really a compiler thing. The parser should not be aware of the concept of mode, only parse code.
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 seems Mode
should be defined out of compile.rs
because ast.parse
works without #[cfg(feature = "rustpython-compiler")]
. Any idea?
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.
@windelbouwman any advice for proper place about this?
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.
ast.parse should not work without the compiler crate, since when you have the compiler crate, the parser crate is implied. Vice versa is not true, but I think it makes sense to excluse ast.parse
when the compiler feature is disabled. So I think we should place Mode
in mode.rs
in the rustpython-compiler
crate.
@@ -73,6 +73,50 @@ pub fn parse_expression(source: &str) -> Result<ast::Expression, ParseError> { | |||
do_lalr_parsing!(source, Expression, StartExpression) | |||
} | |||
|
|||
pub fn parse(source: &str, mode: Mode) -> Result<ast::Top, ParseError> { |
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 suggest to move this function to the builtins.rs
in the vm crate.
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.
Or maybe util.rs
?
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.
Yes, agreed, but in any case, this function should reside in the vm crate, since it uses a compilation mode.
db09eb7
to
f1691a7
Compare
f1691a7
to
2f1fb16
Compare
Add build compatibility with rustc 1.36.0 since #1318 merged
No description provided.