Skip to content

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

Merged
T-Gro merged 10 commits into
mainfrom
copilot/fix-warning-20-let-binding-range
Jun 23, 2026
Merged

Warning 20 range: also walk through let/use bindings (follow-up to #19504)#19896
T-Gro merged 10 commits into
mainfrom
copilot/fix-warning-20-let-binding-range

Conversation

@T-Gro

@T-GroT-Gro commented Jun 5, 2026•
edited
Loading

Copy link
Copy Markdown
Member

Follow-up to #19504 addressing review feedback from @auduchinok.

#19504 fixed warning 20 ("expression is implicitly ignored") to point at the last expression of a sequential body inside for/while. The walk only descended through SynExpr.Sequential, so when the last expression was the body of a let/use binding, the squiggle covered the whole let expression instead of just the offending tail:

for_in[]doletx=1// \
x // > squiggle spanned both lines (from `let` to end of `x`)

This PR also descends through SynExpr.LetOrUse, so the squiggle lands on x alone in cases like the one above.

It also corrects the issue reference cited by the original PR (#5418 — "Wrong expression is ignored warning range"; the previously cited #5735 is unrelated to this diagnostic).

Tracking: #5418

Tomas Grosup@T-Gro
Address Eugene's feedback on PR#19504
2740310
- Walk through let/use bindings in addition to Sequential when locating
the offending expression for warning 20. Fixes the case Eugene flagged:
for _ in [] do
let x = 1
x
- Fix wrong issue link (#5735 -> #5418) in code comment, release notes,
and test annotations.
- Add regression tests for the let/use case (single + nested bindings).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
T-Gro requested a review from a team as a code ownerJune 5, 2026 09:11
@github-actions

github-actionsBot commented Jun 5, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev

✅ Found changes and release notes in following paths:

Change pathRelease notes pathDescription
src/Compilerdocs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@github-actionsgithub-actionsBot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 5, 2026
@T-Gro
T-Gro requested a review from abonieJune 5, 2026 09:49
@T-Gro
T-Gro enabled auto-merge (squash) June 5, 2026 09:49
abonie
abonie reviewed Jun 8, 2026
View reviewed changes

@abonieabonie left a comment

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.

🤖 AI-generated review — verify before acting on findings.

Comment threadtests/FSharp.Compiler.ComponentTests/ErrorMessages/WarnExpressionTests.fs
@abonieabonie added the AI-reviewed PR reviewed by AI review council label Jun 8, 2026
@github-project-automationgithub-project-automationBot moved this from New to In Progress in F# Compiler and ToolingJun 8, 2026
auduchinok
auduchinok approved these changes Jun 11, 2026
View reviewed changes
Comment threadsrc/Compiler/Checking/Expressions/CheckExpressions.fs Outdated
Comment threadtests/FSharp.Compiler.ComponentTests/ErrorMessages/WarnExpressionTests.fs Outdated
Comment threadtests/FSharp.Compiler.ComponentTests/ErrorMessages/WarnExpressionTests.fs
Copilotand others added 4 commits June 12, 2026 12:34
Address review feedback: remove redundant comments, add use/CE tests,…
3330e8c
… add PR link
- Remove issue URL references from code comments and test methods
(per auduchinok: redundant, commit links to PR with full context)
- Add test for 'use' binding (non-unit after use)
- Add test for computation expression after let binding
- Add PR #19896 link to release notes
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
Merge remote-tracking branch 'origin/main' into copilot/fix-warning-2…
377625b
…0-let-binding-range
@T-Gro
Address Eugene's feedback: cut comment bloat, strengthen CE test
3b7449e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
Drop duplicate release-notes entry; PR#19504's existing bullet alrea…
c935971
…dy covers the user-facing fix
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
auduchinok
auduchinok reviewed Jun 12, 2026
View reviewed changes
Comment threadtests/FSharp.Compiler.ComponentTests/ErrorMessages/WarnExpressionTests.fs Outdated
T-Groand others added 5 commits June 12, 2026 13:57
@T-Gro
Use async (real CE) in CE-case test, not seq
19cd063
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add release notes entry for PR#19896
9dbaa2e
The CI check requires a release notes entry referencing this PR's URL.
Also corrects the issue reference from #5735 to #5418 in the existing
warning 20 entry (per review feedback on PR #19504).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace misleading CE test with while-loop + let binding test
0127569
The previous 'computation expression after let binding' test only used
async { return x } as a non-unit value - it didn't exercise CE-specific
syntax constructs. Replace with a while-loop test that genuinely exercises
the TcStmt lastExprRange fix in a different loop context.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merge remote-tracking branch 'origin/main' into copilot/fix-warning-2…
3d24264
…0-let-binding-range
@T-Gro
Merge branch 'main' into copilot/fix-warning-20-let-binding-range
e2189f6
@T-Gro
T-Gro requested a review from abonieJune 23, 2026 13:16
@T-Gro
T-Gro merged commit 1c73b9a into mainJun 23, 2026
57 checks passed
@github-project-automationgithub-project-automationBot moved this from In Progress to Done in F# Compiler and ToolingJun 23, 2026
This was referenced Jun 23, 2026
Merged
Merged
@T-Gro
T-Gro deleted the copilot/fix-warning-20-let-binding-range branch June 24, 2026 12:14
@T-GroT-Gro mentioned this pull request Jun 24, 2026
Merged
@dotnet-botdotnet-bot mentioned this pull request Jun 26, 2026
Closed
T-Gro added a commit that referenced this pull request Jul 1, 2026
@T-Gro
Add computation-expression tests for warning 20 range (follow-up to#…
fc3ee06
…19896) (#19992)
Answers @auduchinok's open question on PR #19896: in a computation
expression the loop body is desugared per-statement, so the trailing
expression reaches TcStmt on its own and warning 20 highlights only the
offending expression (e.g. 'x'), same as the non-CE case.
- async CE: asserts the squiggle lands on 'x' alone (Line 6, Col 13-14).
- seq CE: trailing non-unit value is an implicit yield, so no warning 20.
The CE test originally requested in review was removed in 0127569; this
reinstates a genuine CE case plus the seq implicit-yield contrast.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abonieabonieabonie approved these changes
+1 more reviewer
@auduchinokauduchinokauduchinok approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AI-reviewedPR reviewed by AI review councilAI-Tooling-Check-BypassedTooling check: non-fork PR, not diff-analyzed

Projects

F# Compiler and Tooling
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@T-Gro@auduchinok@abonie