pants icon indicating copy to clipboard operation
pants copied to clipboard

Support overriding param types for rule code

Open thejcannon opened this issue 1 year ago • 10 comments

The main use-case is runtime-typed parameters (for use in "helper"/"inner" rules).


def make_rule(sometype: type[BaseType]):
    @rule(param_type_overrides={"request": sometype.var_type})
    async def helper_rule(request: WhateverVarTypeShouldBe) -> str:
        ...

    return helper_rule

[ci skip-rust] [ci skip-build-wheels]

thejcannon avatar Sep 20 '22 17:09 thejcannon

Will wait for another maintainers review since this is a "wide" change

thejcannon avatar Sep 20 '22 19:09 thejcannon

@kaos by "value" do you mean the benefit, or the concrete value type of the mapping?

thejcannon avatar Sep 20 '22 21:09 thejcannon

@kaos by "value" do you mean the benefit, or the concrete value type of the mapping?

the concrete type I think. Also I'm not sure I've come across the need for the benefit, but my mind is crazy enough to appreciate the flexibility up-front and find the use for it later :P (I can imagine there's one when you're trying to implement PluginOption and the like)

kaos avatar Sep 20 '22 23:09 kaos

I can give two examples:

  1. Most lint partitioners look like this. I want to provide a generic "rule_maker" for that however the Subsystem param can't be known statically and is only known at runtime. So I'd use @rule(param_type_overrides({"subsystem": cls.subsystem_type}) def default_rule_impl(request: ..., subsystem: BaseSubsystemType): ...
  2. See https://github.com/pantsbuild/pants/pull/16412#discussion_r973514403

thejcannon avatar Sep 21 '22 00:09 thejcannon

  1. Most lint partitioners look like this. I want to provide a generic "rule_maker" for that however the Subsystem param can't be known statically and is only known at runtime. So I'd use @rule(param_type_overrides({"subsystem": cls.subsystem_type}) def default_rule_impl(request: ..., subsystem: BaseSubsystemType): ...

Is this effectively using a @union type in an argument position? You'd declare the @rule once with a @union as a positional argument, and we'd install a copy of the @rule per @union member?

stuhood avatar Sep 21 '22 16:09 stuhood

  1. Most lint partitioners look like this. I want to provide a generic "rule_maker" for that however the Subsystem param can't be known statically and is only known at runtime. So I'd use @rule(param_type_overrides({"subsystem": cls.subsystem_type}) def default_rule_impl(request: ..., subsystem: BaseSubsystemType): ...

Is this effectively using a @union type in an argument position? You'd declare the @rule once with a @union as a positional argument, and we'd install a copy of the @rule per @union member?

Bingo! Although it'd be a subset of @union members, as requested by plugin authors.

thejcannon avatar Sep 21 '22 17:09 thejcannon

Bingo! Although it'd be a subset of @union members, as requested by plugin authors.

It could be a separate @union probably: LinterWhichUsesADefaultPartitioner?

Is that worth sketching out as an alternative? It should be easy to implement: similar to this line (which installs a Get per @union member), if we found a @union in the positional arguments a few lines up, we'd install multiple copies of the @rule.

stuhood avatar Sep 21 '22 17:09 stuhood

I think this is much more general and likely easier to grok? See also use-case 2.

thejcannon avatar Sep 21 '22 17:09 thejcannon

See also use-case 2.

Use case 2 also appears to be a @union use case, but maybe I'm misreading it.

I think this is much more general and likely easier to grok?

Possibly... note that this is only actually type safe if the substituted parameter is a subclass of the actual declared type. It might be worth validating that?

stuhood avatar Sep 21 '22 17:09 stuhood

Possibly... note that this is only actually type safe if the substituted parameter is a subclass of the actual declared type. It might be worth validating that?

That's definitely the sane way to think about it. However, much like @unions themselves, we just make assumptions about the member types and hope that our @union's (protocol) "interface" is implemented by the members.

It's not any more-or-less typesafe than, say:

if TYPE_CHECKING:
    MyType = int
else:
    MyType = str

@rule
async def (request: MyType): ...

I'm not against it, but we're forcing our hand on some inheritance (which https://github.com/pantsbuild/pants/pull/16926 is particularly relevant as it will be the first "type" we do this substitution on)

thejcannon avatar Sep 21 '22 17:09 thejcannon

@stuhood would you prefer I prefix with underscore so we can explore it further in Pants repo (and feel comfortable removing it possibly later?)

thejcannon avatar Sep 22 '22 18:09 thejcannon