Fix #16419 at source: avoid double type-check of seq implicit-yield body#19895
Open
T-Gro wants to merge 2 commits into
Open
Fix #16419 at source: avoid double type-check of seq implicit-yield body#19895T-Gro wants to merge 2 commits into
T-Gro wants to merge 2 commits into
Conversation
…ield body Replace the HashSet-based dedup in TcResultsSinkImpl.NotifyFormatSpecifierLocation (added in PR dotnet#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>
Contributor
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
…ndCheck 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>
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #19791 addressing @auduchinok's review: the sink-level
HashSetdedup hid the real bug.In the implicit-yield branch of
tcSequenceExprBodyAsSequenceOrStatement, the body ofseq { e }was type-checked twice — once viaTryTcStmt(probe: is itunit?) and again viaTcExprFlex(as a yielded element). Both passes ran every side-effecting check insideTcExpr(format-string parsing, name-resolution notifications, …), which is why a single%dinseq { sprintf "%d" 1 }produced two sink entries.The second pass now reuses the existing
env.eCachedImplicitYieldExpressionscache thatTcExprSequentialOrImplicitYieldalready uses for thee1; e2shape, so the body is checked once. TheHashSetworkaround inTcResultsSinkImplis reverted.Fixes #16419.
Checklist