Possible bugs with effects applying to phased out permanents

[xenohedron]

As noticed in https://github.com/magefree/mage/commit/5c9d1cd205fd8df1c38d4b21c0edbba474de52d6#r140928934 , phasing logic may not be checked everywhere it needs to be.

A few reported bugs over the years: https://github.com/magefree/mage/issues?q=is%3Aissue+%28phasing+OR+phased%29 and some associated fixes that check isPhasedIn() for a particular effect. It ought to be getting handled deeper in the engine. There are also a few checks to isPhasedOutIndirectly() which I think are superfluous as that should only be for internal use.

Example: #8213 was fixed with the following: https://github.com/magefree/mage/blob/f75b1c9f0aa8f17c6df4e2570c39739a9a2691b7/Mage/src/main/java/mage/abilities/effects/common/ReturnToHandSourceEffect.java#L73-L77

But there are many other effects that could change zone. So if it is needed it should be deeper, perhaps all the way down in ZonesHandler ?

Another example: #8549 was fixed by adding a check to ClassLevelAbility, though I think it might be more appropriate in PermanentImpl::setClassLevel, similar to #9194. This is the only usage though so not as big a deal. But still, we don't want to have to check phased in status for all these methods in Permanent.

Next thought, for usages of source.getSourcePermanentIfItStillExists(game), should this return null if phased out? After all, a phased out permanent is treated as though it doesn't exist.

And then even for getPermanent, are there legitimate use cases to find a phased-out permanent?

If only a few specific known cases need to find phased-out permanents, then special method could be created for those, and the general case should not find phased-out permanents. This would eliminate the need for other methods to check phased in status.

Any associated rework needs to ensure that previously solved bugs have test coverage.

[alexander-novo]

The rules have this to say:

Except for rules and effects that specifically mention phased-out permanents, a phased-out permanent is treated as though it does not exist

So I think phased out permanents should be remove from the battlefield and added to a separate collection of phased out permanents. That way any effect that doesn’t mention phased out permanents will correctly treat them as though they don’t exist - and then any effects which do mention it can correctly check that collection if needed.

We can’t expect every effect to respect a flag - that’ll just lead to more eventual bugs.

[JayDi85]

Battlefield class looks perspective -- it has only few methods without phased out check -- so that's methods can be modified to filter phased-in only like other methods do. I don't know is it require to modify card zones methods -- some effects can look at cards on battlefield (it's weird use case and must be research):

What must be tested after fix:

• is permanent and all attachments keep after phased-out and phased-in;
• is it removed from a game owner or returned control to the owner after controller's leave a game (leaving game is buggy use case for xmage);
• is it keep controlling info after phased-out and phased-in.

[xenohedron]

Main potential problem:

• OneShotEffect logic must not do anything to phased out permanent, and simplest is for phased out permanent to return null
• ContinuousEffect logic must not discard the effect because if the permanent phases in, it should still be active, but many effects discard() on getPermanent() returning null