attrs icon indicating copy to clipboard operation
attrs copied to clipboard

Use enum for typing NOTHING

Open KevinMGranger opened this issue 3 years ago • 6 comments

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

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

KevinMGranger avatar Jul 14 '22 16:07 KevinMGranger

Hm, I don't hate it.

Tinche avatar Jul 14 '22 17:07 Tinche

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

KevinMGranger avatar Jul 14 '22 18:07 KevinMGranger

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? 😅

hynek avatar Jul 27 '22 12:07 hynek

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.

KevinMGranger avatar Aug 01 '22 14:08 KevinMGranger

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.

hynek avatar Aug 01 '22 17:08 hynek

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 avatar Aug 08 '22 23:08 KevinMGranger

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

Tinche avatar Aug 15 '22 11:08 Tinche

yeah, because it probably breaks someone too

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

hynek avatar Aug 15 '22 12:08 hynek

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 avatar Aug 15 '22 15:08 KevinMGranger

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?

KevinMGranger avatar Aug 15 '22 15:08 KevinMGranger

Where are you seeing this Optional[_T] thing?

Tinche avatar Aug 15 '22 22:08 Tinche

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.

KevinMGranger avatar Aug 15 '22 23:08 KevinMGranger

What are the next steps here?

hynek avatar Aug 16 '22 05:08 hynek

We add a short changelog entry and merge this.

Tinche avatar Aug 16 '22 10:08 Tinche

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

Tinche avatar Aug 27 '22 15:08 Tinche

Thanks @KevinMGranger!

hynek avatar Aug 27 '22 15:08 hynek

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 avatar Aug 30 '22 11:08 KevinMGranger

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.)

KevinMGranger avatar Aug 30 '22 12:08 KevinMGranger

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

Tinche avatar Aug 30 '22 12:08 Tinche

Does it help if you annotate the alias with Final?

Tinche avatar Aug 30 '22 12:08 Tinche

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?

hynek avatar Aug 30 '22 13:08 hynek

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

Tinche avatar Aug 30 '22 13:08 Tinche