dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.87k stars 779 forks source link

Mutable use binding compiles #5341

Open cartermp opened 6 years ago

cartermp commented 6 years ago
open System.Drawing

let f() =
    use mutable font = new Font("Arial", 10.0f)
    font <- new Font("Arial", 11.0f)

f()

The above code compiles. Note that in C# it is not:

screen shot 2018-07-14 at 11 46 15

Using latest Mono and FCS from Ionide

cartermp commented 6 years ago

Also, I returned out the font here:

open System.Drawing

let f() =
    use mutable font = new Font("Arial", 10.0f)
    font <- new Font("Arial", 11.0f)
    font

f()

Which hangs FSI indefinitely when the above code is executed.

If I do not make it mutable, it executes:

open System.Drawing

let f() =
    use font = new Font("Arial", 10.0f)
    //font <- new Font("Arial", 11.0f)
    font

f()

Result:

val f : unit -> Font
val it : Font =
  [Font: Name=Arial, Size=10, Units=3, GdiCharSet=1, GdiVerticalFont=False]

So we definitely allow code that cannot execute correctly.

vasily-kirichenko commented 6 years ago

It's definitely should not compile, but it's a breaking change.

matthid commented 6 years ago

On Windows this doesn't seem to "hang" and the "semantics" of use mutable seem to be: "Call dispose on the last reference". So I'd assume this "hang" to be a mono bug.

cartermp commented 6 years ago

Yep, looks like the hang is a mono issue, so adding a restriction on this would have to be a warning.

vasily-kirichenko commented 6 years ago

Actually, the right behaviour would be to call Dispose on the previously bind object just before assigning a new one (ideally, atomically), that's what Rust does:

#[derive(Debug)]
struct Foo(String);

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Dropping {:?}", self)
    }
}

fn main() {
    {
        let mut foo = Foo("1".to_string());
        foo = Foo("2".to_string());
        foo = Foo("3".to_string());
    }
    println!("Done")
}
Dropping Foo("1")
Dropping Foo("2")
Dropping Foo("3")
Done
realvictorprm commented 6 years ago

I'm with Vasily here but this would be a new language suggestion

matthid commented 6 years ago

Interestingly in the language spec 6.6.4 ‘use mutable’ doesn’t exist. So removing this thing would not be a breaking change in the spec...

matthid commented 6 years ago

would be a new language suggestion

Please don't let us assign any meaning to this abomination, let's introduce a compiler error (if allowed due to spec) or introduce a warning.

matthid commented 6 years ago

Seems like the compiler accepts pretty much everything with use it would with let:

module M =
    use internal d<'T> = { new System.IDisposable with member __.Dispose() = printfn "disposing 1, %O" typeof<'T>.Name }
    use rec d1 = { new System.IDisposable with member __.Dispose() = printfn "disposing 1" }
    and d2 = { new System.IDisposable with member __.Dispose() = printfn "disposing 2" }

At least we get:

stdin(82,5): warning FS0524: 'use' bindings are not permitted in modules and are treated as 'let' bindings
stdin(83,5): warning FS0524: 'use' bindings are not permitted in modules and are treated as 'let' bindings

But in principle the compiler seems to allow it (which is against the spec)

TIHan commented 6 years ago

I'm not sure what to think here. Any change to this would be a breaking change. This is somewhat similar to my proposal on disabling let mutable x : byref<int> suggestion.

cartermp commented 6 years ago

Agreed on this turning into a warning.