Skip to content

Conversation

@dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented Mar 6, 2024

This PR adds support for function doc strings. It's disabled by default.

def f():
    "doc string"
    pass

print(f.__doc__)

TODO:

  • maybe have a separate compile-time option for this
  • check that the native .mpy ABI is unchanged

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Mar 6, 2024
@dpgeorge dpgeorge marked this pull request as draft March 6, 2024 13:54
@codecov
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.36%. Comparing base (90e5178) to head (0a3f167).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #14045   +/-   ##
=======================================
  Coverage   98.36%   98.36%           
=======================================
  Files         161      161           
  Lines       21084    21084           
=======================================
  Hits        20739    20739           
  Misses        345      345           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Mar 6, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

#if MICROPY_ENABLE_DOC_STRING
// look for the first statement
mp_parse_node_t pn = pns->nodes[3]; // the function body/suite
if (MP_PARSE_NODE_IS_STRUCT_KIND(pn, PN_suite_block_stmts)) {
Copy link
Member

Choose a reason for hiding this comment

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

This check already seems to be done by check_for_doc_string?

i.e. this whole thing could be a call to check_for_doc_string, the only difference is how it emits the store of __doc__ ?

// compile the function definition
compile_funcdef_lambdef(comp, fscope, pns->nodes[1], PN_typedargslist);

#if MICROPY_ENABLE_DOC_STRING
Copy link
Member

Choose a reason for hiding this comment

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

Can we update the comment in mpconfig.h to explain that this enables docstrings on functions, modules, and classes?

(I think we should keep them as one combined option)

@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 7, 2024

I really want to find a way to make this use less RAM for functions that don't have doc strings. Because at the moment all functions increase by 1 GC block size, regardless of whether they use the new doc string slot.

But I don't know how to do that in a simple way. It probably requires a separate function type like mp_type_fun_bc_with_doc_string and the ability to upgrade a normal function to this type when the doc string entry is stored to (and eg repurpose the const byte *bytecode entry in mp_obj_fun_bc_t to point to a separate struct that points to the doc string and the bytecode).

If we can find a way to do that then we could enable this feature on the large ports, because it would only cost something if docs strings are used.

Also could extend things to allow any attribute to be stored to a function object, not just the __doc__ entry. Doc strings would then just be one user of this feature (to store attrs to functions).

@Josverl
Copy link
Contributor

Josverl commented Jun 6, 2024

Out of interest wrt to the Micropython-stubs project I'd like to understand what the goal is for this feature

Is it targeting stdlib/micropython code, frozen (. Mpy) code, or regular micropython.py modules?

I assume the interface would be the cpython style __doc__ property, is that correct?

Is there something similar considered for modules or classes.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

py-core Relates to py/ directory in source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants