From bdc468e8251f1c1bdeacb836e81b4c7788323211 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 5 Jun 2026 10:55:57 +0200 Subject: [PATCH 1/2] Fix #16419 at source: avoid double type-check of seq implicit-yield body Replace the HashSet-based dedup in TcResultsSinkImpl.NotifyFormatSpecifierLocation (added in PR #19791) with a fix at the source. In CheckSequenceExpressions.fs, the implicit-yield branch of tcSequenceExprBodyAsSequenceOrStatement first calls TryTcStmt to detect whether the body has type unit, then (if not) calls TcExprFlex on the same SynExpr a second time to type-check it as a yielded expression. Both passes invoke side-effecting checks like format-string parsing, which is why printf format-specifier sink notifications, name-resolution events, etc., were doubled for a single sub-expression. Reuse the existing eCachedImplicitYieldExpressions HashMultiMap (already used by TcExprSequentialOrImplicitYield for the e1; e2 case) so the second type-check returns the cached result from the first pass instead of re-running it. TcExprThen looks up by SynExpr.Range gated by obj.ReferenceEquals, so identity is preserved and there is no risk of cross-expression collisions. Tests in EditorTests.fs verify: - single format-specifier emission for seq { sprintf "%d" 1 }, seq { sprintf "%d %s %A" 1 "x" 2 }, seq { printfn "%d" 1 } - expected-type-driven inference is preserved: subsumption int->obj and string->obj, nullness flex, overload resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../.FSharp.Compiler.Service/11.0.100.md | 2 +- .../Expressions/CheckSequenceExpressions.fs | 18 +++- src/Compiler/Checking/NameResolution.fs | 5 +- .../EditorTests.fs | 87 ++++++++++++++++++- 4 files changed, 103 insertions(+), 9 deletions(-) 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 9509ac72a3a..dc135c64eb8 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/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)) diff --git a/src/Compiler/Checking/Expressions/CheckSequenceExpressions.fs b/src/Compiler/Checking/Expressions/CheckSequenceExpressions.fs index 86bf635d58a..838de014182 100644 --- a/src/Compiler/Checking/Expressions/CheckSequenceExpressions.fs +++ b/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 diff --git a/src/Compiler/Checking/NameResolution.fs b/src/Compiler/Checking/NameResolution.fs index 36eedebfbe8..898dc78aa75 100644 --- a/src/Compiler/Checking/NameResolution.fs +++ b/src/Compiler/Checking/NameResolution.fs @@ -2185,8 +2185,6 @@ type TcResultsSinkImpl(tcGlobals, ?sourceText: ISourceText) = let capturedOpenDeclarations = ResizeArray() let capturedFormatSpecifierLocations = ResizeArray<_>() - let capturedFormatSpecifierRanges = HashSet() - let capturedNameResolutionIdentifiers = HashSet { 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 diff --git a/tests/FSharp.Compiler.Service.Tests/EditorTests.fs b/tests/FSharp.Compiler.Service.Tests/EditorTests.fs index f9c1da195fe..bf231e2e0fd 100644 --- a/tests/FSharp.Compiler.Service.Tests/EditorTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/EditorTests.fs @@ -542,18 +542,99 @@ let _ = debug "[LanguageService] Type checking fails for '%s' with content=%A an (4, 108, 4, 110, 1)|] [] -let ``Format specifier locations not duplicated in CE`` () = +let ``Format specifier locations not duplicated in seq CE (implicit yield)`` () = + // 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. 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 + typeCheckResults.GetFormatSpecifierLocationsAndArity() |> Array.filter (fun (r, _) -> r.StartColumn = 23) Assert.Equal(1, percentD.Length) +[] +let ``Format specifier locations not duplicated in seq CE with multiple specifiers`` () = + let input = "let _ = seq { sprintf \"%d %s %A\" 1 \"x\" 2 }" + let file = "/home/user/Test.fsx" + let _parseResult, typeCheckResults = parseAndCheckScript(file, input) + + let locs = typeCheckResults.GetFormatSpecifierLocationsAndArity() + // Three format specifiers: %d %s %A, each must appear exactly once. + Assert.Equal(3, locs.Length) + +[] +let ``Format specifier locations not duplicated in seq CE with non-yielded statement-typed body`` () = + // Body returns unit so TryTcStmt succeeds and TcExprFlex is NOT taken; assert single emission. + let input = "let _ = seq { printfn \"%d\" 1 }" + let file = "/home/user/Test.fsx" + let _parseResult, typeCheckResults = parseAndCheckScript(file, input) + let locs = typeCheckResults.GetFormatSpecifierLocationsAndArity() + Assert.Equal(1, locs.Length) + +// The following tests validate that caching the implicit-yield first-pass typecheck +// does not break expected-type-driven inference (subsumption, type-directed +// conversion, overload resolution, nullness flex). They are aligned with the +// behavior of `seq { yield expr }` (no implicit yield) and the legacy `seq { e1; e2 }` +// implicit-yield path which uses the same cache mechanism. + +[] +let ``Implicit-yield seq subsumption int to obj`` () = + let input = "let xs : seq = seq { 1 }\nlet ys : seq = seq { yield 1 }" + let file = "/home/user/Test.fsx" + let parseResult, typeCheckResults = parseAndCheckScript(file, input) + Assert.False(parseResult.ParseHadErrors) + let errors = + typeCheckResults.Diagnostics + |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) + Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) + +[] +let ``Implicit-yield seq subsumption string to obj`` () = + let input = "let xs : seq = seq { \"hi\" }" + let file = "/home/user/Test.fsx" + let _parseResult, typeCheckResults = parseAndCheckScript(file, input) + let errors = + typeCheckResults.Diagnostics + |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) + Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) + +[] +let ``Implicit-yield seq with nullable element type`` () = + let input = + """ +#nowarn "0025" +let xs : seq = seq { "hi" } +""" + let file = "/home/user/Test.fsx" + let _parseResult, typeCheckResults = parseAndCheckScript(file, input) + let errors = + typeCheckResults.Diagnostics + |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) + Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) + +[] +let ``Implicit-yield seq with overloaded function`` () = + // Test that overload resolution still picks the right overload when the body + // is a method call on an expected element type. + let input = + """ +type T() = + static member M(x: int) = "int" + static member M(x: string) = "string" +let xs : seq = seq { T.M(1) } +""" + let file = "/home/user/Test.fsx" + let _parseResult, typeCheckResults = parseAndCheckScript(file, input) + let errors = + typeCheckResults.Diagnostics + |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) + Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) + #if ASSUME_PREVIEW_FSHARP_CORE [] let ``Printf specifiers for regular and verbatim interpolated strings`` () = From ba946849df7369a0570fc6b607b7f956bcb4dfbd Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 5 Jun 2026 11:13:30 +0200 Subject: [PATCH 2/2] Refactor EditorTests: collapse new tests into Theory, add file/parseAndCheck helpers - Add module-level testFile constant (Path.Combine for OS neutrality) - Add parseAndCheck helper so each test no longer re-declares the file - Collapse the 3 format-specifier and 4 implicit-yield tests added in the previous commit into 2 Theory tests driven by InlineData Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../EditorTests.fs | 121 +++++------------- 1 file changed, 32 insertions(+), 89 deletions(-) diff --git a/tests/FSharp.Compiler.Service.Tests/EditorTests.fs b/tests/FSharp.Compiler.Service.Tests/EditorTests.fs index bf231e2e0fd..a7654129011 100644 --- a/tests/FSharp.Compiler.Service.Tests/EditorTests.fs +++ b/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,99 +548,35 @@ let _ = debug "[LanguageService] Type checking fails for '%s' with content=%A an (4, 82, 4, 84, 1); (4, 108, 4, 110, 1)|] -[] -let ``Format specifier locations not duplicated in seq CE (implicit yield)`` () = - // 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. - let input = "let _ = seq { sprintf \"%d\" 1 }" - let file = "/home/user/Test.fsx" - let _parseResult, typeCheckResults = parseAndCheckScript(file, input) - - let percentD = - typeCheckResults.GetFormatSpecifierLocationsAndArity() - |> Array.filter (fun (r, _) -> r.StartColumn = 23) - - Assert.Equal(1, percentD.Length) - -[] -let ``Format specifier locations not duplicated in seq CE with multiple specifiers`` () = - let input = "let _ = seq { sprintf \"%d %s %A\" 1 \"x\" 2 }" - let file = "/home/user/Test.fsx" - let _parseResult, typeCheckResults = parseAndCheckScript(file, input) - - let locs = typeCheckResults.GetFormatSpecifierLocationsAndArity() - // Three format specifiers: %d %s %A, each must appear exactly once. - Assert.Equal(3, locs.Length) - -[] -let ``Format specifier locations not duplicated in seq CE with non-yielded statement-typed body`` () = - // Body returns unit so TryTcStmt succeeds and TcExprFlex is NOT taken; assert single emission. - let input = "let _ = seq { printfn \"%d\" 1 }" - let file = "/home/user/Test.fsx" - let _parseResult, typeCheckResults = parseAndCheckScript(file, input) +// 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. +[] +[] +[] +[] +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(1, locs.Length) - -// The following tests validate that caching the implicit-yield first-pass typecheck -// does not break expected-type-driven inference (subsumption, type-directed -// conversion, overload resolution, nullness flex). They are aligned with the -// behavior of `seq { yield expr }` (no implicit yield) and the legacy `seq { e1; e2 }` -// implicit-yield path which uses the same cache mechanism. - -[] -let ``Implicit-yield seq subsumption int to obj`` () = - let input = "let xs : seq = seq { 1 }\nlet ys : seq = seq { yield 1 }" - let file = "/home/user/Test.fsx" - let parseResult, typeCheckResults = parseAndCheckScript(file, input) - Assert.False(parseResult.ParseHadErrors) - let errors = - typeCheckResults.Diagnostics - |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) - Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) - -[] -let ``Implicit-yield seq subsumption string to obj`` () = - let input = "let xs : seq = seq { \"hi\" }" - let file = "/home/user/Test.fsx" - let _parseResult, typeCheckResults = parseAndCheckScript(file, input) - let errors = - typeCheckResults.Diagnostics - |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) - Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) - -[] -let ``Implicit-yield seq with nullable element type`` () = - let input = - """ -#nowarn "0025" -let xs : seq = seq { "hi" } -""" - let file = "/home/user/Test.fsx" - let _parseResult, typeCheckResults = parseAndCheckScript(file, input) - let errors = - typeCheckResults.Diagnostics - |> Array.filter (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error) - Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) - -[] -let ``Implicit-yield seq with overloaded function`` () = - // Test that overload resolution still picks the right overload when the body - // is a method call on an expected element type. - let input = - """ -type T() = - static member M(x: int) = "int" - static member M(x: string) = "string" -let xs : seq = seq { T.M(1) } -""" - let file = "/home/user/Test.fsx" - let _parseResult, typeCheckResults = parseAndCheckScript(file, input) + 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). +[] +[ = seq { 1 }")>] +[ = seq { yield 1 }")>] +[ = seq { \"hi\" }")>] +[ = seq { \"hi\" }")>] +[ = 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) - Assert.Equal<_ seq>(Seq.empty, errors |> Array.map (fun d -> d.Message)) + |> Array.map (fun d -> d.Message) + Assert.Equal<_ seq>(Array.empty, errors) #if ASSUME_PREVIEW_FSHARP_CORE []