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

False positive for "--enable default-role" in 0.6.2 #46

Closed
hugovk opened this issue Oct 6, 2022 · 17 comments
Closed

False positive for "--enable default-role" in 0.6.2 #46

hugovk opened this issue Oct 6, 2022 · 17 comments

Comments

@hugovk
Copy link
Collaborator

hugovk commented Oct 6, 2022

$ git clone https://github.com/python/devguide
$ cd devguide
$ python -m pip install sphinx-lint==0.6.1 -q
$ sphinx-lint getting-started/setup-building.rst --enable default-role
No problems found.
$ python -m pip install sphinx-lint==0.6.2 -q
$ sphinx-lint getting-started/setup-building.rst --enable default-role
getting-started/setup-building.rst:28: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:32: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:49: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:50: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:73: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:79: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:93: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:94: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:101: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:122: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:142: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:159: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:166: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:175: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:176: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:198: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:203: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:204: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:205: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:210: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:211: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:212: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:222: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:223: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:226: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:231: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:267: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:288: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:303: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:308: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:313: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:314: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:318: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:323: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:334: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:362: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:379: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:382: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:385: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:404: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:412: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:418: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:444: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:449: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:451: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:452: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:453: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:454: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:456: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:459: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:460: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:461: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:464: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:465: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:466: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:467: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:482: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:483: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:485: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:499: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:512: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:514: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:526: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:556: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:560: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:564: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:567: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:570: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:573: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:577: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:581: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:584: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:587: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:591: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:595: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:600: default role used (hint: for inline literals, use double backticks) (default-role)
getting-started/setup-building.rst:604: default role used (hint: for inline literals, use double backticks) (default-role)

The few of these checked look fine though:

Install ``git``
===============

https://github.com/python/devguide/blame/main/getting-started/setup-building.rst#L28

``Tools``
     Various tools that are (or have been) used to maintain Python.

https://github.com/python/devguide/blame/main/getting-started/setup-building.rst#L604

@CAM-Gerlach
Copy link

Same thing happened in the CPython docs in python/cpython#97962 ; the new regex generated over 18 000 false positives and at least in my spot checks, appeared to match on most if not all double backtick inline string literals.

@ezio-melotti
Copy link
Collaborator

I tested this on the "friends projects" and got:

$ # with an older version of sphinx-lint
$ sphinx-lint tests/fixtures/friends/ --enable=default-role | wc -l
598
$ # with the latest version
$ sphinx-lint tests/fixtures/friends/ --enable=default-role | wc -l
33604
$ sphinx-lint tests/fixtures/friends/cpython/ --enable=default-role | wc -l
18086
$ sphinx-lint tests/fixtures/friends/devguide/ --enable=default-role | wc -l
865
$ # no issues without --enable=default-role
$ sphinx-lint tests/fixtures/friends/
No problems found.

@JulienPalard
Copy link
Collaborator

Oops.

@JulienPalard
Copy link
Collaborator

I just published v0.6.3 with a fix for this:

$ sphinx-lint --enable default-role devguide/
devguide/testing/coverage.rst:262: default role used (hint: for inline literals, use double backticks) (default-role)
devguide/documentation/style-guide.rst:154: default role used (hint: for inline literals, use double backticks) (default-role)
devguide/internals/parser.rst:836: default role used (hint: for inline literals, use double backticks) (default-role)
devguide/internals/parser.rst:837: default role used (hint: for inline literals, use double backticks) (default-role)
devguide/internals/parser.rst:838: default role used (hint: for inline literals, use double backticks) (default-role)

@JulienPalard
Copy link
Collaborator

@CAM-Gerlach The 0.6.3 version finds 83 default-role (the few of them I checked were true positives) in cpython, I'll try to fix them later.

@JulienPalard
Copy link
Collaborator

Please keep this issue open until we find a proper way to check friend repositories with the right flags (I mean in download-more-tests.sh) to avoid this kind of surprises at release time.

(download-more-tests.sh has been written just so we don't have this kind of surprises at release time...)

@ezio-melotti
Copy link
Collaborator

ezio-melotti commented Oct 6, 2022

This is a false positive:

$ echo '`issue tracker`_ and submit a :ref:`pull request <pullrequest>`' > buggy.rst
$ sphinx-lint --enable=default-role buggy.rst 
buggy.rst:1: default role used (hint: for inline literals, use double backticks) (default-role)
$ echo '`tracker`_ and submit a :ref:`pull request <pullrequest>`' > buggy.rst
$ sphinx-lint --enable=default-role buggy.rst 
buggy.rst:1: default role used (hint: for inline literals, use double backticks) (default-role)
$ echo '`tracker`_ and submit a :ref:`pull`' > buggy.rst
$ sphinx-lint --enable=default-role buggy.rst 
buggy.rst:1: default role used (hint: for inline literals, use double backticks) (default-role)

$ echo '`issue tracker`_ and submit a' > buggy.rst
$ sphinx-lint --enable=default-role buggy.rst 
No problems found.
$ echo 'and submit a :ref:`pull request <pullrequest>`' > buggy.rst
$ sphinx-lint --enable=default-role buggy.rst 
No problems found.

It fails when a `link`_ is followed by a :role:`text`.

@ezio-melotti
Copy link
Collaborator

Another one:

$ cat buggy.rst 
Instead, these security concerns should be gathered into a dedicated
"Security Considerations" section within the module's documentation, and
cross-referenced from the documentation of affected interfaces with a note
similar to ``"Please refer to the :ref:`security-considerations` section
for important information on how to avoid common mistakes."``.

Similarly, if there is a common error that affects many interfaces in a
$ sphinx-lint --enable=default-role buggy.rst 
buggy.rst:4: default role used (hint: for inline literals, use double backticks) (default-role)
$ echo 'A ref in a literal looks like: ``foo :ref:`bar` baz``.' > buggy.rst 
$ sphinx-lint --enable=default-role buggy.rst 
No problems found.

The issue seem to be related to the multiline literal that contains a pair of `...` -- it works fine when it's on a single line.

@hugovk
Copy link
Collaborator Author

hugovk commented Oct 6, 2022

A false positive multiline with ellipsis:

:class:`subprocess.Popen` destructor now emits a :exc:`ResourceWarning` warning
if the child process is still running. Use the context manager protocol (``with
proc: ...``) or explicitly call the :meth:`~subprocess.Popen.wait` method to
read the exit status of the child process. (Contributed by Victor Stinner in
:issue:`26741`.)
1.rst:3: default role used (hint: for inline literals, use double backticks) (default-role)

And multiline without ellipsis:

A new :data:`~html.entities.html5` dictionary that maps HTML5 named character
references to the equivalent Unicode character(s) (e.g. ``html5['gt;'] ==
'>'``) has been added to the :mod:`html.entities` module.  The dictionary is
now also used by :class:`~html.parser.HTMLParser`.  (Contributed by Ezio
Melotti in :issue:`11113` and :issue:`15156`.)
1.rst:3: default role used (hint: for inline literals, use double backticks) (default-role)

@hugovk
Copy link
Collaborator Author

hugovk commented Oct 6, 2022

It's not recognising the :envvar: role, due to the other opening and closing double backticks being on another line:

   * ``-X importtime`` to show how long each import takes. It shows module
     name, cumulative time (including nested imports) and self time (excluding
     nested imports).  Note that its output may be broken in multi-threaded
     application.  Typical usage is ``python3 -X importtime -c 'import
     asyncio'``.  See also :envvar:`PYTHONPROFILEIMPORTTIME`.

1.rst:5: default role used (hint: for inline literals, use double backticks) (default-role)


For example, no complaints about this:

   * ``-X importtime`` to show how long each import takes. It shows module
     name, cumulative time (including nested imports) and self time (excluding
     nested imports).  Note that its output may be broken in multi-threaded
     application.  Typical usage is python3 -X importtime -c 'import
     asyncio'.  See also :envvar:`PYTHONPROFILEIMPORTTIME`.

(As it happens, PyCharm has a similar bug)

image

image

@JulienPalard
Copy link
Collaborator

Wow thanks all for your examples!

I just released v0.6.4 that should fix all of them, can you try it?

@CAM-Gerlach
Copy link

CAM-Gerlach commented Oct 6, 2022

A clean run on the PEP repo comes back green ✔️

FWIW, maybe a little too late now, but the pre-commit pygrep check uses ^(?! ).*(^| )`[^`]+`([^_]|$), and while I don't know about false negatives, I've very rarely seen false positives with it.

@hugovk
Copy link
Collaborator Author

hugovk commented Oct 6, 2022

All good with 0.6.4 and 0.6.5! Thanks for the quick fixes and releases!

@ezio-melotti
Copy link
Collaborator

v0.6.5 was released too, and it fixes this:

$ cat buggy.rst
.. this is a comment
   this is not a `default role`
$ sphinx-lint --enable=default-role buggy.rst 
buggy.rst:2: default role used (hint: for inline literals, use double backticks) (default-role)

@CAM-Gerlach
Copy link

CAM-Gerlach commented Oct 7, 2022

Thanks for your hard work at short notice, @JulienPalard @ezio-melotti and @hugovk .

@CAM-Gerlach
Copy link

CAM-Gerlach commented Oct 7, 2022

Just to note, I can confirm that #34 (false positive when last character inside a verbatim is `:`` is still present and reproducible on the PEPs repo with Sphinx-Lint 0.6.5, so we still can't enable this flag for now, unfortunately.

@JulienPalard
Copy link
Collaborator

I'm closing this issue as I think all reported false positives are now fixed, don't hesitate to report new ones as new issues.

We were having a script to fetch "friend repos" (cpython, sphinx, ...) to test them before relasing to avoid this exact issue, but it was just testing with the default flags, not with --enable default-role. Now each repo is tested with its own set of flags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants