Fix double Dispose in use bindings with as patterns (#12300)

docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@@ -1,5 +1,6 @@
 ### Fixed
 
+* Fix double `Dispose` call when a `use` binding aliases its value via an `as` pattern or rebinds an existing `use`-bound value. ([Issue #12300](https://github.com/dotnet/fsharp/issues/12300), [PR #19858](https://github.com/dotnet/fsharp/pull/19858))
 * Honor `--nowarn` and `--warnaserror` for warnings emitted during command-line option parsing ([Issue #19576](https://github.com/dotnet/fsharp/issues/19576), [PR #19776](https://github.com/dotnet/fsharp/pull/19776))
 * Fix `[<return: X>]` prefix attributes being silently dropped on class members, and fix false-positive `AllowMultiple=false` errors when `[<X>]` and `[<return: X>]` are applied to the same binding. ([Issue #17904](https://github.com/dotnet/fsharp/issues/17904), [Issue #19020](https://github.com/dotnet/fsharp/issues/19020), [PR #19738](https://github.com/dotnet/fsharp/pull/19738))
 * Fix attributes on return type of unparenthesized tuple methods being silently dropped from IL. ([Issue #462](https://github.com/dotnet/fsharp/issues/462), [PR #19714](https://github.com/dotnet/fsharp/pull/19714))

src/Compiler/Checking/CheckBasics.fs

@@ -240,6 +240,9 @@ type TcEnv =
       // In order to avoid checking implicit-yield expressions multiple times, we cache the resulting checked expressions.
       // This avoids exponential behavior in the type checker when nesting implicit-yield expressions.
       eCachedImplicitYieldExpressions : HashMultiMap<range, SynExpr * TType * Expr>
+
+      /// See .fsi.
+      eUseBoundValStamps: Set<Stamp>
     }
 
     member tenv.DisplayEnv = tenv.eNameResEnv.DisplayEnv

src/Compiler/Checking/CheckBasics.fsi

@@ -133,6 +133,11 @@ type TcEnv =
         // In order to avoid checking implicit-yield expressions multiple times, we cache the resulting checked expressions.
         // This avoids exponential behavior in the type checker when nesting implicit-yield expressions.
         eCachedImplicitYieldExpressions: HashMultiMap<range, SynExpr * TType * Expr>
+
+        /// Stamps of local values introduced by `use` bindings currently in scope.
+        /// Used to suppress duplicate Dispose calls when a `use` binding's
+        /// right-hand side is a reference to an already-use-bound value (issue #12300).
+        eUseBoundValStamps: Set<Stamp>
     }
 
     member DisplayEnv: DisplayEnv

src/Compiler/Checking/CheckDeclarations.fs

@@ -5654,7 +5654,8 @@ let emptyTcEnv g =
       eCallerMemberName = None 
       eLambdaArgInfos = []
       eIsControlFlow = false
-      eCachedImplicitYieldExpressions = HashMultiMap(HashIdentity.Structural, useConcurrentDictionary = true) }
+      eCachedImplicitYieldExpressions = HashMultiMap(HashIdentity.Structural, useConcurrentDictionary = true)
+      eUseBoundValStamps = Set.empty }
 
 let CreateInitialTcEnv(g, amap, scopem, assemblyName, ccus) =
     (emptyTcEnv g, ccus) ||> List.collectFold (fun env (ccu, autoOpens, internalsVisible) -> 

src/Compiler/Checking/Expressions/CheckExpressions.fs

@@ -11951,17 +11951,47 @@ and TcLetBinding (cenv: cenv) isUse env containerInfo declKind tpenv (synBinds,
         // Add the dispose of any "use x = ..." to bodyExpr
         let mkCleanup (bodyExpr, bodyExprTy) =
             if isUse && not isFixed then
-                let isDiscarded = match checkedPat2 with TPat_wild _ -> true | _ -> false
-                let allValsDefinedByPattern = if isDiscarded then [patternInputTmp] else allValsDefinedByPattern
-                (allValsDefinedByPattern, (bodyExpr, bodyExprTy)) ||> List.foldBack (fun v (bodyExpr, bodyExprTy) ->
+                // Issue #12300, scenario B: `use b = a` where `a` is itself use-bound
+                // would dispose the same backing object twice. Skip cleanup here; the
+                // enclosing `use` will dispose.
+                let rec stripTyLambdas e =
+                    match e with
+                    | Expr.TyLambda(_, _, body, _, _) -> stripTyLambdas body
+                    | _ -> e
+                let isAliasOfUseBoundVal =
+                    match stripTyLambdas rhsExpr with
+                    | Expr.Val(vref, _, _) -> env.eUseBoundValStamps.Contains vref.Deref.Stamp
+                    | _ -> false
+                if isAliasOfUseBoundVal then
+                    bodyExpr, bodyExprTy
+                else
+                    // Issue #12300, scenario A: one Dispose per `use` binding,
+                    // regardless of how many names the pattern introduces.
+                    // `patternInputTmp` is the canonical value holding the bound expression:
+                    //   - for `use v = expr` it is `v` itself (see the TPat_as arm above);
+                    //   - for `use _ = expr` it is a fresh compiler-generated temp;
+                    //   - for `use a as b = expr` it is the outermost named val (e.g. `b`),
+                    //     and every other name in the pattern aliases the same object.
+                    let v = patternInputTmp
                     AddCxTypeMustSubsumeType ContextInfo.NoContext denv cenv.css v.Range NoTrace g.system_IDisposableNull_ty v.Type
                     let cleanupE = BuildDisposableCleanup cenv env m v
-                    mkTryFinally g (bodyExpr, cleanupE, m, bodyExprTy, DebugPointAtTry.No, DebugPointAtFinally.No), bodyExprTy)
+                    mkTryFinally g (bodyExpr, cleanupE, m, bodyExprTy, DebugPointAtTry.No, DebugPointAtFinally.No), bodyExprTy
             else
                 (bodyExpr, bodyExprTy)
 
         let envInner = AddLocalValMap g cenv.tcSink scopem prelimRecValues env
 
+        let envInner =
+            if isUse && not isFixed then
+                // Issue #12300: remember stamps of vals introduced by this `use` so a
+                // subsequent `use y = x` does not emit a duplicate Dispose for the same object.
+                let newStamps =
+                    (env.eUseBoundValStamps, prelimRecValues)
+                    ||> Map.fold (fun acc _ (v: Val) -> Set.add v.Stamp acc)
+                { envInner with eUseBoundValStamps = newStamps }
+            else
+                envInner
+
         ((buildExpr >> mkCleanup >> mkPatBind >> mkRhsBind), envInner, tpenv))
 
 /// Return binds corresponding to the linearised let-bindings.

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

@@ -368,6 +368,7 @@
     <Compile Include="Language\Nullness\NullableReferenceTypesTests.fs" />
     <Compile Include="Language\Nullness\EventNullnessTests.fs" />
     <Compile Include="Language\IndexedSetTests.fs" />
+    <Compile Include="Language\UseBindingDisposalTests.fs" />
     <Compile Include="Import\ImportTests.fs" />
     <Compile Include="ConstraintSolver\PrimitiveConstraints.fs" />
     <Compile Include="ConstraintSolver\MemberConstraints.fs" />

tests/FSharp.Compiler.ComponentTests/Language/UseBindingDisposalTests.fs

@@ -0,0 +1,123 @@
+namespace FSharp.Compiler.ComponentTests.Language
+
+open Xunit
+open FSharp.Test.Compiler
+
+module UseBindingDisposalTests =
+
+    [<Fact>]
+    let ``use a as b = d disposes once`` () =
+        FSharp """
+module M
+let mutable count = 0
+let d = { new System.IDisposable with
+            member _.Dispose() = count <- count + 1 }
+do
+    use a as b = d
+    ()
+printfn "%d" count
+"""
+        |> asExe
+        |> compileExeAndRun
+        |> shouldSucceed
+        |> withStdOutContains "1"
+
+    [<Fact>]
+    let ``use rebinding of use-bound value disposes once`` () =
+        FSharp """
+module M
+type Counter() =
+    let mutable count = 0
+    member _.DisposeCount = count
+    interface System.IDisposable with
+        member this.Dispose() = count <- count + 1
+let test() =
+    let c = new Counter()
+    (
+        use a = c :> System.IDisposable
+        use b = a
+        ()
+    )
+    c.DisposeCount
+printfn "%d" (test())
+"""
+        |> asExe
+        |> compileExeAndRun
+        |> shouldSucceed
+        |> withStdOutContains "1"
+
+    [<Fact>]
+    let ``Normal use disposes exactly once`` () =
+        FSharp """
+module M
+type Counter() =
+    let mutable count = 0
+    member _.DisposeCount = count
+    interface System.IDisposable with
+        member this.Dispose() = count <- count + 1
+let c = new Counter()
+let test() =
+    (
+        use x = (c :> System.IDisposable)
+        ()
+    )
+    c.DisposeCount
+printfn "%d" (test())
+"""
+        |> asExe
+        |> compileExeAndRun
+        |> shouldSucceed
+        |> withStdOutContains "1"
+
+    [<Fact>]
+    let ``Discarded use still disposes`` () =
+        FSharp """
+module M
+let mutable count = 0
+let d = { new System.IDisposable with
+            member _.Dispose() = count <- count + 1 }
+do
+    use _ = d
+    ()
+printfn "%d" count
+"""
+        |> asExe
+        |> compileExeAndRun
+        |> shouldSucceed
+        |> withStdOutContains "1"
+
+    [<Fact>]
+    let ``Two independent use bindings each dispose`` () =
+        FSharp """
+module M
+let mutable count = 0
+let mk () = { new System.IDisposable with member _.Dispose() = count <- count + 1 }
+let test() =
+    (
+        use a = mk()
+        use b = mk()
+        ()
+    )
+    count
+printfn "%d" (test())
+"""
+        |> asExe
+        |> compileExeAndRun
+        |> shouldSucceed
+        |> withStdOutContains "2"
+
+    [<Fact>]
+    let ``Triple alias via as-pattern disposes once`` () =
+        FSharp """
+module M
+let mutable count = 0
+let d = { new System.IDisposable with member _.Dispose() = count <- count + 1 }
+do
+    use a as b as c = d
+    ignore (a, b, c)
+printfn "%d" count
+"""
+        |> asExe
+        |> compileExeAndRun
+        |> shouldSucceed
+        |> withStdOutContains "1"