improve ergonomics of checking for @union types with new @decorated_type_checkable decorator #8496

cosmicexplorer · 2019-10-18T04:00:24Z

Problem

We manually check getattr(some_type, '_is_union', False) in multiple places in order to check if some_type was declared with the @union annotation. We would like to canonicalize this check and make it type-safe in some sense.

Solution

Create @decorated_type_checkable in pants.util.meta, which contains two methods:
1. define_instance_of(cls), which wraps a class cls and sets the attribute _decorated_type_checkable_type to the return value of @decorated_type_checkable.
2. is_instance(cls), which checks whether the sentinel attribute _decorated_type_checkable_type matches the type of the return value of @decorated_type_checkable.

### Problem `pants.util.meta.frozen_after_init` has a couple mypy type checking errors, specifically at: https://github.com/pantsbuild/pants/blob/00c4f8cc4a997f80a34f78b6d5fd6fca746befa7/src/python/pants/util/meta.py#L140-L144 This caused a failure on master, see https://travis-ci.org/pantsbuild/pants/jobs/600466692. ### Solution - Add `# type: ignore` to the offending lines until #8496 is merged. ### Result CI is unbroken!

stuhood · 2019-10-22T03:01:31Z

This is pretty meta-programming heavy. Is this actually worth it to avoid Any in this case? It's not a very widespread case, so it feels like maybe just removing the comment and letting it be would be preferable...

Eric-Arellano · 2019-10-22T13:55:08Z

Agreed with Stu. I appreciate how much thought you put into this PR - it’s cool to see how it works.

However, I’d err on the side of keeping things as simple as possible and think Any is fine. My original comment asking for fleshed out type hints was more to go from Any to something slightly more precise like Set[Any], which you’ve done in that precursor.

cosmicexplorer · 2019-10-25T00:44:05Z

I'd be fine reducing this to just the part that adds the @sentinel_attribute decorator in meta.py! I think that being able to do a type-checked union.is_instance(cls) instead of the non-type-checked bool(getattr(input_type, '_is_union', None)) currently in native.py is a huge win. I don't think the rest is worth much and can be tossed.

src/python/pants/util/meta.py

illicitonion

Looks good :) Thanks!

illicitonion · 2019-11-18T14:17:50Z

src/python/pants/engine/objects.py

@@ -167,3 +169,41 @@ def __iter__(self) -> Iterator[_C]:

  def __bool__(self) -> bool:
    return bool(self.dependencies)
+
+
+@sentinel_attribute('_is_union')


Would we be able to drop the argument by referring instead to the symbol itself at a fixed field name for the check?

I'm imagining something like:

@sentinel_attribute def union(cls): ... def is_instance(self, obj: type) -> bool: return obj._sentinel_attribute_type == self.__class__._sentinel_attribute_type

or something along those lines (depending on exactly what self/cls/metavariables exist in the context where is_instance is actually defined). Where self.__class__._sentinel_attribute_type == union

Thank you!!! Implemented!!!

Eric-Arellano · 2019-11-18T21:41:40Z

src/python/pants/util/meta.py

@@ -23,7 +24,7 @@ def __call__(cls, *args: Any, **kwargs: Any) -> Any:
    return cls.instance


-T = TypeVar("T")
+_T = TypeVar("_T")


Good change! The main reason I started making type vars private is to avoid autocomplete suggesting it in other files. Other files shouldn't really use this type var.

Eric-Arellano · 2019-11-18T21:42:11Z

src/python/pants/util/meta.py

+  """
+
+  @abstractmethod
+  def __call__(self, cls: type) -> type: ...


Technically should be typing.Type for the same reason it's typing.List instead of list

What is that reason?

To work with generics. You can't say list[int] or type[Foo], but you can say List[int] and Type[Foo].

However, PEP 585 is proposed to allow the former! https://www.python.org/dev/peps/pep-0585/ This is currently one of the most awkward parts of Python type hints.

Eric-Arellano · 2019-11-18T21:43:05Z

src/python/pants/util/meta.py

+  def define_instance_of(self, obj: type, **kwargs) -> type:
+    return type(obj.__name__, (obj,), {
+      '_sentinel_attribute_type': type(self),
+      **kwargs
+    })


MyPy is cool with this?

Yes!! I thought it was just returning a type -- is there something suspicious about it?

src/python/pants/util/meta.py

illicitonion

Looks great, thanks!

Now that it's not specifically relating to an attribute, I'm not 100% sure on the name... Something more declarative could be nice, but I don't have a good name... Something like @meta_type_checkable or @can_check_is_subclass or something... Except good?

tests/python/pants_test/util/test_meta.py

[ci skip-rust-tests] [ci skip-jvm-tests] ### Problem `@decorated_type_checkable` was added in #8496, and it makes checking for certain decorator types a little cleaner. ### Solution - Use `@decorated_type_checkable` for `@side_effecting`. - Update the error message to avoid referring to console rules.

cosmicexplorer requested review from stuhood, benjyw, pierrechevalier83, blorente and Eric-Arellano October 18, 2019 04:00

cosmicexplorer force-pushed the fully-type-engine-params branch from 6786898 to b6f239a Compare October 18, 2019 04:16

cosmicexplorer mentioned this pull request Oct 18, 2019

support using @dataclass for engine params #8181

Merged

cosmicexplorer force-pushed the fully-type-engine-params branch from b6f239a to 8942d22 Compare October 20, 2019 22:03

cosmicexplorer mentioned this pull request Oct 20, 2019

fix mypy type error in meta.py #8509

Merged

cosmicexplorer requested review from jsirois and illicitonion October 20, 2019 22:58

cosmicexplorer force-pushed the fully-type-engine-params branch from eabed7c to dec7f0c Compare October 21, 2019 16:50

cosmicexplorer force-pushed the fully-type-engine-params branch from dec7f0c to 0138140 Compare November 17, 2019 10:28

cosmicexplorer changed the title ~~Fully type-check engine Params~~ improve ergonomics of checking for @union types with new @sentinel_attribute decorator Nov 17, 2019

cosmicexplorer requested review from pierrechevalier83, Eric-Arellano, stuhood, benjyw and gshuflin and removed request for stuhood, jsirois, benjyw, illicitonion, pierrechevalier83, blorente and Eric-Arellano November 17, 2019 10:34

stuhood approved these changes Nov 17, 2019

View reviewed changes

src/python/pants/util/meta.py Outdated Show resolved Hide resolved

src/python/pants/util/meta.py Outdated Show resolved Hide resolved

illicitonion approved these changes Nov 18, 2019

View reviewed changes

Eric-Arellano reviewed Nov 18, 2019

View reviewed changes

illicitonion approved these changes Nov 19, 2019

View reviewed changes

cosmicexplorer force-pushed the fully-type-engine-params branch from f7847f1 to 6c0c8b3 Compare November 25, 2019 03:37

cosmicexplorer changed the title ~~improve ergonomics of checking for @union types with new @sentinel_attribute decorator~~ improve ergonomics of checking for @union types with new @decorated_type_checkable decorator Nov 25, 2019

cosmicexplorer force-pushed the fully-type-engine-params branch from 6c0c8b3 to 304fa92 Compare November 25, 2019 07:30

cosmicexplorer mentioned this pull request Nov 25, 2019

Revert "Revert dataclass engine params (#8540)" #8548

Closed

illicitonion approved these changes Nov 25, 2019

View reviewed changes

tests/python/pants_test/util/test_meta.py Show resolved Hide resolved

cosmicexplorer force-pushed the fully-type-engine-params branch from 304fa92 to 0b207a4 Compare December 24, 2019 19:45

cosmicexplorer added 9 commits December 24, 2019 15:52

add some more ergonomics to sentinel decorators

21c7460

add tests for @sentinel_attribute

15f8434

refactor implementation

3f64205

add docstrings and make mypy pass!

f3ad012

add BUILD file dep!

5c8edb4

respond to review comments and drastically simplify the implementation!

b4cc0c2

add capital letters to type annotations

03d55f9

rename @sentinel_attribute -> @decorated_type_checkable

38b64ab

update after rebase

481502f

cosmicexplorer force-pushed the fully-type-engine-params branch from 0b207a4 to 431c42a Compare December 24, 2019 20:59

remove @decorated_type_checkable from @frozen_after_init for now

abb2a59

cosmicexplorer force-pushed the fully-type-engine-params branch from 431c42a to abb2a59 Compare December 25, 2019 01:36

cosmicexplorer merged commit e500eb8 into pantsbuild:master Dec 25, 2019

cosmicexplorer mentioned this pull request May 15, 2020

use @decorated_type_checkable for side-effecting rules! #9780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve ergonomics of checking for @union types with new @decorated_type_checkable decorator #8496

improve ergonomics of checking for @union types with new @decorated_type_checkable decorator #8496

cosmicexplorer commented Oct 18, 2019 •

edited

Loading

stuhood commented Oct 22, 2019

Eric-Arellano commented Oct 22, 2019

cosmicexplorer commented Oct 25, 2019

illicitonion left a comment

illicitonion Nov 18, 2019 •

edited

Loading

cosmicexplorer Nov 19, 2019

Eric-Arellano Nov 18, 2019

Eric-Arellano Nov 18, 2019

cosmicexplorer Nov 19, 2019

Eric-Arellano Nov 19, 2019

Eric-Arellano Nov 18, 2019

cosmicexplorer Nov 19, 2019 •

edited

Loading

illicitonion left a comment

improve ergonomics of checking for @union types with new @decorated_type_checkable decorator #8496

improve ergonomics of checking for @union types with new @decorated_type_checkable decorator #8496

Conversation

cosmicexplorer commented Oct 18, 2019 • edited Loading

Problem

Solution

stuhood commented Oct 22, 2019

Eric-Arellano commented Oct 22, 2019

cosmicexplorer commented Oct 25, 2019

illicitonion left a comment

Choose a reason for hiding this comment

illicitonion Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

cosmicexplorer Nov 19, 2019

Choose a reason for hiding this comment

Eric-Arellano Nov 18, 2019

Choose a reason for hiding this comment

Eric-Arellano Nov 18, 2019

Choose a reason for hiding this comment

cosmicexplorer Nov 19, 2019

Choose a reason for hiding this comment

Eric-Arellano Nov 19, 2019

Choose a reason for hiding this comment

Eric-Arellano Nov 18, 2019

Choose a reason for hiding this comment

cosmicexplorer Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

cosmicexplorer commented Oct 18, 2019 •

edited

Loading

illicitonion Nov 18, 2019 •

edited

Loading

cosmicexplorer Nov 19, 2019 •

edited

Loading