Fix lazy magic lookup resolving the wrong provider per kind by bunnysayzz · Pull Request #15387 · ipython/ipython · GitHub

Fixes #15383.

When one magic name is lazily declared with a different provider per kind, load_lazy() always preferred the line-table placeholder, so %%name? loaded the wrong provider and left a stale cell placeholder behind. On retry the stale placeholder was dropped, which made the outcome depend on registration order: line-then-cell worked on the second attempt, cell-then-line never worked at all.

This passes the requested kind from find() into load_lazy(), so each kind loads its own provider on the first lookup. It also remembers a displaced lazy declaration per (kind, name): if the newer declaration never delivers the magic, the previous one is restored for one retry instead of the name going unresolvable. That last part covers the time case in the issue, where a line-only override kept working for %time/time? while %%time? and %%time fell back to IPython's own cell magic again.

Tests added in tests/test_magic.py (test_lazy_magic_help_finds_both_kinds_first_try, parametrized over both registration orders, and test_lazy_magic_falls_back_to_previous_declaration). All three fail without the fix and pass with it. Full test_magic.py + test_magic_table.py green (149 passed, 5 skipped); the 5 failures in test_interactiveshell.py/test_prefilter.py are pre-existing environment failures, identical on clean main.

@Darshan808 would appreciate your review when you have a moment.

…isplaced declarations When one name is lazily declared with a different provider per kind, load_lazy() always preferred the line placeholder, so %%name? loaded the wrong provider, left a stale cell placeholder, and failed. On retry the stale placeholder was dropped, which made the outcome depend on registration order: line-then-cell worked on second attempt, cell-then-line never worked. Pass the requested kind from find() into load_lazy() so each kind loads its own provider on first lookup. Also, when a newer declaration displaces an older one for a kind but never delivers it, restore the previous declaration for one retry instead of dropping the name, so e.g. a line-only override of time keeps the builtin cell time working. Fixes ipython#15383

Thanks @bunnysayzz for working on this. Left few comments.

One thing though: _lazy_fallbacks only remembers one level, so it breaks as soon as a name gets declared twice:

@magics_class
class MyMagics(Magics):
    @line_magic
    def time(self, line):
        """My own %time."""

@magics_class
class MyAnotherMagics(Magics):
    @line_magic
    def time(self, line):
        """MyAnother own %time."""

sys.modules["provider"] = provider = types.ModuleType("provider")
provider.MyMagics, provider.MyAnotherMagics = MyMagics, MyAnotherMagics

ip.magics_manager.register_lazy("time", "provider:MyMagics")
ip.magics_manager.register_lazy("time", "provider:MyAnotherMagics")

%time?    # MyAnother own %time.  ✅
%%time?   # nothing — built-in %%time is gone ❌

The second register_lazy overwrites _lazy_fallbacks[("cell", "time")] with provider:MyMagics, so the built-in's spec is lost. find then restores MyMagics, which is also line-only, doesn't deliver a cell magic either, and hits the del, so %%time is destroyed for the session. Same symptom as the original case C in the issue, just one declaration later.

Good catch here!

Well, storing the displaced entry on the LazyMagic that displaced it instead of in a side table gives you arbitrary depth for free, and it can't get out of sync with the magics table.

Consider this:

# LazyMagic.__init__
self.shadowed = shadowed

# register_lazy
self.magics[kind][name] = LazyMagic(self, fully_qualified_name, kind, name, shadowed=existing)

# find — instead of the single retry + del
while isinstance(fn, LazyMagic):
    self.load_lazy(magic_name, magic_kind)
    if table.get(magic_name) is not fn:
        fn = table.get(magic_name)
        continue
    fn = fn.shadowed
    if fn is None:
        del table[magic_name]
    else:
        table[magic_name] = fn

Thanks, both points taken. Reworked per your sketch: the displaced entry now rides on the placeholder itself (shadowed), and find swaps each undispatched placeholder into the table and retries, so the chain unwinds to whatever declaration actually delivers, at any depth. Side table is gone.

Added test_lazy_magic_falls_back_through_two_declarations (two newest declarations deliver nothing, oldest resolves) — I verified it fails on the old single-level code and passes now. Full test_magic.py + test_magic_table.py still 150 passed, and the reformatted lines match the formatter.

Good catch, same root cause one layer over. register_lazy overwrites lazy_magics[name] per call, and load_all then fed that single spec through kind-blind load_lazy, so only the line half's class ever imported. Now load_all_lazy_magics walks the placeholders per kind first (extensions still skipped, same as before) and keeps the old loop as fallback. Added test_load_all_lazy_magics_loads_both_kind_halves; verified it plus the other four new tests fail on clean main and pass here. Suite still 151 green.

Suggested change

      
                            mm.register_lazy("dual", line_spec, "line")  # type: ignore[arg-type]
          
                            mm.register_lazy("dual", line_spec, "line")

Suggested change

      
                            mm.register_lazy("dual", first[1], first[0])  # type: ignore[arg-type]
          
                            mm.register_lazy("dual", first[1], first[0])

Suggested change

      
                            mm.register_lazy("dual", second[1], second[0])  # type: ignore[arg-type]
          
                            mm.register_lazy("dual", second[1], second[0])

Suggested change

      
                            mm.register_lazy("dual", cell_spec, "cell")  # type: ignore[arg-type]
          
                            mm.register_lazy("dual", cell_spec, "cell")

Calling load_all_lazy_magics() on the shared session shell may break later tests.
Lets build a standalone MagicsManager(shell=...) for this test instead of using the session ip. There's a manager fixture in test_magic_table.py

One suggestion: the two loops here are deriving "everything still declared lazily" from the two stores inline, and _MagicsRegistry.__missing__ needs the same thing. Worth pulling it into a helper?

def _lazy_declarations(self) -> list[tuple[str, _MagicKind | None, str]]:
    """Every live lazy declaration, as ``(name, kind, spec)``.

    The placeholders in :attr:`magics` are the source of truth, because
    :attr:`lazy_magics` is keyed by name alone and so keeps only the last
    spec for a name declared once per kind. A name declared only through
    the trait has no placeholder, and so no known kind.
    """
    seen: set[tuple[str, str]] = set()
    declarations: list[tuple[str, _MagicKind | None, str]] = []
    for kind in magic_kinds:
        for name, fn in list(self.magics[kind].items()):
            if isinstance(fn, LazyMagic) and (name, fn.spec) not in seen:
                seen.add((name, fn.spec))
                declarations.append((name, kind, fn.spec))
    for name, spec in list(self.lazy_magics.items()):
        if (name, spec) not in seen:
            seen.add((name, spec))
            declarations.append((name, None, spec))
    return declarations

Then this method is just a filter:

def load_all_lazy_magics(self) -> None:
    for magic_name, magic_kind, spec in self._lazy_declarations():
        if ":" in spec:
            self.load_lazy(magic_name, magic_kind)

__missing__ with the helper:

def __missing__(self, key: s
    for magic_name, magic_kind, spec in self._manager._lazy_declarations():
        if spec.endswith(":"
            self._manager.load_lazy(magic_name, magic_kind)
            # A matching spelared once per kind
            # has one spec per kind, so keep going until `key` shows up.
            if key in self:
                break
    if key not in self:
        raise KeyError(key)
    return self[key]

… test

Round 2 done, thanks for the detailed review:

Helper: added MagicsManager._lazy_declarations() essentially as you sketched it, and load_all_lazy_magics is now just the filter over it. _MagicsRegistry.__missing__ uses it too: it keeps loading matching specs until key shows up instead of stopping after the first trait hit.
Standalone manager: the load_all test now builds its own MagicsManager(shell=...) via a local fixture (same shape as the one in test_magic_table.py), so the shared session shell is untouched and the try/finally cleanup shrank to just sys.modules.
Arg-order suggestions: I double-checked these against the signature (name, fully_qualified_name, magic_kind) and the calls already pass (dual, spec, kind) — the tuples are (line, line_spec) etc., indexed [1], [0]. E.g. line 2023 unpacks to (dual, line_spec, line), which is what the suggestion shows. So I left those four lines as-is; if you were seeing a different version, let me know.

All green locally: test_magic.py + test_magic_table.py, 153 passed, 3 skipped. I also re-verified the load_all test still fails if the helper is neutered back to trait-only iteration, so it still guards the original bug.

You likely want to create docstring for __init__ and start documenting the parameter instead of comments.

And that seem like overkill, if someone called register_magic with the wrong kind, and it does not "deliver" (A lot of that vocabulary seem a lot like opus 5) then I think it is fair to error or do the wrong things than to try to walk to the next declaration (unless I missunderstand)

I think this solution is for a particular issue that @krassowski described in the issue thread.

@magics_class
class MyMagics(Magics):
    @line_magic
    def time(self, line):
        """My own %time."""


sys.modules["provider"] = provider = types.ModuleType("provider")
provider.MyMagics = MyMagics

ip = InteractiveShell()
ip.magics_manager.register_lazy("time", "provider:MyMagics")

ip.run_cell("time?")            # my own %time
ip.run_cell("%%time?")          # expected: IPython's %%time docs
ip.run_cell("%%time\npass")     # expected: a wall time report

if someone called register_magic with the wrong kind, and it does not "deliver"

Yes a direct solution would be to just use:

ip.magics_manager.register_lazy("time", "provider:MyMagics", "line")

but by default it is "line_cell" and overrides both %time and %%time without checking if it provides both implementation (and I don't think we can validate since the magic is registered lazily and the provider module has not been imported yet), which is causing the above failure.

I think it is fair to error or do the wrong things than to try to walk to the next declaration

I also agree with this, since it is a mistake on the side of the magic registration too. Maybe we could look into improving the error message. I'll see.

Thanks for looking, both points taken seriously:

Docstring over inline comment: agreed, I'll move the shadowed explanation into an __init__ docstring documenting the parameter. (Also noted on the vocabulary, I'll use plainer words.)

Is the walk overkill? Let me make sure the case it covers is worth it before I rip it out. The concrete scenario is the time test: IPython itself declares time lazily for both kinds. A user then registers their own line-only time, which displaces both placeholders. When something looks up %%time, the new provider loads fine but delivers no cell magic. Without the walk, the cell lookup ends with the name resolving to nothing, so %%time breaks entirely as a side effect of overriding %time. The walk restores the previously displaced declaration in that case, so %%time keeps working.

If you'd rather that cell lookup raise (or just resolve to whatever the new provider gave) instead of falling back, say the word and I'll simplify it down to that. Your call.

I applied the documentation-only part of your first note in commit fda3b63f1: the LazyMagic.__init__ docstring now documents shadowed, and the old explanatory inline comment is gone. I left the fallback behavior unchanged while waiting for your answer on whether the displaced-entry walk should be removed. The magic test suites remain green: 153 passed, 3 skipped.

bunnysayzz mentioned this pull request Sep 8, 2026

Custom lazy magics do not show help (?) until they are imported #15383

Open

Format the displaced-declaration guard per darker

6daf42e

Darshan808 suggested changes Sep 9, 2026

View reviewed changes

Darshan808 added the bug label Sep 9, 2026

Rework displaced-lazy fallback as a shadow chain on the placeholder

255e4b3

Load every kind's lazy provider in load_all_lazy_magics

07b6aa0

Darshan808 suggested changes Sep 9, 2026

View reviewed changes

Address review round 2: _lazy_declarations helper, standalone manager…

34571f7

… test

Carreau reviewed Sep 11, 2026

View reviewed changes

docs(magic): document displaced lazy entries

fda3b63

+                              # A newer declaration displaces an older one for this kind;
+                              # remember the old spec so it can be restored if the new
+                              # declaration never delivers the magic.
+                              self._lazy_fallbacks[(kind, name)] = existing.spec

                       # wholesale, so prefer the spec the placeholder carries.
-                      fn = self.magics["line"].get(magic_name) or self.magics["cell"].get(magic_name)
+                      if magic_kind is not None:
+                          fn = self.magics[magic_kind].get(magic_name)

+                              # Declared but not delivered. If an older declaration for
+                              # this kind was displaced, restore it and give it one
+                              # chance rather than dropping the name entirely.
+                              fallback = self._lazy_fallbacks.pop((magic_kind, magic_name), None)
+                              if fallback is not None and fallback != fn.spec:
+                                  self.magics[magic_kind][magic_name] = LazyMagic(
+                                      self, fallback, magic_kind, magic_name
+                                  )
+                                  self.load_lazy(magic_name, magic_kind)
+                                  fn = self.magics[magic_kind].get(magic_name)
+                              if isinstance(fn, LazyMagic):
+                                  # Still nothing delivered; drop the stale placeholder.
+                                  del self.magics[magic_kind][magic_name]
+                                  fn = None

+                          line_spec = f"{mod}:LineHalf"
+                          cell_spec = f"{mod}:CellHalf"
+                          try:
+                              mm.register_lazy("dual", line_spec, "line")  # type: ignore[arg-type]

+                      # Load per kind from the placeholders themselves: one name may be
+                      # declared with a different provider per kind, and ``lazy_magics``
+                      # only remembers the last spec per name. Extensions (specs without a
+                      # ":") are still skipped: importing one can run arbitrary code.
+                      for kind in magic_kinds:
+                          for magic_name in list(self.magics[kind]):
+                              fn = self.magics[kind].get(magic_name)
+                              if isinstance(fn, LazyMagic) and ":" in fn.spec:
+                                  self.load_lazy(magic_name, kind)

	mm.register_lazy("dual", line_spec, "line") # type: ignore[arg-type]
	mm.register_lazy("dual", line_spec, "line")

	mm.register_lazy("dual", first[1], first[0]) # type: ignore[arg-type]
	mm.register_lazy("dual", first[1], first[0])

	mm.register_lazy("dual", second[1], second[0]) # type: ignore[arg-type]
	mm.register_lazy("dual", second[1], second[0])

	mm.register_lazy("dual", cell_spec, "cell") # type: ignore[arg-type]
	mm.register_lazy("dual", cell_spec, "cell")

+                      # The table entry this declaration displaced, if any. A name may be
+                      # declared lazily any number of times; the chain is walked if a
+                      # newer declaration never delivers the magic.
+                      self.shadowed = shadowed

Fix lazy magic lookup resolving the wrong provider per kind by bunnysayzz · Pull Request #15387 · ipython/ipython · GitHub

Uh oh!

Uh oh!

Fix lazy magic lookup resolving the wrong provider per kind - #15387

Fix lazy magic lookup resolving the wrong provider per kind#15387bunnysayzz wants to merge 6 commits intoipython:mainipython/ipython:mainfrom bunnysayzz:fix/lazy-magic-help-both-kinds-15383bunnysayzz/ipython:fix/lazy-magic-help-both-kinds-15383Copy head branch name to clipboard

Conversation

bunnysayzz commented Sep 8, 2026

Uh oh!

Darshan808 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 9, 2026

Uh oh!

bunnysayzz commented Sep 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bunnysayzz commented Sep 11, 2026

Uh oh!

bunnysayzz commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix lazy magic lookup resolving the wrong provider per kind#15387
bunnysayzz wants to merge 6 commits into
ipython:mainfrom
bunnysayzz:fix/lazy-magic-help-both-kinds-15383