From 25cadddf10186f3fb10200f2d8269bac6cc2ab74 Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 11:07:06 +0100 Subject: [PATCH 1/7] add test case showing a false positive with yield! --- .../ErrorMessages/TailCallAttribute.fs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs index 9d34b2466e6..c1247d754e4 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs @@ -1522,3 +1522,42 @@ namespace N Message = "The member or function 'reverse' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." } ] + + [] + let ``Don't warn for yield! call of tail rec func`` () = + """ +namespace N + +module M = + + type SynExpr = + | Sequential of expr1 : SynExpr * expr2 : SynExpr + | NotSequential + member _.Range = 99 + + type SyntaxNode = SynExpr of SynExpr + + type SyntaxVisitor () = member _.VisitExpr _ = None + + let visitor = SyntaxVisitor () + let dive expr range f = range, fun () -> Some expr + let traverseSynExpr _ expr = Some expr + + [] + let rec traverseSequentials path expr = + seq { + match expr with + | SynExpr.Sequential(expr1 = expr1; expr2 = SynExpr.Sequential _ as expr2) -> + yield dive expr expr.Range (fun expr -> visitor.VisitExpr(path, traverseSynExpr path, (fun _ -> None), expr)) + let path = SyntaxNode.SynExpr expr :: path + yield dive expr1 expr1.Range (traverseSynExpr path) + yield! traverseSequentials path expr2 // should not warn + + | _ -> + yield dive expr expr.Range (traverseSynExpr path) + } + """ + |> FSharp + |> withLangVersion80 + |> compile + |> shouldSucceed From 21abe7d35802751fbe9158c9f8d55e37f22a31a0 Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 11:07:43 +0100 Subject: [PATCH 2/7] add missing case in IsAppInLambdaBody that can happen with yield! --- src/Compiler/Checking/TailCallChecks.fs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Compiler/Checking/TailCallChecks.fs b/src/Compiler/Checking/TailCallChecks.fs index aaeec0e81db..95b5c4a99df 100644 --- a/src/Compiler/Checking/TailCallChecks.fs +++ b/src/Compiler/Checking/TailCallChecks.fs @@ -222,6 +222,13 @@ and CheckCall cenv args ctxts (tailCall: TailCall) = | Expr.App _ -> Some(TailCall.YesFromExpr cenv.g e) | IsAppInLambdaBody t -> Some t | _ -> None + | Expr.App(_funcExpr, _formalType, _typeArgs, args, _range) -> + args + |> List.tryPick (fun a -> + match a with + | IsAppInLambdaBody t -> Some t + | _ -> None) + | _ -> None // if we haven't already decided this is no tail call, try to detect CPS-like expressions From 093b281794c36e311060759a880e9378fd17bf25 Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 11:19:46 +0100 Subject: [PATCH 3/7] add release notes entry --- docs/release-notes/.FSharp.Compiler.Service/8.0.300.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index d055d7c988b..5879364041b 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -1,5 +1,6 @@ ### Fixed +* Fix a false positive of the `[]` analysis in combination with `yield!`. ([PR #16882](https://github.com/dotnet/fsharp/pull/xxx)) * Don't blow the stack when traversing deeply nested sequential expressions. ([PR #16882](https://github.com/dotnet/fsharp/pull/16882)) * Fix wrong range start of INTERP_STRING_END. ([PR #16774](https://github.com/dotnet/fsharp/pull/16774), [PR #16785](https://github.com/dotnet/fsharp/pull/16785)) * Fix missing warning for recursive calls in list comprehensions. ([PR #16652](https://github.com/dotnet/fsharp/pull/16652)) From 7faddd837e9b3c54d3e5e4f2c6f52d041a72f4f5 Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 11:26:38 +0100 Subject: [PATCH 4/7] update PR number --- docs/release-notes/.FSharp.Compiler.Service/8.0.300.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index 5879364041b..3d85db7e572 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -1,6 +1,6 @@ ### Fixed -* Fix a false positive of the `[]` analysis in combination with `yield!`. ([PR #16882](https://github.com/dotnet/fsharp/pull/xxx)) +* Fix a false positive of the `[]` analysis in combination with `yield!`. ([PR #16933](https://github.com/dotnet/fsharp/pull/16933)) * Don't blow the stack when traversing deeply nested sequential expressions. ([PR #16882](https://github.com/dotnet/fsharp/pull/16882)) * Fix wrong range start of INTERP_STRING_END. ([PR #16774](https://github.com/dotnet/fsharp/pull/16774), [PR #16785](https://github.com/dotnet/fsharp/pull/16785)) * Fix missing warning for recursive calls in list comprehensions. ([PR #16652](https://github.com/dotnet/fsharp/pull/16652)) From 59eda6bc938310893257a88045b7e9405e052088 Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 11:30:48 +0100 Subject: [PATCH 5/7] only bind to what we are interested in --- src/Compiler/Checking/TailCallChecks.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Checking/TailCallChecks.fs b/src/Compiler/Checking/TailCallChecks.fs index 95b5c4a99df..97bdc05680b 100644 --- a/src/Compiler/Checking/TailCallChecks.fs +++ b/src/Compiler/Checking/TailCallChecks.fs @@ -222,7 +222,7 @@ and CheckCall cenv args ctxts (tailCall: TailCall) = | Expr.App _ -> Some(TailCall.YesFromExpr cenv.g e) | IsAppInLambdaBody t -> Some t | _ -> None - | Expr.App(_funcExpr, _formalType, _typeArgs, args, _range) -> + | Expr.App(args = args) -> args |> List.tryPick (fun a -> match a with From f98a097e7e55f7f1106c49fd3251221be9a376ed Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 13:36:47 +0100 Subject: [PATCH 6/7] add test expecting a warning for yield! in a list comprehension --- .../ErrorMessages/TailCallAttribute.fs | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs index c1247d754e4..df45a6e0cf5 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs @@ -1524,7 +1524,7 @@ namespace N ] [] - let ``Don't warn for yield! call of tail rec func`` () = + let ``Don't warn for yield! call of rec func in seq`` () = """ namespace N @@ -1561,3 +1561,58 @@ module M = |> withLangVersion80 |> compile |> shouldSucceed + + [] + let ``Warn for yield! call of rec func in list comprehension`` () = + """ +namespace N + +module M = + + type SynExpr = + | Sequential of expr1 : SynExpr * expr2 : SynExpr + | NotSequential + member _.Range = 99 + + type SyntaxNode = SynExpr of SynExpr + + type SyntaxVisitor () = member _.VisitExpr _ = None + + let visitor = SyntaxVisitor () + let dive expr range f = range, fun () -> Some expr + let traverseSynExpr _ expr = Some expr + + [] + let rec traverseSequentials path expr = + [ + match expr with + | SynExpr.Sequential(expr1 = expr1; expr2 = SynExpr.Sequential _ as expr2) -> + // It's a nested sequential expression. + // Visit it, but make defaultTraverse do nothing, + // since we're going to traverse its descendants ourselves. + yield dive expr expr.Range (fun expr -> visitor.VisitExpr(path, traverseSynExpr path, (fun _ -> None), expr)) + + // Now traverse its descendants. + let path = SyntaxNode.SynExpr expr :: path + yield dive expr1 expr1.Range (traverseSynExpr path) + yield! traverseSequentials path expr2 // should warn + + | _ -> + // It's not a nested sequential expression. + // Traverse it normally. + yield dive expr expr.Range (traverseSynExpr path) + ] + """ + |> FSharp + |> withLangVersion80 + |> compile + |> shouldFail + |> withResults [ + { Error = Warning 3569 + Range = { StartLine = 32 + StartColumn = 24 + EndLine = 32 + EndColumn = 54 } + Message = + "The member or function 'traverseSequentials' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." } + ] From 35bb99b2f00c50c597959192315c2d7fb2d6f355 Mon Sep 17 00:00:00 2001 From: dawe Date: Fri, 22 Mar 2024 23:22:18 +0100 Subject: [PATCH 7/7] add test case using yield! in a custom CE that overflows the stack --- .../ErrorMessages/TailCallAttribute.fs | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs index df45a6e0cf5..e3ff4102f7c 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs @@ -1616,3 +1616,70 @@ module M = Message = "The member or function 'traverseSequentials' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." } ] + + [] + let ``Warn for yield! call of rec func in custom CE`` () = + """ +namespace N + +module M = + + type SynExpr = + | Sequential of expr1 : SynExpr * expr2 : SynExpr + | NotSequential + member _.Range = 99 + + type SyntaxNode = SynExpr of SynExpr + + type SyntaxVisitor () = member _.VisitExpr _ = None + + let visitor = SyntaxVisitor () + let dive expr range f = range, fun () -> Some expr + let traverseSynExpr _ expr = Some expr + + type ThingsBuilder() = + + member _.Yield(x) = [ x ] + + member _.Combine(currentThings, newThings) = currentThings @ newThings + + member _.Delay(f) = f () + + member _.YieldFrom(x) = x + + let things = ThingsBuilder() + + [] + let rec traverseSequentials path expr = + things { + match expr with + | SynExpr.Sequential(expr1 = expr1; expr2 = SynExpr.Sequential _ as expr2) -> + // It's a nested sequential expression. + // Visit it, but make defaultTraverse do nothing, + // since we're going to traverse its descendants ourselves. + yield dive expr expr.Range (fun expr -> visitor.VisitExpr(path, traverseSynExpr path, (fun _ -> None), expr)) + + // Now traverse its descendants. + let path = SyntaxNode.SynExpr expr :: path + yield dive expr1 expr1.Range (traverseSynExpr path) + yield! traverseSequentials path expr2 // should warn + + | _ -> + // It's not a nested sequential expression. + // Traverse it normally. + yield dive expr expr.Range (traverseSynExpr path) + } + """ + |> FSharp + |> withLangVersion80 + |> compile + |> shouldFail + |> withResults [ + { Error = Warning 3569 + Range = { StartLine = 43 + StartColumn = 17 + EndLine = 43 + EndColumn = 68 } + Message = + "The member or function 'traverseSequentials' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." } + ]