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

Check autodoc's exclude_members before mock. #8653

Closed
wants to merge 1 commit into from

Conversation

jezdez
Copy link

@jezdez jezdez commented Jan 4, 2021

This prevents accidently checking for the sphinx_mock attribute on objects that may be proxies or other lazily evaluated objects that shouldn't be documented and are included in exclude_members.

Subject: Check autodoc's exclude_members before mock.

Feature or Bugfix

  • Bugfix

Purpose

  • The purpose is to make sure that laziliy evaluated objects aren't accidently triggering side-effects in this code path, such as Flask objects g, request, current_app etc when they are module globals.

Detail

  • Changes the order of evaluation when considering the members to document, evaluating exclude_members first.

Relates

This prevents accidently checking for the __sphinx_mock__ attribute on objects that may be proxies or other lazily evaluated objects that shouldn't be documented and are included in exclude_members.
@tk0miya
Copy link
Member

tk0miya commented Jan 5, 2021

Thank you for the proposal. But I'm thinking #8656 is better for the case of requests module because this is a workaround.

@jezdez
Copy link
Author

jezdez commented Jan 6, 2021

@tk0miya Ah, hm, I'm not sure I agree since it's not just AttributeError that could be raised when getattr fails, here's the traceback of the error that was relating to having Flask's thread local proxies as module globals:

Traceback (most recent call last):
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/cmd/build.py", line 280, in build_main
    app.build(args.force_all, filenames)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/application.py", line 352, in build
    self.builder.build_update()
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 296, in build_update
    self.build(to_build,
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 310, in build
    updated_docnames = set(self.read())
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 417, in read
    self._read_serial(docnames)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 438, in _read_serial
    self.read_doc(docname)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 478, in read_doc
    doctree = read_doc(self.app, self.env, self.env.doc2path(docname))
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/io.py", line 221, in read_doc
    pub.publish()
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/core.py", line 217, in publish
    self.document = self.reader.read(self.source, self.parser,
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/io.py", line 126, in read
    self.parse()
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/readers/__init__.py", line 77, in parse
    self.parser.parse(self.input, document)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/parsers.py", line 104, in parse
    self.statemachine.run(inputlines, document, inliner=self.inliner)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 170, in run
    results = StateMachineWS.run(self, input_lines, input_offset,
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/statemachine.py", line 241, in run
    context, next_state, result = self.check_line(
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/statemachine.py", line 459, in check_line
    return method(match, context, next_state)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 2769, in underline
    self.section(title, source, style, lineno - 1, messages)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 327, in section
    self.new_subsection(title, lineno, messages)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 393, in new_subsection
    newabsoffset = self.nested_parse(
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 281, in nested_parse
    state_machine.run(block, input_offset, memo=self.memo,
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 196, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/statemachine.py", line 241, in run
    context, next_state, result = self.check_line(
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/statemachine.py", line 459, in check_line
    return method(match, context, next_state)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 2769, in underline
    self.section(title, source, style, lineno - 1, messages)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 327, in section
    self.new_subsection(title, lineno, messages)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 393, in new_subsection
    newabsoffset = self.nested_parse(
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 281, in nested_parse
    state_machine.run(block, input_offset, memo=self.memo,
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 196, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/statemachine.py", line 241, in run
    context, next_state, result = self.check_line(
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/statemachine.py", line 459, in check_line
    return method(match, context, next_state)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 2342, in explicit_markup
    nodelist, blank_finish = self.explicit_construct(match)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 2354, in explicit_construct
    return method(self, expmatch)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 2096, in directive
    return self.run_directive(
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/docutils/parsers/rst/states.py", line 2146, in run_directive
    result = directive_instance.run()
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/ext/autodoc/directive.py", line 146, in run
    documenter.generate(more_content=self.content)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/ext/autodoc/__init__.py", line 959, in generate
    self.document_members(all_members)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/ext/autodoc/__init__.py", line 821, in document_members
    for (mname, member, isattr) in self.filter_members(members, want_all):
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/ext/autodoc/__init__.py", line 734, in filter_members
    if ismock(member):
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/sphinx/ext/autodoc/mock.py", line 156, in ismock
    if not hasattr(subject, '__sphinx_mock__'):
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/werkzeug/local.py", line 347, in __getattr__
    return getattr(self._get_current_object(), name)
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/werkzeug/local.py", line 306, in _get_current_object
    return self.__local()
  File "/Users/jezdez/Code/git/python-dockerflow/.tox/py38-docs/lib/python3.8/site-packages/flask/globals.py", line 52, in _find_app
    raise RuntimeError(_app_ctx_err_msg)
RuntimeError: Working outside of application context.

@tk0miya
Copy link
Member

tk0miya commented Jan 6, 2021

My understanding is this helps avoiding the error via :exclude-members: option. It means this requires users to skip the members that raises an error on getattr, manually. I feel it is a workaround. And #8656 can ignore all of errors on getattr. It uses safe_getattr() to handle all kinds of exceptions. So I consider my PR makes this unnecessary, right?

@jezdez
Copy link
Author

jezdez commented Jan 6, 2021

It uses safe_getattr() to handle all kinds of exceptions

Ah, I missed that part, apologies, thanks for elaborating @tk0miya!

@jace
Copy link

jace commented Jan 7, 2021

Perhaps safe_getattr needs to be complemented with safe_hasattr? As the hasattr call is breaking on proxies.

@tk0miya
Copy link
Member

tk0miya commented Jan 7, 2021

It would be better to implement safe_hasattr to optimize it. But it is not required. Original hasattr() uses getattr() internally. So we can implement hasattr() using getattr().

@tk0miya tk0miya closed this Jan 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants