Skip to content
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

Python private fields incorrectly require documentation in version 1.9.6+ #10231

Closed
lelegard opened this issue Aug 14, 2023 · 10 comments
Closed
Labels
Python Usage The mentioned problem is not a doxygen problem but due to usage of a feature.

Comments

@lelegard
Copy link

Starting with doxygen 1.9.6, private fields in Python classes incorrectly require documentation, even when the Doxyfile excludes private fields. The problem is also present in 1.9.7 and current repo for 1.9.8.

Sample file test.py:

## A class
class C
    ## Constructor.
    def __init__(self):
        super().__init__()
        ## A public field
        self.a = None
        # A private field, not documented
        self._b = None

Output:

$ ../doxygen-1.9.4/build/bin/doxygen
$ ../doxygen-1.9.5/build/bin/doxygen
$ ../doxygen-1.9.6/build/bin/doxygen
test.py:9: warning: Member _b (variable) of class test::C is not documented.
$ ../doxygen-1.9.7/build/bin/doxygen
test.py:9: warning: Member _b (variable) of class test::C is not documented.
$ ../doxygen-master/build/bin/doxygen
test.py:9: warning: Member _b (variable) of class test::C is not documented.
$ ../doxygen-master/build/bin/doxygen  --version
1.9.8 (a996a63a5a7527b29117737db60019ade81d7fa7)

The set of files to reproduce the issue, including Doxyfile, is attached in doxybug2.zip.

@albert-github albert-github added Python Usage The mentioned problem is not a doxygen problem but due to usage of a feature. labels Aug 14, 2023
@albert-github
Copy link
Collaborator

Between the version 1.9.5 and 1.9.6 the convention of determining what is a private member in python has been changed.
Till and including 1.9.5 a member starting with an underscore was seen as a private member
As of version 1.9.6 members starting with

  • with 2 or more underscores are seen as private
  • with one underscore are seen as protected

see also #9706.
This change explains the warning.

I think the problem is usage.

@lelegard
Copy link
Author

Thanks for explanation @albert-github

Is this a doxygen-specific convention?

I am not a Python expert (my project is a C++ one with Python bindings) but I have read many times in the Python literature that, even though the concept of private field does not exist in Python, the convention was to prepend one underscore (not two) to fields which are not supposed to be used by the application and, hence, could be considered as private. The two underscores conventions, as in __init__, was dedicated to predefined names.

Doesn't this new doxygen policy break a lot of Python documentation?

But I agree that the lack of proper specification in the Python language is a source of problems.

I will modify my code with two underscores in "private" fields names. I just hope that some Python bigot won't make a case out of it 😢 BTW, this is not the first time I have to modify the code of my Python bindings to avoid doxygen issues: bug report #8416 was open two years ago and not yet fixed.

Thanks anyway.

You may close this issue if you consider this two underscores convention as final in doxygen.

@lelegard
Copy link
Author

lelegard commented Aug 14, 2023

Hi @albert-github

After running the test suite of my project, replacing the initial underscore of all "private" fields by two actually broke the code.

The reason is that a leading double underscore in Python implicitly activates name mangling. Thus, when a private field is used in a friend manner (as in C++), the name is broken, the field is not found.

So, it sounds that this convention is not a good idea. I now understand why the Python literature makes a difference between one and two underscores. The people who recommended this for doxygen in Python may have thought in terms of protected and private but they forgot friend.

Since this new (1.9.6) doxygen convention breaks actual code, I ask you to consider it as a bug.

As the very minimum, I would request a new option in Doxyfile to specify the prefix for Python private fields. I recommend to use one single underscore as default to avoid breaking existing code. If you absolutely want to keep the double underscore as default, then we must have an option to override it, typically to one underscore.

Breaking existing code for the sake of documentation is really not a good idea.

@lelegard
Copy link
Author

lelegard commented Aug 14, 2023

@albert-github, more information to justify that this is a bug.

Let's assume that we accept that the double underscore is for "private" field. Then, the interpretation of doxygen is still incomplete because it flags explicit use (demangled name) of the private field as non-private.

Consider test.py:

## A class
class C1:
    ## Constructor.
    def __init__(self):
        super().__init__()
        ## A public field
        self.a = None
        # A private field, not documented
        self.__b = 'C1'
    ## Test.
    def test(self):
        return self.__b


## A subclass
class C2(C1):
    ## Constructor.
    def __init__(self):
        super().__init__()
        self._C1__b = 'C2' # required demangled syntax to access the field

Then:

$ ../doxygen-master/build/bin/doxygen
test.py:20: warning: Member _C1__b (variable) of class test::C2 is not documented.

When the field is created, in class C1, doxygen does not complain because it considers it as private. But when we access the field using its fully qualified (demangled) name, doxygen no longer consider it as private. This is inconsistent.

I urge you to either revert to the original behavior (one underscore means private) or add an option in Doxyfile to revert it.

@albert-github
Copy link
Collaborator

albert-github commented Aug 17, 2023

Looks a bit like there has been an attempt to make it more flexible, see #6555

@lelegard
Copy link
Author

Looks a bit like there has been an attempt to make it more flexible, see #6555

Yes, this sounds good. It's flexible and the default is to consider anything starting with one underscore as private.

But it was submitted 5 years ago and nobody merged it, unlike the annoying change which is present in version 1.9.6 which considers everything starting with one underscore as protected.

So, we have to consider that doxygen maintainers favored the latter at the expense of the former. This is a very bad decision. This is why I firmly ask to revert that decision and adopt the flexible solution from #6555.

@albert-github
Copy link
Collaborator

So, we have to consider that doxygen maintainers favored the latter at the expense of the former. This is a very bad decision. This is why I firmly ask to revert that decision and adopt the flexible solution from #6555.

That is your opinion. The real problem is the fact that there is in python no real notion of private / protected etc. and that there is no real standard for python (when I'm not mistaken you indicated that once as well). There are "best practices" and also there are attempts to do things with (again non standard) decorators) ...

I think the default behavior could be as it is now but when there is the option as given in #6555 this would be favorable for some projects that don't follow the ad-hoc rule.

@lelegard
Copy link
Author

That is your opinion.

This is not my opinion. This is the common usage in most existing Python code. Should I have any opinion on this, it would be that the object model of Python is broken 😄 (this, for sure, is an "opinion" from a C++ programmer and former Java and Ada programmer).

The real problem is the fact that there is in python no real notion of private / protected etc. and that there is no real standard for python (when I'm not mistaken you indicated that once as well).

Agreed.

There are "best practices" and also there are attempts to do things with (again non standard) decorators) ...

Precisely. "Common" practices (I don't know if I can call them "best") are to use one underscore for private fields.

I think the default behavior could be as it is now but when there is the option as given in #6555 this would be favorable for some projects that don't follow the ad-hoc rule.

Agreed. The default is not that important ("best" practices or not), as long as you can specify other conventions. But nobody cared merging #6555 in two years. That is the problem.

@lelegard
Copy link
Author

As additional information, I confirm that the change (without option to override it) in 1.9.6 breaks a lot of things, even if you try to adapt to the imposed convention of protected/private variables in 1.9.6.

I tried to adapt the code to these new conventions.

One could argue that applying the new convention was useful to discover three other bugs. But, honestly, programmers should not have to do this on existing code.

This is why I ask again to either revert this controversial new convention or add an option in Doxyfile to override it.

@albert-github
Copy link
Collaborator

I had a look at the #6555 and in its current state it is hard to merge, too many changes. The ideas behind it are still valid so I'll have a look at it to incorporate this functionality.

(When it is that important to you you can also create a proposed pull request with the mentioned functionality from #6555)

@doxygen doxygen closed this as completed Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python Usage The mentioned problem is not a doxygen problem but due to usage of a feature.
Projects
None yet
Development

No branches or pull requests

3 participants