diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index c18e1d8f21..5f76659ac6 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/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 `[]` prefix attributes being silently dropped on class members, and fix false-positive `AllowMultiple=false` errors when `[]` and `[]` 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)) diff --git a/src/Compiler/Checking/CheckBasics.fs b/src/Compiler/Checking/CheckBasics.fs index d8fdd288af..ebc842db68 100644 --- a/src/Compiler/Checking/CheckBasics.fs +++ b/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 + + /// See .fsi. + eUseBoundValStamps: Set } member tenv.DisplayEnv = tenv.eNameResEnv.DisplayEnv diff --git a/src/Compiler/Checking/CheckBasics.fsi b/src/Compiler/Checking/CheckBasics.fsi index 0191cf018f..f5685bb922 100644 --- a/src/Compiler/Checking/CheckBasics.fsi +++ b/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 + + /// 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 } member DisplayEnv: DisplayEnv diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 4a4361e980..aa8eb266d0 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/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) -> diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index 37ff7f54b6..75a1997b6c 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/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. diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index acfeca79f3..2f23cdc107 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -368,6 +368,7 @@ + diff --git a/tests/FSharp.Compiler.ComponentTests/Language/UseBindingDisposalTests.fs b/tests/FSharp.Compiler.ComponentTests/Language/UseBindingDisposalTests.fs new file mode 100644 index 0000000000..f2cf991aa9 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Language/UseBindingDisposalTests.fs @@ -0,0 +1,123 @@ +namespace FSharp.Compiler.ComponentTests.Language + +open Xunit +open FSharp.Test.Compiler + +module UseBindingDisposalTests = + + [] + 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" + + [] + 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" + + [] + 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" + + [] + 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" + + [] + 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" + + [] + 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"