Exclude static functions from c:autodoc

[RastislavTuranyi]

Resolves #283

Adds :exclude-private-members: option to c:autodoc which, when added, causes the following docstrings to be excluded from the documentation:

• All static members (i.e. members which have static in their cursor.type/Docstring._ttype)
• All macros (though currently not macro functions)
• Simple typedefs (though not typedef struct ... combined declaration, but those seem to be handled the exact same as normal struct declarations in hawkmoth and so i haven't been able to find a way to distinguish them)

Do let me know if there is anything else that should be excluded as well!

I have attempted to implement this in as general a way as possible, one that would allow the framework to be used for different filtering in the future, but please let me know any comments you have!

Also, do you have any recommendations for how to do the tests for this?

[jnikula]

I think the DocCursor class should have a method for "is this public". I believe this is the correct abstraction layer for the information.

The parser.py parse() function may have to pass in "is this a header" information to DocCursor; I don't remember if that information can be determined from the Clang cursor directly.

The DocString class should copy this information in its __init__ method from the passed in cursor. This should make the filtering much nicer both in the extension and in DocString.walk().

Now, what is public? I think this should be based on the visibility another random .c file has to symbols in the file in question:

• In a header, everything is public. You include the header, you see everything.

• In a non-header, non-static functions and non-static variables are public, i.e. most things in a .c file are private.

I think the only problematic case is opaque types. Say, you have a struct foo defined in a .c file, and its corresponding header uses struct foo *. The users don't have visibility on the type, but would be nice to have some documentation on it. The documentation for struct foo in .c might contain member documentation and generally implementation details that should not be visible outside. I'm not sure if you can document forward declarations. Anyway, the workaround would be to have typedef struct foo foo_t in the header, and document that.

@BrunoMSantos thoughts?

[jnikula]

Also, do you have any recommendations for how to do the tests for this?

Add test cases where your .c and .h files have all sorts of symbols and visibilities, add your new option in the .yaml (grep for options:) and ensure the result .rst only has the expected symbols.

[jnikula]

If there's a way to exclude private members, I think there should also be a way to only include private members, so you can split the documentation into public and private (implementation details). I think that's a valid use case.

There's an option to only include private members in autodoc, it's :private-members: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-option-automodule-private-members

I think we should mimic that. And I guess perhaps :public-members: would then be a better companion than :exclude-private-members:?

[jnikula]

(Oops, sorry for the close/reopen noise, hit the wrong button. 🤦)

[RastislavTuranyi]

I think the DocCursor class should have a method for "is this public". I believe this is the correct abstraction layer for the information.

Sure! Would you like to define the behaviour for the C++ exclusive subclasses (e.g. ClassDocString) in this PR as well?

Now, what is public? I think this should be based on the visibility another random .c file has to symbols in the file in question:

• In a header, everything is public. You include the header, you see everything.
• In a non-header, non-static functions and non-static variables are public, i.e. most things in a .c file are private.

Are you certain this should be enforced at the library level instead of leaving it to the user? This would mean that the option(s) would have no effect at all with a header file, at which point it might be easier to turn the option off in the directive rather than passing the .c/.h information around.

If there's a way to exclude private members, I think there should also be a way to only include private members, so you can split the documentation into public and private (implementation details). I think that's a valid use case.
There's an option to only include private members in autodoc, it's :private-members:

That could indeed be useful! I am a little bit concerned, though, about potential confusion since the behaviour is different between sphinx and hawkmoth - sphinx autodoc :private-members: includes public and private members, while hawkmoth autodoc :private-members: would include only private members

[RastislavTuranyi]

Also, would you like to continue using the filter_func: Callable general approach, or replace it with something more specific, e.g. filter_private: bool?

[BrunoMSantos]

I think the DocCursor class should have a method for "is this public". I believe this is the correct abstraction layer for the information.

I think we'd want a "is this declared / defined in a header" and a "is this static / in an empty namespace" for this purpose instead.

The parser.py parse() function may have to pass in "is this a header" information to DocCursor; I don't remember if that information can be determined from the Clang cursor directly.

Think so too. I don't think that the cursor could know actually: any file can be #includeed, extension notwithstanding. The only requirement is that it expands to valid code in-situ.

Now, what is public? [...]

Only what is declared public in C++ I would hope. Same for protected :P

I think the only problematic case is [...]

I think this is confusing the separation of API and non-API with other concepts that are simply not a thing in C and really are a thing in C++, but a different thing altogether (with keywords and everything). Comparisons to Python's autodoc are unfounded in this case, I think.

Now, many projects have different levels and definitions of API access. The kernel guys have EXPORT_SYMBOL and EXPORT_SYMBOL_GPL and they also have module API and user level APIs. Most libraries will also have __attribute__((visibility("hidden/default"))) macros. Etc. If we can't tell a user to split API documentation into its own file generally, how could we say it to the authors of such a library?

But, considering we changed what we call this not to clash with C++'s private / protected concepts (probably 'excluded members' like in autodoc), I think there's a good alternative:

---------------8<----------------

We want to (conditionally or not) skip certain symbols, but we don't want to be naming them all the time with :exclude-members: and I wouldn't want to add nor maintain any heuristic because these seem very volatile to me. Even a new C++ standard (there's so many...) could derail our heuristics.

Also pause to consider that Doxygen uses a @private tag, they have no such heuristics that I'm aware of. And Doxygen hasn't been shy about adding features and complexity.

Now, I don't think a :exclude: or API or whatever tag would scale even if we thought it was a good idea to have such tags in general, and I don't think we do (yet). But Sphinx has events!

In fact looking at autodoc we have an example of using an event to skip members already. We could have a similar thing and let the heuristics to the user, which I'd be a lot more comfortable with.

We would pass along the cursor with those "am I static / in an empty namespace" and "am I in a header" methods among everything else and the user could easily achieve the same result proposed here without us maintaining the heuristics.

[jnikula]

I think the DocCursor class should have a method for "is this public". I believe this is the correct abstraction layer for the information.

I think we'd want a "is this declared / defined in a header" and a "is this static / in an empty namespace" for this purpose instead.

Fair enough.

The parser.py parse() function may have to pass in "is this a header" information to DocCursor; I don't remember if that information can be determined from the Clang cursor directly.

Think so too. I don't think that the cursor could know actually: any file can be #includeed, extension notwithstanding. The only requirement is that it expands to valid code in-situ.

Now, what is public? [...]

Only what is declared public in C++ I would hope. Same for protected :P

I used the term loosely, and for module level symbols, not members. autodoc not autostruct or autoclass.

For functions and variables, extern and static would be what I was after. But if you have your basic module with .c and .h, any macros and types in .c aren't visible to the "users" of that module, but anything in .h are.

And static inline in a header is also visible.

I think the only problematic case is [...]

I think this is confusing the separation of API and non-API with other concepts that are simply not a thing in C and really are a thing in C++, but a different thing altogether (with keywords and everything). Comparisons to Python's autodoc are unfounded in this case, I think.

Now, many projects have different levels and definitions of API access. The kernel guys have EXPORT_SYMBOL and EXPORT_SYMBOL_GPL and they also have module API and user level APIs. Most libraries will also have __attribute__((visibility("hidden/default"))) macros. Etc.

Yeah, sigh, visibility in C alone can be complicated...

If we can't tell a user to split API documentation into its own file generally, how could we say it to the authors of such a library?

I am actually symphatetic to both approaches. On the one hand, it's clean to to put API documentation in the header, next to function declarations. But on the other hand, I like having the documentation above the function definition.

But, considering we changed what we call this not to clash with C++'s private / protected concepts (probably 'excluded members' like in autodoc), I think there's a good alternative:

---------------8<----------------

We want to (conditionally or not) skip certain symbols, but we don't want to be naming them all the time with :exclude-members: and I wouldn't want to add nor maintain any heuristic because these seem very volatile to me. Even a new C++ standard (there's so many...) could derail our heuristics.

Also pause to consider that Doxygen uses a @private tag, they have no such heuristics that I'm aware of. And Doxygen hasn't been shy about adding features and complexity.

Now, I don't think a :exclude: or API or whatever tag would scale even if we thought it was a good idea to have such tags in general, and I don't think we do (yet). But Sphinx has events!

In fact looking at autodoc we have an example of using an event to skip members already. We could have a similar thing and let the heuristics to the user, which I'd be a lot more comfortable with.

We would pass along the cursor with those "am I static / in an empty namespace" and "am I in a header" methods among everything else and the user could easily achieve the same result proposed here without us maintaining the heuristics.

So what would the directive look like with that? To achieve the same end result, I think we'd have to add an option to autodoc to pass on to the event, so that the use can control it. It's the options parameter in autodoc-skip-member, but those alone aren't enough to implement the extern/static distinction.

And would we anyway have to add that functionality in a built-in extension, and maintain it, because it has to be easy for the users too!

[BrunoMSantos]

Now, what is public? [...]

Only what is declared public in C++ I would hope. Same for protected :P

I used the term loosely, and for module level symbols, not members. autodoc not autostruct or autoclass.

That's the thing: in Python there's no such difference, a _symbol will be private to a module or to a class all the same and what I gathered so far (didn't see the code here yet I'm afraid) would try to draw parallels in C that are not ok I think.

For functions and variables, extern and static would be what I was after.

Hmm, we could have a "is this extern" (or even a "is this declared with extern" since some like putting those in function declarations 🤷) as well.

So what would the directive look like with that? To achieve the same end result, I think we'd have to add an option to autodoc to pass on to the event, so that the use can control it. It's the options parameter in autodoc-skip-member, but those alone aren't enough to implement the extern/static distinction.

You mean if a user wanted to overrule the skip decision from the event? I'd expect :members: and :exclude-members: to be able to exempt or extend the list 'post event'. Otherwise the event would apply globally.

And would we anyway have to add that functionality in a built-in extension, and maintain it, because it has to be easy for the users too!

Not sure we need to. We could document a trivial example that achieves exactly what you've proposed:

def skip_member(..., cursor, ...):
    return not cursor.in_header and cursor.has_static_linkage
 
def setup(app):
    app.connect('hawkmoth-skip-member', skip_member)

Actually, I'm just realizing that I'm using 'member' for file level symbols. In a way I think it's ok to interpret it contextually depending on the directive, but for the event we'd probably need to distinguish them as both could make sense separately. Maybe a hawkmoth-skip-symbol. And then maybe we should do :symbols: and :exclude-symbols:...

I haven't thought it through quite that far.

Edit: fixed inverted logic in snippet. Hope I got it right this time 🙃

[jnikula]

[I'm sorry, I'm cherry-picking what I reply to.]

And would we anyway have to add that functionality in a built-in extension, and maintain it, because it has to be easy for the users too!

Not sure we need to. We could document a trivial example that achieves exactly what you've proposed:

def skip_member(..., cursor, ...):
    return not cursor.in_header and cursor.has_static_linkage
 
def setup(app):
    app.connect('hawkmoth-skip-member', skip_member)

My point is, if a user wants to document external interfaces (extern) and internal details (static) separately, what do they put in the .. c:autodoc: directive options? The event needs some info specific to the directive for that.

[BrunoMSantos]

My point is, if a user wants to document external interfaces (extern) and internal details (static) separately, what do they put in the .. c:autodoc: directive options? The event needs some info specific to the directive for that.

Maybe I misunderstood the question. You mean an something like this?

.. autodoc:: file.c

.. autodoc:: file.c
   :static:

Not what I thought this was about, but I'm fine with that, as long as it's rooted on actual C/C++ features. I.e. no cross checking with file extensions, which is just a poor heuristic.

I thought we meant something akin to Python's autodoc that would 'know' what is API or not and show what would otherwise be hidden by default with a :private-members:. In my opinion, nothing like that should happen in C at least (C++ does have private and protected stuff we could hide by default). But we could support a way to hide things by default with an event according to a project's own rules.

[BrunoMSantos]

Just to clarify, the event filter I'm proposing would be just a dumb filter. By default it filters nothing and you'd be able to do :exclude-symbols: foo or you could add a filter for foo and then override it with :symbols: foo. No magic at all.

[jnikula]

My point is, if a user wants to document external interfaces (extern) and internal details (static) separately, what do they put in the .. c:autodoc: directive options? The event needs some info specific to the directive for that.

Maybe I misunderstood the question. You mean an something like this?

.. autodoc:: file.c

.. autodoc:: file.c
   :static:

Yes, I don't think this is that different from the hawkmoth-process-docstring event. You don't want the skip member event to treat every directive the same. Or if you do, implementing the event handler gets much more complicated than your simple example.

Not what I thought this was about, but I'm fine with that, as long as it's rooted on actual C/C++ features. I.e. no cross checking with file extensions, which is just a poor heuristic.

Just like transform this could be up to the event to decide? I know it's deviating from Sphinx autodoc, but like you said, Python is not C/C++, and the same rules don't apply.

I thought we meant something akin to Python's autodoc that would 'know' what is API or not and show what would otherwise be hidden by default with a :private-members:. In my opinion, nothing like that should happen in C at least (C++ does have private and protected stuff we could hide by default). But we could support a way to hide things by default with an event according to a project's own rules.

I think we're maybe conflating several more or less brainstormed ideas here, and that's why we're getting confused?

There are various things we could do, some orthogonal, some overlapping:

1. In general, improve support for Sphinx autodoc member (including module level) filtering, where applicable for C and C++.
2. Add member (including module level) filtering that is tweaked for C and C++. This is basically this pull request (but for autodoc i.e. module level only), and potentially with the heuristics I've proposed. I think we disagree on how generally useful that heuristic is.
3. Add an event for member (including module level) filtering, mimicing that of Sphinx autodoc. Figure out what information should be passed to the event.

I think I was leaning towards 2) and you were leaning towards 3). I think 2) is easier for the average user, and I believe the heuristic I proposed cover most cases. However 3) is slightly harder for the average user, is obviously more flexible, and can be used to achieve the same as 2).

Is that an accurate summary?

In any case, I don't think the options rule each other out. The question is what to focus on first.

[jnikula]

Side note, I think starting to pass DocCursor to events requires documenting it more comprehensively, and adding its Python autodoc to Hawkmoth documentation. It's a failure to require users to look at Hawkmoth source to implement the event handlers.

[BrunoMSantos]

Yes, I don't think this is that different from the hawkmoth-process-docstring event. You don't want the skip member event to treat every directive the same. Or if you do, implementing the event handler gets much more complicated than your simple example.

Yeah, agreed. Something like a hawkmoth-skip-symbol event only makes sense at file level for instance. Not sure if I missed something here though.

Just like transform this could be up to the event to decide?

Sorry, I didn't understand this one. :static: is a well defined thing in C/C++, I don't think it would involve any heuristic or require any event to modulate its behaviour.

[...]
Is that an accurate summary?

Hmm, kind of? I think it's still mixing concepts a bit and I'm still uneasy about the way it's phrased. Or maybe I'm taking certain nomenclature too literally in your comments? Or maybe I haven't phrased the crux of my disagreement well enough yet?

Trying to digest my ideas above, this is how I see the concepts split in my ideal world. And indeed, all of these could be implemented separately, if not in multiple steps, and only point 3 relates to the feature actually under discussion.

1. Auto hide symbols with static linkage and expose them with :static:. This is well defined and it may be useful I guess.
2. Be able to auto hide and extend members (think structs, classes, enums, ...) in C++ with :private-members:, :protected-members:. This is well defined. [1][2]
3. Provide :exclude-symbols: :private-symbols: together with a hawkmoth-skip-symbol hawkmoth-private-symbol event (or maybe :internal: or something else). This is not well defined! By default no symbol is private in my ideal world, but as long as it can be overridden... I guess I can be persuaded [3]. The :private-symbols: option would then expose the symbols hidden by the event callback.

I believe this generalizes at least better than what I understood of the original feature request and the discussion above. And it provides an answer to the desired feature, albeit in a different way.

[1] In Python you can do module.member, in C/C++ you can't do file.symbol. I can't see a sensible way of mixing these without it hurting us eventually.

[2] Side note: C++ has namespaces and modules, which do and might have members respectively (never looked into modules). In namespaces, members are not any more private / public than any global symbol in C/C++ though, so nothing changes there. In modules, there seems to be a concept of 'exported' stuff which might map well to the concept of 'private members'.

[3] Ultimately I think most serious projects will be more interested in looking for an EXPORT macro, so we'd be breaking people's documentations for very questionable gains... And we can easily volunteer that one liner heuristic in the documentation or as a default-off official extension.

[RastislavTuranyi]

So, what would you like to do? I'm not fully able to follow the discussion, but the thing I was originally after in #283 (and still am at the moment) is exactly:

1. Auto hide symbols with static linkage and expose them with :static:. This is well defined and it may be useful I guess.

I wouldn't mind doing only that much in this PR and leaving more advanced stuff for a separate one, if that is desirable?

That said, learning about the EXPORT macro mentioned, being able to use it with hawkmoth seems perfect for the future! Maybe we could implement an exclusion by a ttype kind of thing?

.. autodoc:: main.c
   :exclude-symbols: static struct typedef

which would exclude any symbol with any of the specified strings (i.e. above would exclude all static, all structs and all typedefs):

def _match(...):
    ...
    for exclude_symbol in exclude_symbols:
        if exclude_symbol in self._ttype:
            return False
    ...

This could potentially be combined with a (potentially mutually non-exclusive) include-symbols option. There are some problems with it such as macros not having anything in the _ttype attribute and EXPORT and similar not showing up, but those could potentially handle by either expanding _ttype or adding a new attribute with the info. What do you think?

[BrunoMSantos]

Hey, sorry for the delay.

So, what would you like to do? I'm not fully able to follow the discussion, but the thing I was originally after in #283 (and still am at the moment) is exactly:

1. Auto hide symbols with static linkage and expose them with :static:. This is well defined and it may be useful I guess.

I wouldn't mind doing only that much in this PR and leaving more advanced stuff for a separate one, if that is desirable?

Yeah, I guess that does allow you to basically hide the stuff selectively, albeit lacking the 'intelligence' to hide things contextually like we were discussing.

Assuming @jnikula agrees, I think it's ok to implement that alone for now feature wise, but this implies breaking stuff. Existing projects might suddenly see symbols disappear from the documentation.

I have to defer on that one.

That said, learning about the EXPORT macro mentioned, being able to use it with hawkmoth seems perfect for the future! Maybe we could implement an exclusion by a ttype kind of thing?

.. autodoc:: main.c
   :exclude-symbols: static struct typedef

which would exclude any symbol with any of the specified strings (i.e. above would exclude all static, all structs and all typedefs):

Hmm, that sort of thing would probably be a no go I think: it would not scale well and someone else would invariably request yet another filtering option.

An event however could allow the user to do that sort of thing globally, not per directive. For 'per directive' filtering, I'd follow Python's autodoc: force the user to explicitly name the symbols.

[jnikula]

Hey, I've been away, vacations and all that. Is this waiting for my input?

Assuming @jnikula agrees, I think it's ok to implement that alone for now feature wise, but this implies breaking stuff. Existing projects might suddenly see symbols disappear from the documentation.

That's not great. At this stage of the project, I'm kind of okay with making changes that break things loudly. However this one would silently drop stuff from documentation, and might take a while for anyone to notice.

[RastislavTuranyi]

We could also do the converse and keep the default behaviour (which includes everything) and have :exclude-static: which excludes symbols with static linkage, right?

In that case, the bigger question might be whether to go ahead with this at all - if I remember the discussion correctly, there were a couple proposals (and the default behaviour thing applies to each):

1. :static:/:exclude-static: - only deals with the static linkage
2. :private-members: and/or(?) :protected-members: for C++ only (or their exclusion) - uses C++ keywords for determination
3. :exclude-private-members: originally implemented in this PR - uses custom algorithm to decide what to include/exclude
4. :private-symbols: option with hawkmoth-private-symbol event - leaves it to the user to implement custom inclusion/exclusion algorithms

Which (combination?) of these would of interest to implement?

[BrunoMSantos]

That's not great. At this stage of the project, I'm kind of okay with making changes that break things loudly. However this one would silently drop stuff from documentation, and might take a while for anyone to notice.

Indeed. Best I imagine (assuming we prefer the default off semantics):

1. Add the :static: option and set it by default.
2. Add a global option like future['static_filter_option'] = True or something for those ready to adopt it early.
3. Document that this option will default off in a sufficiently distant version X.
4. Log an INFO level(?) message in the interim for those currently not using the option explicitly.

[jnikula]

I understand the original issue, but apart from adding an event, I don't yet see a solution that would properly address the issue and be acceptable to everyone.

One particular point:

:static: is a well defined thing in C/C++, I don't think it would involve any heuristic or require any event to modulate its behaviour.

static is only well defined for things you can use it with. It doesn't make sense for macros or types.

---

I believe these are the relevant use cases for the problem at hand:

1. Extract documentation for functions and objects with external linkage (non-static) from a .c file. This means "externally visible" or "public" identifiers (not to be interpreted as keywords).
2. Extract documentation for everything else but not functions or objects with external linkage from a .c file. This includes identifiers with internal linkage (static) and macros and types. This means "internal" or "private" identifiers and macros and types (not to be interpreted as keywords).
3. Extract all documentation from a file. This is what we currently have, and it's also the only mode that makes sense for a .h file. Header files get included, and everything in them will be "externally visible" or "public" (not to be interpreted as keywords), including macros and types and static inline functions.

I originally suggested the .h/.c distinction be built in Hawkmoth. This doesn't have to be the case. The above description could be done without mentioning .h or .c, but you'd typically only use 1 and 2 for .c and 3 for both .c and .h.

Note how 1 and 2 do not fit static vs. non-static, but also not internal vs. external linkage. It's non-static vs. everything else, or external linkage vs. everything else. Specifically, the intersection of documentation with modes 1 and 2 is the empty set, and the union of documentation with modes 1 and 2 equals 3. Therefore, I don't think the naming should have static or linkage.

Arguably top-level or generic documentation comments would fall into 1, since we have no way to make the distinction otherwise.

But before jumping into bikeshedding what to name the option, do you agree the three cases above would cover the problem?

[BrunoMSantos]

One particular point:

:static: is a well defined thing in C/C++, I don't think it would involve any heuristic or require any event to modulate its behaviour.

static is only well defined for things you can use it with. It doesn't make sense for macros or types.

Yup, I could see a :static: option on the autodoc directive, just like I could see a :macro: or :function:, etc. In the limit, autodoc would only return the top level comments by default. Maybe missing something?

But yeah, ultimately, :static: in particular is only a poor man's version of :give-me-the-"""public"""-or-"""private"""-bits: and I think these would rarely be useful, or only in trivial contexts like the OP's scenario (hopefully not misrepresenting). But I think it could be made quite maintainable (and I expect that these features would result in a clean-up that would be well worth it... I never got around to that cursor refactor 😅).

[...]
But before jumping into bikeshedding what to name the option, do you agree the three cases above would cover the problem?

In the sense that a user could then implement an event handler with that logic, yes.

E.g. I would just as likely put the macros and types together with the extern stuff. And this is well before considering some of the other counter examples mentioned earlier that would render any such option a distraction to be worked around.

edit: sorry, I was half asleep... I can think of cases where macros and types in a .c are 'public', but they sure aren't 'as likely'. So yeah, those rules do work for simple cases.

[jnikula]

Anyway.

I'm looking at the code, and I believe the biggest blocker to implementing any of the features like this cleanly is that the filtering is split between docstring.py and __init__.py. I'm toying with making Docstring iterable with some helper features, and moving the filtering to the calling side. I think this will clean things up.