Opened 7 years ago

Last modified 7 years ago

#16532 new defect

Weak morphism created with register_coercion can result in invalid coercion

Reported by: tscrim Owned by: tscrim
Priority: critical Milestone: sage-6.4
Component: coercion Keywords:
Cc: SimonKing, nbruin, jpflori, darij Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

First noted on #15475. Given UniqueRepresentation parents A, B and a coercion phi : A -> B that was constructed/registered in the __init__ of A by using register_coercion. Next suppose that B becomes garbage collected, and then so does phi. Then B is recreated. Expecting there to be a coercion map from A -> B, the user becomes very surprised when Sage responds with an errors saying there is no coercion map.

Right now I'm too tired to come up with a minimal example, but is this a case of misuse or should we have morphisms passed to register_coercion carry strong references? Other ideas?

Change History (5)

comment:1 Changed 7 years ago by nbruin

As far as I'm aware, (weak) coercion maps already hold a strong reference to their codomain. In fact, coercions are stored *on* their codomain, so this strong reference is just a cycle and hence doesn't prevent garbage collection.

It sounds like A->B should be registered as an *embedding*, in which case A would keep B alive (with all the memory-leaking consequences that might have). The same effect can be obtained by simply letting A hold a reference to B. If there has to be a coercion between A and B, but no lifetime implication, then you must make the coercion discoverable instead of "manually" putting it in place. Then a "fresh" copy of B will happily discover the coercion again.

As a general rule, it is not the job of the coercion system to ensure lifetime implications. If you want those, enforce them yourself (e.g., by letting A hold a reference to B). The reason to not put that task on the coercion system is because it turns out different situations need different lifetime implications, and the coercion system doesn't have enough info to do the right thing. So the most flexible setup arises from trying to let the coercion system have minimal implications. Even in the current setup we leak.

Last edited 7 years ago by nbruin (previous) (diff)

comment:2 in reply to: ↑ description Changed 7 years ago by SimonKing

Replying to tscrim:

First noted on #15475. Given UniqueRepresentation parents A, B and a coercion phi : A -> B that was constructed/registered in the __init__ of A by using register_coercion.

Is this a typo? Or is it really the case that a coerce map into B is registered during creation of A by calling register_coercion on B? This would be a misuse.

Next suppose that B becomes garbage collected, and then so does phi.

That's the expected/preferred behaviour, since otherwise we'd have memory leaks.

Then B is recreated. Expecting there to be a coercion map from A -> B, the user becomes very surprised when Sage responds with an errors saying there is no coercion map.

That's why the coerce map from A to B should either be registered during creation of B using register_coercion, or (as Nils pointed out) should be registered using register_embedding during creation of A, which assumes that A embeds into B and may be done only once.

Right now I'm too tired to come up with a minimal example, but is this a case of misuse

Misuse.

Last edited 7 years ago by SimonKing (previous) (diff)

comment:3 Changed 7 years ago by SimonKing

Or, if calling register_coercion during creation of B rather than A is no option, you should/could rely on _coerce_map_from_ (on B, of course).

comment:4 Changed 7 years ago by tscrim

Okay, then we need to get back on #15303 as setting up _coerce_map_from_ can be somewhat painful for things with multiple realizations (ex. symmetric functions). Also, do you think it's worthwhile documenting this somewhere and if so, where? The coercion thematic tutorial(s) or somewhere in coerce.pyx?

comment:5 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.