Fix #16419 at source: avoid double type-check of seq implicit-yield body

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

@@ -1,7 +1,7 @@
 ### Fixed
 
 * Suppress hover/symbol resolution for wildcard `_` patterns inside `member _.…` bodies that incorrectly showed `val _: T` tooltip. ([PR #19760](https://github.com/dotnet/fsharp/pull/19760))
-* Deduplicate format specifier locations in computation expressions so editor tooling no longer reports duplicate entries for the same `%` specifier. ([Issue #16419](https://github.com/dotnet/fsharp/issues/16419), [PR #19791](https://github.com/dotnet/fsharp/pull/19791))
+* Fix the source of duplicate format specifier locations in `seq { }` computation expressions with implicit yield: the body was speculatively type-checked twice (once as a statement, once as a yielded expression), causing the sink to be notified twice per `%` specifier. The second pass now reuses the cached first-pass result. ([Issue #16419](https://github.com/dotnet/fsharp/issues/16419), [PR #19791](https://github.com/dotnet/fsharp/pull/19791))
 * Reject non-function bindings for single-case and partial active pattern names with FS1209, matching the existing multi-case behavior. ([PR #19763](https://github.com/dotnet/fsharp/pull/19763))
 * Fix FS0421 "The address of the variable cannot be used at this point" incorrectly raised for the discard pattern `let _ = &expr` when `let x = &expr` compiles. ([Issue #18841](https://github.com/dotnet/fsharp/issues/18841), [PR #19811](https://github.com/dotnet/fsharp/pull/19811))
 * 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))

src/Compiler/Checking/Expressions/CheckSequenceExpressions.fs

@@ -439,7 +439,23 @@ let TcSequenceExpression (cenv: TcFileState) env tpenv comp (overallTy: OverallT
                     let genResultTy = NewInferenceType g
                     let mExpr = expr.Range
                     UnifyTypes cenv env mExpr genOuterTy (mkSeqTy cenv.g genResultTy)
-                    let expr, tpenv = TcExprFlex cenv flex true genResultTy env tpenv comp
+
+                    // Cache the result of the first type-check so that re-checking 'comp' below as a yielded
+                    // expression reuses it instead of running side-effecting checks (e.g. format string parsing)
+                    // a second time. See also TcExprSequentialOrImplicitYield which uses the same cache.
+                    let cachedExpr =
+                        match expr with
+                        | Expr.DebugPoint(_, e) -> e
+                        | _ -> expr
+
+                    env.eCachedImplicitYieldExpressions.Add(comp.Range, (comp, _ty, cachedExpr))
+
+                    let expr, tpenv =
+                        try
+                            TcExprFlex cenv flex true genResultTy env tpenv comp
+                        finally
+                            env.eCachedImplicitYieldExpressions.Remove comp.Range
+
                     let exprTy = tyOfExpr cenv.g expr
                     AddCxTypeMustSubsumeType env.eContextInfo env.DisplayEnv cenv.css mExpr NoTrace genResultTy exprTy
 

src/Compiler/Checking/NameResolution.fs

@@ -2185,8 +2185,6 @@ type TcResultsSinkImpl(tcGlobals, ?sourceText: ISourceText) =
     let capturedOpenDeclarations = ResizeArray<OpenDeclaration>()
     let capturedFormatSpecifierLocations = ResizeArray<_>()
 
-    let capturedFormatSpecifierRanges = HashSet<range>()
-
     let capturedNameResolutionIdentifiers =
         HashSet<pos * string>
             { new IEqualityComparer<_> with
@@ -2291,8 +2289,7 @@ type TcResultsSinkImpl(tcGlobals, ?sourceText: ISourceText) =
                     capturedMethodGroupResolutions.Add(CapturedNameResolution(itemMethodGroup, [], occurrenceType, nenv, ad, m))
 
         member sink.NotifyFormatSpecifierLocation(m, numArgs) =
-            if capturedFormatSpecifierRanges.Add(m) then
-                capturedFormatSpecifierLocations.Add((m, numArgs))
+            capturedFormatSpecifierLocations.Add((m, numArgs))
 
         member sink.NotifyRelatedSymbolUse(m, item, kind) =
             if allowedRange m then

tests/FSharp.Compiler.Service.Tests/EditorTests.fs

@@ -11,6 +11,13 @@ open FSharp.Compiler.Tokenization
 
 #nowarn "1182" // Unused bindings when ignored parsed results etc.
 
+/// Virtual source filename used by tests that don't need a real on-disk path.
+/// Path.Combine keeps this OS-neutral for any test that compares the resulting range filename.
+let private testFile = System.IO.Path.Combine(System.IO.Path.GetTempPath(), "Test.fsx")
+
+/// Parses and type-checks an inline source snippet against the shared `testFile` identifier.
+let private parseAndCheck (source: string) = parseAndCheckScript (testFile, source)
+
 let stringMethods =
     [
         "Chars"; "Clone"; "CompareTo"; "Contains"; "CopyTo"; "EndsWith";
@@ -541,18 +548,35 @@ let _ = debug "[LanguageService] Type checking fails for '%s' with content=%A an
                      (4, 82, 4, 84, 1);
                      (4, 108, 4, 110, 1)|]
 
-[<Fact>]
-let ``Format specifier locations not duplicated in CE`` () =
-    let input = "let _ = seq { sprintf \"%d\" 1 }"
-    let file = "/home/user/Test.fsx"
-    let _parseResult, typeCheckResults = parseAndCheckScript(file, input)
-
-    let locations = typeCheckResults.GetFormatSpecifierLocationsAndArity()
-    let percentD =
-        locations
-        |> Array.filter (fun (r, _) -> r.StartColumn = 23)
-
-    Assert.Equal(1, percentD.Length)
+// Regression for issue #16419: in `seq { e }` with implicit-yield, the body 'e' was
+// type-checked twice (once as a statement via TryTcStmt, once as a yielded expression).
+// Both passes used to notify the sink, leading to duplicate format-specifier entries
+// when 'e' contained a printf-style format string.
+[<Theory>]
+[<InlineData("let _ = seq { sprintf \"%d\" 1 }", 1)>]
+[<InlineData("let _ = seq { sprintf \"%d %s %A\" 1 \"x\" 2 }", 3)>]
+[<InlineData("let _ = seq { printfn \"%d\" 1 }", 1)>]
+let ``Format specifier locations are not duplicated in seq computation expression`` (source: string, expectedCount: int) =
+    let _, typeCheckResults = parseAndCheck source
+    let locs = typeCheckResults.GetFormatSpecifierLocationsAndArity()
+    Assert.Equal(expectedCount, locs.Length)
+
+// Validates that caching the implicit-yield first-pass typecheck does not break
+// expected-type-driven inference (subsumption, type-directed conversion,
+// nullness flex, overload resolution).
+[<Theory>]
+[<InlineData("let xs : seq<obj> = seq { 1 }")>]
+[<InlineData("let ys : seq<obj> = seq { yield 1 }")>]
+[<InlineData("let xs : seq<obj> = seq { \"hi\" }")>]
+[<InlineData("#nowarn \"0025\"\nlet xs : seq<string | null> = seq { \"hi\" }")>]
+[<InlineData("type T() =\n    static member M(x: int) = \"int\"\n    static member M(x: string) = \"string\"\nlet xs : seq<string> = seq { T.M(1) }")>]
+let ``Implicit-yield in seq preserves expected-type-driven inference`` (source: string) =
+    let _, typeCheckResults = parseAndCheck source
+    let errors =
+        typeCheckResults.Diagnostics
+        |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error)
+        |> Array.map (fun d -> d.Message)
+    Assert.Equal<_ seq>(Array.empty, errors)
 
 #if ASSUME_PREVIEW_FSHARP_CORE
 [<Fact>]