Skip to content

Support typing.Self#613

Open
eb8680 wants to merge 10 commits into
masterfrom
eb-self-type
Open

Support typing.Self#613
eb8680 wants to merge 10 commits into
masterfrom
eb-self-type

Conversation

@eb8680
Copy link
Copy Markdown
Contributor

@eb8680 eb8680 commented Mar 13, 2026 •
edited
Loading

Addresses #602

@eb8680 eb8680 linked an issue Mar 13, 2026 that may be closed by this pull request
Type inference should dynamically resolve typing.Self #602
Open
@eb8680 eb8680 marked this pull request as ready for review May 6, 2026 17:14
@jfeser jfeser mentioned this pull request May 27, 2026
Open
@jfeser jfeser self-requested a review May 27, 2026 13:49
datvo06 added a commit that referenced this pull request May 27, 2026
@datvo06
Trim PR scope down to the annotation flip, per jfeser
652c881 
Revert the ``_DistributionMethodTerm`` fallback class, the recursive
``_is_eager``, and the ``_pos_base_dist`` override. jfeser pointed out
that the right fix is ``typing.Self`` resolution (#613), after which
``Normal(mu()).expand(s)`` routes through the existing concrete
``NormalTerm`` directly — no abstract-base fallback needed. Drop the
workaround; come back with the principled fix once #613 lands.

What remains in this PR:

- The ``_DistributionTerm.expand`` return-annotation flip
  (``jax.Array`` → ``dist.Distribution``). Stops the result from being
  routed through ``_ArrayTerm``; necessary precursor to the Self-based
  routing.
- One ``xfail(strict=True)``-marked regression test pinning the desired
  chain-construction behaviour. Flips to PASS once #613 + the
  follow-up land.

Also restore ``xfail="to_event not implemented"`` on the parametrised
``Beta(...).to_event(k)`` / ``Dirichlet(...).to_event(k)`` cases —
those pass through the same routing hole and need the same follow-up.
datvo06 added a commit that referenced this pull request May 27, 2026
@datvo06
Merge #613 + annotate _DistributionTerm.expand with typing.Self
106ab34 
Brings in the ``typing.Self`` resolution machinery from #613 (eb-self-type)
and annotates ``expand``'s ``self`` / return as ``Self`` per @jfeser's
suggestion — the goal being that ``Normal(mu()).expand(s)`` routes through
the concrete ``NormalTerm`` registration via ``Self → dist.Normal``.

Status: not yet sufficient on its own. ``unify`` (post-#613) widens
subclasses to parent classes:

    >>> unify(TypeVar('T'), Bar)              # Bar extends Foo
    {T: Foo}
    >>> unify(TypeVar('T'), dist.Normal)
    {T: dist.Distribution}

So even with the correct ``Self`` annotation, dispatch on the unified
return type lands on ``dist.Distribution`` (no registration) and falls
through to ``_CallableTerm``. The xfail regression test from the prior
commit therefore still xfails. Surfacing the unify-widening behaviour
for jfeser to address before this can land green.
@datvo06
Copy link
Copy Markdown
Contributor

datvo06 commented May 27, 2026

Have a question previously raised in #667 but I think worth checking again here. When we unify a type var with an inherited class, we are widening to the ancestor:

import typing                                                                                                                                                                                                                                                       
from effectful.internals.unification import unify                                                                                                                                                                                                                   
  
T = typing.TypeVar("T")                                                                                                                                                                                                                                             
                                                                                                                                                                                                                                                                    
class Foo: pass
class Bar(Foo): pass

assert unify(T, Bar) == {T: Bar}      # FAILS: gets {T: Foo}

Is it intended? I think by standard the principal type would be Bar instead of Foo?

@eb8680
Copy link
Copy Markdown
Contributor Author

eb8680 commented May 27, 2026

Is it intended? I think by standard the principal type would be Bar instead of Foo?

unify works up to the normal-form equivalence relation defined by canonicalize, which may attempt to widen types it does not know how to work with directly:

assert unify(T, Bar) == unify(canonicalize(T), canonicalize(Bar)) == {T: canonicalize(Bar)}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jfeser jfeser Awaiting requested review from jfeser

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Type inference should dynamically resolve typing.Self

2 participants

@eb8680 @datvo06