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.

Sign up for GitHub

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

Jump to bottom

fix: closes #11343's [attr-defined] type errors #11345

Merged
merged 9 commits into from
Aug 27, 2023
Merged

fix: closes #11343's [attr-defined] type errors #11345

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2a91c63
fix: closes #11343's [attr-defined] type errors
WarrenTheRabbit Aug 23, 2023
7600243
Merge branch 'main' into add-type-ignore-for-attr-defined-errors
WarrenTheRabbit Aug 24, 2023
15ebcf5
refactor: replace type:ignore with platform check
WarrenTheRabbit Aug 24, 2023
fe67feb
Merge branch 'add-type-ignore-for-attr-defined-errors' of https://git…
WarrenTheRabbit Aug 25, 2023
2d5bfa8
refactor: add self-documenting UNRELIABLE keyword
WarrenTheRabbit Aug 25, 2023
f9aa628
refactor: self-document error flag with a constant
WarrenTheRabbit Aug 26, 2023
af4ebeb
doc: improve explanation of return values
WarrenTheRabbit Aug 27, 2023
60f6566
refactor: change name of self-documenting constant
WarrenTheRabbit Aug 27, 2023
c7b602c
doc: remove unwanted implication
WarrenTheRabbit Aug 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/_pytest/compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ def get_user_id() -> int | None:
return None
# getuid shouldn't fail, but cpython defines such a case.
# Let's hope for the best.
uid = os.getuid()
uid = os.getuid() # type: ignore[attr-defined]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this pass if the else block was used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this pass if the else block was used?

Hi, Ronny. Thanks for taking a look. I'll need a clarification, and I also thank you for your patience while I'm learning.

Are you asking
1. what happens if I remove the type: ignore and use an else block instead?
2. what happens when the current code is executed through the implied else block (lines 322 to 325)?
3. something else?

Responding to 1 (use an else block instead)

The Python docs state that os.getuid only has availability on Unix. Since my system is Windows, changing the code to

    if sys.platform in ("win32", "emscripten"):
        return None
    # getuid shouldn't fail, but cpython defines such a case.
    # Let's hope for the best.
    else:
        uid = os.getuid()
        return uid if uid != -1 else None

still fails the mypy hook:

pre-commit run mypy --files .\src\_pytest\compat.py
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

src\_pytest\compat.py:325: error: Module has no attribute "getuid"; maybe "getpid" or "getppid"?  [attr-defined]
Found 1 error in 1 file (checked 1 source file)

But either option (using type: ignore or an explicit if: else: block) passes when I run the hook on VS Code's WSL.

I'm still very new and I'm trying to understand the motivation behind the question. Are there advantages to having an else block instead of the type: ignore comment - such that you would prefer it, if possible?

Responding to 2 (execute through lines 321 - 325)

The function seems to work fine when executed on a Windows system (it halts at the guard on line 320 and returns None) and a Unix system (it completes execution at 325 and returns uid if uid != -1 else None.

If this is the heart of your question, would you like me to write a test that demonstrates that?

Responding to 3

I worry I have not understood the underlying concern and that my responses do not address them!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the response,

I was wondering about 1

My concerns are resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is someone able to give feedback on my communication? I've reflected and think this would have been easier for Ronny:

Hi, Ronny. Thanks for taking a look. I'm quite new and I'm not confident I have understood the question but I'll respond with the interpretation I think is most likely - let me know if I miss the mark.

(and then the 'Responding to 1' content as before)

I know everyone is busy. But I'd like to flag that I am always open to and welcome feedback. I'm still learning. Moreover, I want to be a very harmonious and easy-to-work-with contributor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're fine in my book 🙃 I didn't understand what Ronny meant exactly either. If he would have meant something else, your detailed answer would have prevented another round-trip.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WarrenTheRabbit it's always fine to ask for clarifications. Let me explain a bit more what (I think) @RonnyPfannschmidt was aiming for.

Some functions, like os.getuid, are only defined on certain platforms. This is reflected in the type definition of os.getuid in typeshed, the project which defines the typing for all of the standard library. See here:

https://github.com/python/typeshed/blob/a094aa09c2aa47721664d3fdef91eda4fac24ebb/stdlib/os/__init__.pyi#L460-L476

You can see that os.getuid is only defined when sys.platform != "win32".

Now, mypy is a static type checker, and such platform-dependence puts it in a conundrum - which platform should it use when type checking? This can cause meaningful differences, like in this example - when run on linux platform (like CI) there is no error, but on Windows there is. What mypy does is to type-check using the platform it is running on, but you can ask it use a different platform using for example mypy --platform win32 or mypy --platform linux.

So we see that mypy can give different results on different platforms, that's not great. But it turns out that mypy does provide a solution to this -- it understands if sys.platform == "..." checks and knows to ignore either the if or the else according to the platform it's using. See here for the mypy docs on this.

OK, if mypy does this then why doesn't work here? The reason is that while mypy is smart, it's not smart enough to understand if sys.platform in (...) checks. So to help it we need to write the code using or and else instead:

    if sys.platform == "win32" or sys.platform == "emscripten":
         return None
    else:
        # getuid shouldn't fail, but cpython defines such a case.
        # Let's hope for the best.
        uid = os.getuid()
        return uid if uid != -1 else None

Now when running with win32 platform, mypy ignores the else branch and everything is good :)

I recommend switching to this, as it removes the need for the type: ignore which is better avoided when possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case we might also want report a upstream bug for mypy
after all previously if/else made sense due to win32, now with emscripten, its much more likely to see those in checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you everyone for being so welcoming and supportive - either in this repo or on LinkedIn. 🙏
And thank you, @bluetech, for taking the time to elaborate and teach me. 🎓💛

bluetech: I recommend switching to {non-containment checks}, as it removes the need for the type: ignore which is better avoided when possible.

Perfect. I'll do that. I've done that.
I'll also I've added a comment that gives the reader more type checking context. But I can remove this if the information is obvious to all but the very inexperienced:

    # mypy follows the version and platform checking expectation of PEP 484:
    # https://mypy.readthedocs.io/en/stable/common_issues.html?highlight=platform#python-version-and-system-platform-checks
    # Containment checks are too complex for mypy v1.5.0 and cause failure.

Outstanding items

Ronny: in that case we might also want report a upstream bug for mypy
after all previously if/else made sense due to win32, now with emscripten, its much more likely to see those in checks

1

That sounds like a great idea, @RonnyPfannschmidt. I'll happily look into how to do that. I've posted to the python/typing gitter with my understanding of the motivation and asked how to officially discuss the code reachability issue. I'll report in when I have been successful or become stuck.

2

The in-code comments confused me.

 # getuid shouldn't fail, but cpython defines such a case.
 # Let's hope for the best.

I don't know what we are hoping for or if it is a good idea to! So I'll be looking into that.
At a minimum, I'd like to link to something that gives more context (unless that is a bad idea?).

3

I need to figure out how to add the discussed changes in a way that maintains continuity. Am unclear how to do

"Add more commits by pushing to the add-type-ignore-for-attr-defined-errors branch on WarrenTheRabbit/pytest."

as advised.

3

I need to understand and resolve the codecov/patch failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The in-code comments confused me

I believe it means that the return from os.getuid() should always work, but it might return -1 so we account for that in the last line:

return uid if uid != -1 else None

We should move that comment to above that line. 👍

return uid if uid != -1 else None


Expand Down
3 changes: 2 additions & 1 deletion testing/test_parseopt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,8 @@ def test_multiple_metavar_help(self, parser: parseopt.Parser) -> None:

def test_argcomplete(pytester: Pytester, monkeypatch: MonkeyPatch) -> None:
try:
encoding = locale.getencoding() # New in Python 3.11, ignores utf-8 mode
# New in Python 3.11, ignores utf-8 mode
encoding = locale.getencoding() # type: ignore[attr-defined]
except AttributeError:
encoding = locale.getpreferredencoding(False)
try:
Expand Down