missing-param-doc differences between 2.11.1 and 2.12.1

[allanc65]

Bug description

False positive on missing-param-doc when transitioning from Pylint 2.11.1 to 2.12.1, with code:
Code is:

""" Module docstring.
"""

def run_callback(callback: int) -> None:
    """ Run the call-back, handles sync and async functions.
        Args:
            callback:
                sync or async function which will be called
    """
    print(callback)

Interestingly, if name and description on same line, it doesn't complain. The re_param_line regex for GoogleDocstring in extensions/_check_docs_utils.py seems to allow for a single line with colon, or multi-line with no colon \s* (\*{{0,2}}\w+)(\s?(:|\n)) but I couldn't get any multi-line working.

See https://stackoverflow.com/questions/70196858/pylint-2-12-1-different-behaviour-from-2-11-1 for original question on SO.

Configuration

No response

Command used

pylint qq.py

Pylint output

************* Module qq
qq.py:4:0: W9015: "callback" missing in parameter documentation (missing-param-doc)

Expected behavior

Expected no errors.

Pylint version

v2.12.1

OS / Environment

Ubuntu and WSL2.

Additional dependencies

No response

[allanc65]

Daniel's probably already all over this issue but I'll attach some artefacts just in case it makes life easier. This file includes the pylintrc we use (a slightly modified Google-external one), the test program test5452.py with four functions (the two with the multi-line param specifications, one with and one without colon, are the ones that raise an issue), and the output we get from pylint.
test5452.tar.gz

[DanielNoord]

I have not looked at this particular issue yet, just assigned myself to show that this in on my radar and on my todo list for soon™. (Also because I was likely the one we caused this with changes for 2.12 😉 )

Thanks for the examples, that will help fix this more easily 😄

[allanc65]

Daniel et al, I think you need to look at this bit of code in extensions/_check_docs_utils.py:

            re_only_desc = re.search(":\n", entry)
            if re_only_desc:
                param_name = match.group(1)
                param_desc = match.group(2)
                param_type = None
            else:
                param_name = match.group(1)
                param_type = match.group(2)
                param_desc = match.group(3)

I'm not entirely certain what was going through the mind of the developer there, it appears that the :\n is used to decide that the type isn't available but that doesn't seem right. The presence of :\n means that the description is on the following line but the fact that you're using \s means line breaks are treated the same as any other white space in the section so it's irrelevant.

In any case, the regex used for this captures all three groups whether the type is there or not. If it's not there, the second group (the parameter type) is set to None and the third group is still the description (it doesn't move to group 2), as per my debug code below (important lines begin with PAX match_param_docs group):

PAX GoogleDocstring: self.doc =  Some random docstring.
        Args:
            param:
                sync or async function which will be called
    
PAX GoogleDocstring: self.re_param_section.search(self.doc) = <re.Match object; span=(24, 121), match='        Args:\n            param:\n              >

PAX match_param_docs match entry 1 |['            param:\n                sync or async function which will be called']|: |['            param:\n                sync or async function which will be called']|
PAX match_param_docs match entry 2 |['            param:\n                sync or async function which will be called']|: |['            param:\n                sync or async function which will be called']|

PAX match_param_docs match entry |            param:
                sync or async function which will be called|: |<re.Match object; span=(0, 78), match='            param:\n                sync or async>|

PAX match_param_docs group 0: '            param:
                sync or async function which will be called'
PAX match_param_docs group 1: 'param'
PAX match_param_docs group 2: 'None'
PAX match_param_docs group 3: 'sync or async function which will be called'

PAX match_param_docs block 1, name='param', desc='None', type ='None'
PAX match_param_docs: params_with_doc  = |set()|
PAX match_param_docs: params_with_type = |set()|

test5452/test5452.py:18:0: W9015: "param" missing in parameter documentation (missing-param-doc)

If the code is instead changed into (no if needed):

            param_name = match.group(1)
            param_type = match.group(2)
            param_desc = match.group(3)

then the first case in my test code still works, and the second one starts working, the two styles shown below:

Args:
    first_param: description
    second_param:
        description

The other two test cases don't work (the ones where the parameter name aren't followed by a colon) but I think you can make an argument that they ARE actually invalid, the colon being necessary to separate parameter from description.

If you want those to work, you'll need to do some more work. I'd be happy with just what I've done so far but others may want more. For now, we've version locked our build system to 2.11.1 so we don't get affected by it.

Hope that analysis has helped you bods out.

AllanC.

[DanielNoord]

Thanks for the thorough analysis @allanc65.

If you want you can make the PR yourself and become a contributor to pylint? If not, I'll create the PR myself. The following diff would fix this plus add tests in the functional test framework we're in the process of moving these parameter tests to:

diff --git a/tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.py b/tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.py
new file mode 100644
index 00000000..920366cc
--- /dev/null
+++ b/tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.py
@@ -0,0 +1,18 @@
+"""Tests for missing-param-doc and missing-type-doc for Google style docstrings
+with accept-no-param-doc = no
+
+Styleguide:
+https://google.github.io/styleguide/pyguide.html#doc-function-args
+"""
+# pylint: disable=invalid-name
+
+
+def test_multi_line_parameters(param: int) -> None:
+    """Checks that multi line parameters lists are checkec correctly
+    See https://github.com/PyCQA/pylint/issues/5452
+
+    Args:
+        param:
+            a description
+    """
+    print(param)
diff --git a/tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.rc b/tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.rc
new file mode 100644
index 00000000..dd77ffed
--- /dev/null
+++ b/tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.rc
@@ -0,0 +1,6 @@
+[MASTER]
+load-plugins = pylint.extensions.docparams
+
+[BASIC]
+accept-no-param-doc=no
+docstring-min-length: -1
diff --git a/pylint/extensions/_check_docs_utils.py b/pylint/extensions/_check_docs_utils.py
index 18c2ec0b..5227e74a 100644
--- a/pylint/extensions/_check_docs_utils.py
+++ b/pylint/extensions/_check_docs_utils.py
@@ -646,16 +646,9 @@ class GoogleDocstring(Docstring):
             if not match:
                 continue
 
-            # check if parameter has description only
-            re_only_desc = re.search(":\n", entry)
-            if re_only_desc:
-                param_name = match.group(1)
-                param_desc = match.group(2)
-                param_type = None
-            else:
-                param_name = match.group(1)
-                param_type = match.group(2)
-                param_desc = match.group(3)
+            param_name = match.group(1)
+            param_type = match.group(2)
+            param_desc = match.group(3)
 
             if param_type:
                 params_with_type.add(param_name)

You'll only need to add a changelog entry in Changelog for 2.12.2 and add yourself to the Contributors list. I think after your investigation you deserve the credit for this 😄

[allanc65]

Daniel,

I've made all those changes to a branch taken off the main branch but I'm not able to push to the repo using either my password or the token I got from Github as part of the attempted push. Not sure what the process is here but, if it's too much of a hassle, I'm happy to let you do it. If it's actually done via diff patching, that's way too primitive for my normal workflow. I'm more used to push, PR, approve and merge.

If you do decide to do it yourself, the changes I made are exactly as you suggested but I spelt "checked" correctly :-)

[DanielNoord]

I've made all those changes to a branch taken off the main branch but I'm not able to push to the repo using either my password or the token I got from Github as part of the attempted push. Not sure what the process is here but, if it's too much of a hassle, I'm happy to let you do it. If it's actually done via diff patching, that's way too primitive for my normal workflow. I'm more used to push, PR, approve and merge.

Are you trying to push to the pylint repo or to your own fork? Seeing as you joined Github recently I'm assuming you might not have worked with git or Github previously (please disregard anything I say if this is not the case).
To make this work you would need to fork the repository to your own account, so you have a https://github.com/allanc65/pylint repository. You should then create a branch locally, preferably with a descriptive name (fix-missing-param-dox) where you commit the proposed changes. You then push these changes to GitHub (or this) from your local repository. Then you can create a PR on this repository from your repository. I and others will then review that PR and finally merge it into our main branch.

As I said, if you already know this and are experiencing trouble with passwords and tokens locally disregard anything I said 😄

If you do decide to do it yourself, the changes I made are exactly as you suggested but I spelt "checked" correctly :-)

😄

[allanc65]

No, I didn't know any of that. VsCode did give me the option of creating a fork but I thought that was heavy-handed, a totally separate repo. But, if you guys can bring stuff from a fork back into the main repo, that seems doable. Let me see if I can convince VsCode to give me that option again ...

[allanc65]

Okay, done, that was less painful than I expected. The PR is from allanc65:issue-5452 to PyCQA:main, I hope that the right flow.

[allanc65]

Let me know if there's any deficiencies, I'm happy to fix.

[DanielNoord]

@allanc65 Please don't close issues if they haven't been resolved yet. If for whatever reason we end up not merging your PR then we would lose track of this issue 😉

[allanc65]

@allanc65 Please don't close issues if they haven't been resolved yet. If for whatever reason we end up not merging your PR then we would lose track of this issue 😉

Apologies, Daniel, I (wrongly) assumed you assigned this issue back to me for the purposes of closure. I'll know better next time.

[DanielNoord]

No worries! Everybody needs to learn stuff like this one time :)

[allanc65]

@DanielNoord : quick question (hopefully). I've improved the tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.py file so that it tests a whole lot more variations, as I wanted to cover a few more test cases but mostly make sure I hadn't broken anything :-). If I wanted to get you bods to look at that, is it better to re-open this issue or create a new one. The good new is that I now know how to run the tests, and tox -e py38 -- -k missing_param_doc_required_Google passes all the new variations. So probably a lowish-risk.

[DanielNoord]

@allanc65 Hi, we're actually in the process of moving some of our current tests to that file as well. We're using unit tests to test these right now and want to start using the functional test. Sorry, I should have told you about this before! You can look at the Sphinx file to see what it will look like.
I don't think I'll be able to complete our own move today, but would you be okay with waiting for me to finish moving the current tests before you open a PR with potential new tests? I am sorry about the duplicate work but I think we want to make sure we keep our current test suite and only then add potential new ones to it.

[allanc65]

That's fine, @DanielNoord - I attach the modified file here in case I get hit by a bus in the next few days but will otherwise wait for notification before doing anything further.
missing_param_doc_required_Google.zip