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

Use enum for typing NOTHING #983

Merged
merged 9 commits into from
Aug 27, 2022
Merged

Conversation

KevinMGranger
Copy link
Contributor

@KevinMGranger KevinMGranger commented Jul 14, 2022

Summary

This will allow those extending attrs to type NOTHING as Literal[NOTHING].

This is the recommended way of doing this, at least until PEP-661 lands.

I also just realized that Attribute types default as Optional[_T], when it should really be Union[_T, Literal[NOTHING]]. This would be a prerequisite for that.

Pull Request Check List

  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
    • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

@Tinche
Copy link
Member

Tinche commented Jul 14, 2022

Hm, I don't hate it.

@KevinMGranger
Copy link
Contributor Author

Forgot to mention-- this is the recommended way of doing this, at least until PEP-661 lands.

@hynek
Copy link
Member

hynek commented Jul 27, 2022

I'm not really firm on these topics, so I'm assigning this to Tin. I hope the test_mypy failures are just caused by this? 😅

@KevinMGranger
Copy link
Contributor Author

I didn't see any local failures nor failures in CI-- maybe I missed something?

One thing I didn't consider is that mypy supports python 2-- perhaps that's what's failing? I don't have any experience with how it handles stub files for python 2.

@hynek
Copy link
Member

hynek commented Aug 1, 2022

Don't worry, it's not failing anymore. That was just mypy improving attrs support and subtly changing an error message. I suspect you haven't seen an error, because your local mypy was slightly older.

@Tinche Tinche self-requested a review August 8, 2022 13:02
src/attr/__init__.pyi Outdated Show resolved Hide resolved
@KevinMGranger
Copy link
Contributor Author

KevinMGranger commented Aug 8, 2022

NOTHING is now the single variant of an Enum, keeping __repr__ and __bool__'s function.

However, since a type stub file overrides any typing from the python module, the enum has to be (mostly) repeated in the stub. If we're fine with it, that's okay. If not, we can work around this by putting it in its own module, and reimporting it in _make.py and __init__.pyi.

I'm looking into how the enum affects the docs automation right now fixed.

@KevinMGranger KevinMGranger marked this pull request as ready for review August 8, 2022 23:47
@hynek hynek added this to the 22.2.0 milestone Aug 11, 2022
@Tinche
Copy link
Member

Tinche commented Aug 15, 2022

@hynek I think this is ready to go. We we want a changelog entry?

@hynek
Copy link
Member

hynek commented Aug 15, 2022

yeah, because it probably breaks someone too

ideally with a short summary of the upsides please because i keep forgetting myself

@KevinMGranger
Copy link
Contributor Author

KevinMGranger commented Aug 15, 2022

The only upside is that it allows the use of Literal[NOTHING] in type signatures. That's useful for those extending attrs.

A project I'm working on does some of its own initialization logic, and inspecting Attribute properties to see if they're NOTHING is necessary. Using it as a return type annotation helps us make sure it's handled properly down the line.

(We have a compatibility shim that just re-exports NOTHING, but type stubs it as an enum.)

(Side note: if you want to know why we're recreating so much initialization logic...)

It's so we can:

  1. Log errors for missing data all at once, instead of failing on the first error.
  2. Log which environment variable the value came from, since there are legacy env vars and some have precedence over others. We could make that part of an attrs.Factory function, but we also want to...
  3. Inspect Attribute names so that sensitive data has a better chance of being redacted in case of a mistake. The name isn't available to attrs.Factory functions.

@KevinMGranger
Copy link
Contributor Author

KevinMGranger commented Aug 15, 2022

Ah, I forgot to revisit this one part:

Attribute.default's typing is technically incorrect. It's typed as Optional[_T]. For example:

@frozen
class Foo:
    no_default: int = field()
    two_default: int = field(default=2)

Someone inspecting the Attribute[int] for no_default will have default typed as Optional[int], and will be surprised to find a NOTHING! Similarly, inspecting Attribute[int] for two_default will have someone handling the None case, even though it can't be None. Having it be Union[_T, Literal[NOTHING]] would be correct.

I'll have to see if there are other examples. I don't see any other examples.

If you want, fixing this could be handled as part of this PR, or another. Like you said, it might break someone's code (but I think it'll mainly just change up the type checking they do). Doing it in this PR would mean they handle it all in one fell swoop. Thoughts?

@Tinche
Copy link
Member

Tinche commented Aug 15, 2022

Where are you seeing this Optional[_T] thing?

@KevinMGranger
Copy link
Contributor Author

KevinMGranger commented Aug 15, 2022

In attrs.Attribute's type stub.

I just tried changing it, and every mypy test starting failing. Maybe that requires a change to the plugin as well? Might be out of scope for this PR then.

@hynek
Copy link
Member

hynek commented Aug 16, 2022

What are the next steps here?

@Tinche
Copy link
Member

Tinche commented Aug 16, 2022

We add a short changelog entry and merge this.

@Tinche
Copy link
Member

Tinche commented Aug 27, 2022

@hynek I've added a small changelog entry, merge at will.

@hynek hynek merged commit c860e9d into python-attrs:main Aug 27, 2022
@hynek
Copy link
Member

hynek commented Aug 27, 2022

Thanks @KevinMGranger!

@KevinMGranger KevinMGranger deleted the NOTHING_as_enum branch August 28, 2022 23:12
@KevinMGranger
Copy link
Contributor Author

KevinMGranger commented Aug 30, 2022

I'm seeing really weird behavior. This works perfectly fine within attrs, but when importing it outside of attrs, mypy gives a weird complaint:

# foo.py
from typing import Literal
from attrs import NOTHING

def foo(val: str | Literal[NOTHING]) -> None:
    pass
$ mypy foo.py
foo.py:4: error: Parameter 1 of Literal[...] is invalid
foo.py:4: error: Variable "attr.NOTHING" is not valid as a type
foo.py:4: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
Found 3 errors in 1 file (checked 1 source file)

Anyone else seeing this? I'm trying to debug if this is a mypy issue.

Edit: even weirder, I added a method to the TestNothing class in test_dunders.py:

    def test_type(self, val: str | Literal[NOTHING]):
        pass

No complaints from mypy! (Edit: weirder and weirder. It doesn't complain within the VS Code mypy integration, but complains on the CLI.)

If I can't figure this out soon, you may want to back out this change. Sorry :(

KevinMGranger added a commit to KevinMGranger/attrs that referenced this pull request Aug 30, 2022
@KevinMGranger
Copy link
Contributor Author

pyright doesn't complain, while mypy does. It looks like I misread GVR's suggestion, and missed:

(Alas, I haven't found a way to alias A = AA.A and be able to write Literal[A].)

There are two (and a half) options here:

  1. Revert this change
  2. Rename the enum class Nothing, use Nothing for type annotations, and NOTHING for the value. If that's too confusing for users, 1 is the better option.
  3. Contribute to mypy so this works (kidding. maybe.)

@Tinche
Copy link
Member

Tinche commented Aug 30, 2022

I think we can leave it and submit a bug report to Mypy. Unfortunate.

@Tinche
Copy link
Member

Tinche commented Aug 30, 2022

Does it help if you annotate the alias with Final?

@hynek
Copy link
Member

hynek commented Aug 30, 2022

Re: MyPy option: how broken is this? Is this gonna make me fix all my projects like the AttrsInstance thing in 22.1 or is this something that's only broken if you're trying to use the new feature it's supposed to implement?

@Tinche
Copy link
Member

Tinche commented Aug 30, 2022

Yeah, I think it's only broken if you try to Literal it, and hopefully Mypy fixes that.

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

Successfully merging this pull request may close these issues.

3 participants