Skip to content

Warning 20 range: also walk through let/use bindings (follow-up to #19504) #19896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
T-Gro wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −8
Open

Warning 20 range: also walk through let/use bindings (follow-up to #19504) #19896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/release-notes/.FSharp.Compiler.Service/11.0.100.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
* Fix DU case names matching IWSAM member names no longer cause duplicate property entries. (Issue [#14321](https://github.com/dotnet/fsharp/issues/14321), [PR #19341](https://github.com/dotnet/fsharp/pull/19341))
* Fix DefaultAugmentation(false) duplicate entry in method table. (Issue [#16565](https://github.com/dotnet/fsharp/issues/16565), [PR #19341](https://github.com/dotnet/fsharp/pull/19341))
* Fix abstract event accessors now have SpecialName flag. (Issue [#5834](https://github.com/dotnet/fsharp/issues/5834), [PR #19341](https://github.com/dotnet/fsharp/pull/19341))
* Fix warning 20 ("expression is implicitly ignored") pointing at the wrong range when the last expression in a sequential block (e.g. inside `for`, `while` loops) is non-unit. The squiggle now correctly highlights only the offending expression. ([Issue #5735](https://github.com/dotnet/fsharp/issues/5735), [PR #19504](https://github.com/dotnet/fsharp/pull/19504))
* Fix warning 20 ("expression is implicitly ignored") pointing at the wrong range when the last expression in a sequential block (e.g. inside `for`, `while` loops) is non-unit. The squiggle now correctly highlights only the offending expression. ([Issue #5418](https://github.com/dotnet/fsharp/issues/5418), [PR #19504](https://github.com/dotnet/fsharp/pull/19504))
* Fix missing "No implementation was given" error when F# class inherits from a C# class with `abstract override` members without providing an implementation. ([Issue #7776](https://github.com/dotnet/fsharp/issues/7776), [PR #19503](https://github.com/dotnet/fsharp/pull/19503))
* Extend the warning 20 range fix to also walk through `let`/`use` bindings, so the squiggle lands on the offending expression when it is the body of a `let`/`use`. ([Issue #5418](https://github.com/dotnet/fsharp/issues/5418))
* Fix CLIEvent properties to be correctly recognized as events: `IsEvent` returns `true` and `XmlDocSig` uses `E:` prefix instead of `P:`. ([Issue #10273](https://github.com/dotnet/fsharp/issues/10273), [PR #18584](https://github.com/dotnet/fsharp/pull/18584))
* Fix extra sequence point at the end of match expressions. ([Issue #12052](https://github.com/dotnet/fsharp/issues/12052), [PR #19278](https://github.com/dotnet/fsharp/pull/19278))
* Fix wrong sequence point range for `return`/`yield`/`return!`/`yield!` inside computation expressions. ([Issue #19248](https://github.com/dotnet/fsharp/issues/19248), [PR #19278](https://github.com/dotnet/fsharp/pull/19278))
Expand Down
8 changes: 5 additions & 3 deletions src/Compiler/Checking/Expressions/CheckExpressions.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5582,12 +5582,14 @@ and TcStmt (cenv: cenv) env tpenv synExpr =
let g = cenv.g
let expr, ty, tpenv = TcExprOfUnknownType cenv env tpenv synExpr

// Use the range of the last expression in a sequential chain for warnings,
// so that "expression is ignored" diagnostics point at the offending expression
// rather than the entire sequential body. See https://github.com/dotnet/fsharp/issues/5735
// Use the range of the last expression in a sequential chain (also walking through
// 'let'/'use' bindings) for warnings, so that "expression is ignored" diagnostics point
// at the offending expression rather than the entire sequential body.
// See https://github.com/dotnet/fsharp/issues/5418
Comment on lines +5585 to +5588

@auduchinok auduchinok Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@T-Gro Could you cherry-pick the comments rules from the other PR?

let rec lastExprRange (e: SynExpr) =
match e with
| SynExpr.Sequential(expr2 = expr2) -> lastExprRange expr2
| SynExpr.LetOrUse({ Body = body }) -> lastExprRange body
| _ -> e.Range

let m = lastExprRange synExpr
Expand Down
35 changes: 31 additions & 4 deletions tests/FSharp.Compiler.ComponentTests/ErrorMessages/WarnExpressionTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ while x < 1 do
|> withSingleDiagnostic (Warning 20, Line 6, Col 5, Line 6, Col 9,
"The result of this expression has type 'bool' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

// https://github.com/dotnet/fsharp/issues/5735
// https://github.com/dotnet/fsharp/issues/5418

@auduchinok auduchinok Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are also redundant: the resulting commit is going to have the link to the PR with all the needed context.

[<Fact>]
let ``Warn On Last Expression In For Loop - int``() =
FSharp """
Expand All @@ -192,7 +192,7 @@ for i in 1 .. 10 do
|> withSingleDiagnostic (Warning 20, Line 7, Col 5, Line 7, Col 8,
"The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

// https://github.com/dotnet/fsharp/issues/5735
// https://github.com/dotnet/fsharp/issues/5418
[<Fact>]
let ``Warn On Last Expression In For Loop - string``() =
FSharp """
Expand All @@ -205,7 +205,7 @@ for i in 1 .. 10 do
|> withSingleDiagnostic (Warning 20, Line 4, Col 5, Line 4, Col 12,
"The result of this expression has type 'string' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

// https://github.com/dotnet/fsharp/issues/5735
// https://github.com/dotnet/fsharp/issues/5418
[<Fact>]
let ``Warn On Last Expression In Integer For Loop``() =
FSharp """
Expand All @@ -218,7 +218,7 @@ for i = 1 to 10 do
|> withSingleDiagnostic (Warning 20, Line 4, Col 5, Line 4, Col 7,
"The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

// https://github.com/dotnet/fsharp/issues/5735
// https://github.com/dotnet/fsharp/issues/5418
[<Fact>]
let ``Warn On Last Expression In While Loop - non-bool``() =
FSharp """
Expand All @@ -233,6 +233,33 @@ while x < 1 do
|> withSingleDiagnostic (Warning 20, Line 6, Col 5, Line 6, Col 8,
"The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

// https://github.com/dotnet/fsharp/issues/5418
[<Fact>]
let ``Warn On Last Expression In For Loop - non-unit after let binding``() =
FSharp """
for _ in [] do
let x = 1
x
"""
|> typecheck
|> shouldFail
|> withSingleDiagnostic (Warning 20, Line 4, Col 5, Line 4, Col 6,
"The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

// https://github.com/dotnet/fsharp/issues/5418
[<Fact>]
let ``Warn On Last Expression In For Loop - non-unit after nested let bindings``() =
FSharp """
for _ in 1 .. 3 do
let a = 1
let b = 2
a + b
"""
|> typecheck
|> shouldFail
|> withSingleDiagnostic (Warning 20, Line 5, Col 5, Line 5, Col 10,
"The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

@abonie abonie Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Test Coverage / LOW] The PR title and release note both say "let/use", but the two new tests only cover let bindings. The use path goes through the identical code (SynExpr.LetOrUse is one union case for both), so it works — but there is no regression guard for it.

Suggested addition:

// https://github.com/dotnet/fsharp/issues/5418
[<Fact>]
let ``Warn On Last Expression In For Loop - non-unit after use binding``() =
    FSharp """
type D() =
    interface System.IDisposable with
        member _.Dispose() = ()
    member _.Value = 1
for _ in [] do
    use d = new D()
    d.Value
    """
    |> typecheck
    |> shouldFail
    |> withSingleDiagnostic (Warning 20, Line 8, Col 5, Line 8, Col 12,
                             "The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")

@auduchinok auduchinok Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If adding more cases, a case with a computation expression could be useful.


[<Fact>]
let ``Warn If Possible Property Setter``() =
FSharp """
Expand Down
Loading