implement IDisposable interfaces explicitly

[ncave]

added some missing IDisposable interfaces

[forki]

Cool

[KevinRansom]

@ncave

How would this change actually be consumed? What code today needs this change, or what will this change enable?

Thanks

[ncave]

@KevinRansom Any code that obtains sequence enumerator and then tries to dispose it (e.g. with a use statement). This just fixes inconsistensies, most of the sequence enumerators already had the IDisposable interface, there were only a few that did not.

[KevinRansom]

with this change:

The generated enumerator doesn't really need collection. It is implemented as:

member __.Dispose () = () 

Dispose may be necessary if one were to generate a derived class with an enumeration over a COM collection. But since that would be special code it should add IDisposable to the derived class.

let main argv = 
    use x = [ for x in 0 .. 10 -> x * x]

    printf "%A" x

    0 // return an integer exit code


d:\ncave\visualfsharp\release\net40\bin\temp>..\fsc Program.fs
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

Program.fs(6,9): error FS0193: Type constraint mismatch. The type
    'int list'
is not compatible with type
    'System.IDisposable'

[ncave]

@KevinRansom use on sequence enumerators, not lists. Most of the sequence enumerators already have the IDisposable interface (here, here, here, etc.), this just fixes the ones where it was missed.

[dsyme]

@ncave Generated classes like this do all have the IDisposable interface, it's just not listed explicitly. It's implied by inheritance via the IEnumerator<T> interface. F# lets you implement inherited interfaces in the listing of methods for a sub-interface.

However it may be that Fable translation wants to see these as explicit interface implementations? I don't mind making this explicit.

The one change we can't take is that change to the public API surface of the DIspose helper function.

[dsyme]

@KevinRansom As long as this change is undone then AFAICS this is simply code cleanup, making interfaces that are already implemented implicitly more explicit.

[ncave]

@dsyme I'll take it out, but do you mind elaborating a bit, as this Dispose is only supposed to be called by compiled code? The change I made preserves existing functionality and fixes some use cases when code is generated to dispose things that don't implement IDisposable.

[KevinRansom]

Oh wow ... I didn't know that about IEnumerator and I think it's kind of silly, however, are we going to teach the compiler to work for:

let main argv = 
    use x = [ for x in 0 .. 10 -> x * x]

    printf "%A" x

    0 // return an integer exit code


d:\ncave\visualfsharp\release\net40\bin\temp>..\fsc Program.fs
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

Program.fs(6,9): error FS0193: Type constraint mismatch. The type
    'int list'
is not compatible with type
    'System.IDisposable'

[ncave]

@KevinRansom If we add IDisposable.Dispose somewhere here, then yes, why not.

[dsyme]

@dsyme I'll take it out, but do you mind elaborating a bit, as this Dispose is only supposed to be called by compiled code? The change I made preserves existing functionality and fixes some use cases when code is generated to dispose things that don't implement IDisposable.

Strictly speaking removing a generic constraint isn't a breaking change to the API of a component (other changes to method signatures would be). However this is a little subtle and I'd just rather avoid that change in what is otherwise a cleanup PR

[dsyme]

however, are we going to teach the compiler to work for..

To be clear, this PR doesn't do that. IDisposable is only implied by IEnumerator<T>

[ncave]

@dsyme @KevinRansom Ok, Dispose change reverted. It really does help alleviate some code generation issues on lesser platforms, so it's a nice to have, but yeah, I'll leave it at that. Thanks for reviewing!

[dsyme]

@dsyme @KevinRansom Ok, Dispose change reverted. It really does help alleviate some code generation issues on lesser platforms, so it's a nice to have, but yeah, I'll leave it at that. Thanks for reviewing!

@ncave does this change help Fable in some way, e.g. by making the IDisposable implementation explicit in the generated classes?

[ncave]

@dsyme I'm sure it will, in a way, in the sense that it makes all the sequence enumerators consistent (most of them already have the explicit IDispose). It's just a code cleanup, as you said, but it also helps platforms that can't handle non-explicit interfaces.

[dsyme]

... helps platforms that can't handle non-explicit interfaces.

So Fable can't handle non-explicit interfaces? We should add an implement-all-interfaces-explicitly option, is that already in Fable compiler?

[ncave]

@dsyme Sorry, perhaps I was unclear, wasn't talking about Fable but other projects like this.