Support class and function __doc__ #762
Conversation
vm/src/frame.rs
Outdated
| @@ -586,6 +588,7 @@ impl Frame { | |||
| let obj = vm.ctx.new_function(code_obj, scope, defaults); | |||
|
|
|||
| vm.ctx.set_attr(&obj, "__annotations__", annotations); | |||
| vm.ctx.set_attr(&obj, "__doc__", doc); | |||
There was a problem hiding this comment.
Instead of implementing setattr here, we could implement it at the compiler part, by using the instruction StoreAttr on the just created function. This will also prevent empty docstrings in for example lambda functions.
vm/src/compile.rs
Outdated
| @@ -1411,6 +1462,13 @@ impl Compiler { | |||
| // Fetch code for listcomp function: | |||
| let code = self.pop_code_object(); | |||
|
|
|||
| // function doc | |||
There was a problem hiding this comment.
This can be removed if you change the function and class compilation.
vm/src/compile.rs
Outdated
| // function doc | ||
| self.emit(Instruction::LoadConst { | ||
| value: bytecode::Constant::String { | ||
| value: "".to_string(), |
There was a problem hiding this comment.
This empty doc string can be removed if you implement StoreAttr at the functiondef compilation time.
vm/src/compile.rs
Outdated
| self.emit(Instruction::LoadConst { | ||
| value: bytecode::Constant::None, | ||
| }); | ||
| self.emit(Instruction::ReturnValue); | ||
|
|
||
| let code = self.pop_code_object(); | ||
|
|
||
| // function doc | ||
| self.emit(Instruction::LoadConst { |
There was a problem hiding this comment.
Remove this empty doc string.
vm/src/compile.rs
Outdated
| @@ -597,7 +597,20 @@ impl Compiler { | |||
| self.in_loop = false; | |||
| self.in_function_def = true; | |||
| let mut flags = self.enter_function(name, args)?; | |||
| self.compile_statements(body)?; | |||
|
|
|||
| let doc = get_doc(body); | |||
There was a problem hiding this comment.
This code is duplicated with the same code in the class definition part. Try to create a function which performs this split.
vm/src/compile.rs
Outdated
| @@ -644,6 +657,10 @@ impl Compiler { | |||
| }); | |||
| } | |||
|
|
|||
| self.emit(Instruction::LoadConst { | |||
There was a problem hiding this comment.
Modify the code here something like this:
if let Some(doc_string) = doc_str {
self.emit(LoadConst { doc_string });
self.emit(StoreAttr("__doc__"));
}
// else -> no code emitted!There was a problem hiding this comment.
Btw, micropython is a good source of inspiration for these kind of things: https://github.com/micropython/micropython/blob/master/py/compile.c#L3015
vm/src/builtins.rs
Outdated
| @@ -766,6 +766,7 @@ pub fn make_module(ctx: &PyContext) -> PyObjectRef { | |||
| pub fn builtin_build_class_(vm: &VirtualMachine, mut args: PyFuncArgs) -> PyResult { | |||
| let function = args.shift(); | |||
| let name_arg = args.shift(); | |||
| let doc = args.shift(); | |||
There was a problem hiding this comment.
See my other comments, this code can be removed if you solve the set attr in the compilation phase.
There was a problem hiding this comment.
@palaviv Nice job! This is a good improvement! I left some ideas for improvement in your code changes.
|
Thanks @windelbouwman. I changed the code as you suggested. |
Codecov Report
@@ Coverage Diff @@
## master #762 +/- ##
==========================================
+ Coverage 61.98% 62.04% +0.05%
==========================================
Files 80 80
Lines 12693 12722 +29
Branches 2609 2620 +11
==========================================
+ Hits 7868 7893 +25
+ Misses 3160 3159 -1
- Partials 1665 1670 +5
Continue to review full report at Codecov.
|
I tried doing this from the lexer but the doc declaration is a legal statement so I did not figured out a way to distinguish between the two.