Ensure Disposable.NULL always returns a new Disposable

[colin-grant-work]

What it does

Because of the way DisposableCollections handle disposal of their members, it is unsafe to add Disposable.NULL to any disposable collection: doing so risks disposal of one collection marking another collection as disposed inappropriately. This PR adds a test showing the risk of Disposable.NULL and replaces Disposable.NULL throughout the framework with a new method Disposable.createNull, with a test to demonstrate that disposables created in that way are safe for use in DisposableCollections.

How to test

1. Assert that the tests in disposable.spec.ts pass (and look correct)
2. Check that changes other than disposable.ts and dispospable.spec.ts just replace Disposable.NULL with Disposable.createNull
3. Check that no references to Disposable.NULL remain in the framework.
4. Check that the application behaves as expected.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

Is this related to the fact that Disposable.NULL always returns the same disposable instance, and DisposableCollection is messing with the reference of disposables added to it?

[paul-marechal]

I honestly dislike what's being done to disposable references in DisposableCollection: it mutates instances with a new dispose function, and then tries to revert it somehow. Instead we should consider the instances as managed by the collection and assume dispose will only be called once by the collection and no one else.

Anyway, I would propose two alternative changes:

1. Make Disposable.NULL return new instances on each access: see playground.
2. Make DisposableCollection aware of Disposable.NULL and ignore it when pushed to it.

Those alternatives both have the benefit of requiring no changes for downstream projects. WDYT?

[colin-grant-work]

Is this related to the fact that Disposable.NULL always returns the same disposable instance, and DisposableCollection is messing with the reference of disposables added to it?

Yes, exactly.

I honestly dislike what's being done to disposable references in DisposableCollection.

Yeah, it's certainly not ideal. I'm not exactly sure what system to put in its place: if the point of disposables is to avoid leaks, then having some mechanism for removing references to them that occur outside of the collection currently disposing of itself is probably correct - otherwise they're leaking - but it probably doesn't make sense to have two different collections that can both dispose of something the holder of the other collection might be depending on. So it would probably make sense to guarantee that no disposable is ever put in two disposable collections, but I'm not sure how frequently that's happening in the wild or how much fixing it would take to ensure it doesn't happen.

Anyway, I would propose two alternative changes:

1. Make Disposable.NULL return new instances on each access: see playground.
2. Make DisposableCollection aware of Disposable.NULL and ignore it when pushed to it.

Those alternatives both have the benefit of requiring no changes for downstream projects. WDYT?

I'm in favor of 1, though in general I'm not very fond of manual prototype manipulation. But as you say, it would avoid having adopters need to do anything.

[paul-marechal]

So it would probably make sense to guarantee that no disposable is ever put in two disposable collections [...]

I don't see how to enforce that nicely, I have better hope with being careful with our references and not sharing them to multiple dependants that all will try to dispose the same instance. To me it sounds like a "double free" kind of issue. Where is Rust's borrow checker when we need it? :)

Let's go with my first proposed change, I don't like it either but you can add a comment mentioning how each access will return a new disposable instance (the jsdoc should go in the declared namespace?).

[paul-marechal]

LGTM