Conversation
Breakpoints inside 'for x in xs -> body' (arrow syntax in list/array/seq comprehensions) failed to bind because the SeqMap active pattern in LowerComputedCollections.fs did not preserve debug point wrappers. Fix: wrap body extraction in DebugPoints and return 'debug body', matching the existing SeqCollectSingle pattern. Add 3 tests verifying debug points exist on body expressions for list arrow, array arrow, and do-yield comprehension forms.
range (e.g., 'return 1' not just 'return') by using mFull instead of keyword-only range m in DebugPointAtLeafExpr.Yes - Fix use binding in CE to use DebugPointAtTarget.No on the SynMatchClause to avoid generating an extra sequence point from the match target - Add VerifyMethodSequencePoints test infrastructure to verify sequence points for specific methods (e.g., Invoke, MoveNext, GenerateNext) - Add CEDebugPoints test module with 3 tests validating the fixes
- Fix compiler: use full binding range (mBind) instead of keyword-only range (leadingKeyword.Range) in mkSynCall for 'use' CE translation, eliminating the use-keyword-only sequence point in state machine output. - Extract verifyCEMethodDebugPoints helper to reduce test boilerplate. - Change use test from async/Invoke to task/MoveNext for single-method SP validation in the state machine. - Update expected SP values to match actual compiler output. - All 3 CEDebugPoints tests pass; PortablePdbs, ForArrowDebugPoints, and AsyncExpressionStepping regression tests pass.
…harp into bugfix/sequence-points
T-Gro
force-pushed
the
bugfix/sequence-points
branch
from
38c2f6e to
1ce8e10
Compare
…oc, and echo comments Addresses NO-LEFTOVERS verifier feedback: - Remove banner-style section separators in test helpers - Remove xmldoc that restates function names - Remove comments echoing code below them in test assertions - Condense verbose implementation xmldoc to issue URLs - Fix tryFixupSpan -> FixedSpan reference in ClassificationService.fs - Fix duplicate URL in FindReferences.fs #line directive test
The function is only called internally within NameResolution.fs, so it should not be exposed in the .fsi file. This minimizes the module's public API surface as flagged by CODE-QUALITY verifier.
🔧 CLI Command Report
✅ Patch applied: |
|
@T-Gro Thanks for working on it! I usually try to go through the PRs in this repo and to leave a comment if something doesn't look correct. With these recent big PRs I find it very difficult to review them when multiple unrelated changes get mixed. Could you consider working on mutually independent fixes in separate PRs in the future? I'm afraid otherwise there would much less chance that someone else reviews them carefully enough to spot an issue. In addition to the possible creeping in bugs it makes it almost impossible to learn the codebase from these PRs. With independent PRs you can easily see what changes were needed for a fix and where and then try learn from it. With tens of fixes mixed up, like in #19279, the time needed to even isolate the changes is just not worth it, which is really sad. |
|
@auduchinok : Thanks for keeping an eye on the changes, point taken. I want to leverage coupling potential as there sometimes is a hiding shared cause for multiple issues, or at least some overlapping concerns. Please ignore the "WIP .." one - indeed there is not much correlation, it's more on the experimental side. Based on your point, even if/when I get that one to success, there it will be appropriate to split it prior to marking it "Ready for review". What is your view on this one in particular? |
|
/run ilverify |
🔧 CLI Command Report
✅ Command succeeded, no changes needed. |
3 participants
Debug point / sequence point fixes
Fixes Extra sequence point at the end of match expressions #12052 — Extra sequence point at end of
matchexpressions caused debugger to briefly stop at unrelated branches during Step Over. Fixed by always emitting hidden code at decision tree join points inIlxGen.fs, not only when the stack is non-empty. Also covers theif/elif/elsecase mentioned in the comments. Tests:MatchEndSequencePoint.fs(5 tests including if/then/else).Fixes Wrong sequence point range for
returninsideasynccomputation expression #19248 —return,yield,return!,yield!in CEs had debug points covering only the keyword range instead of the full expression (e.g.returninstead ofreturn 1). Fixed by using the full expression range (mFull) forDebugPointAtLeafExprinCheckComputationExpressions.fs. Tests:CEDebugPoints.fs(6 tests covering return, yield, return!, yield! in async/task/custom CEs).Fixes Extra out-of-order sequence point for
useintaskcomputation expressions #19255 —usebindings intaskCEs emitted an extra out-of-order sequence point (nop at theuseline beforeleton the prior line), making execution appear to jump backwards. Fixed by usingmBindinstead ofleadingKeyword.Rangefor theUsingcall and switchingDebugPointAtTargettoNo. Tests:CEDebugPoints.fs(Use in task CEtest).Fixes Debug points failing to bind in body of "[ for x in xs -> body ]" #13504 — Debug points failed to bind in
[ for x in xs -> body ]arrow comprehensions. TheSeqMappattern inLowerComputedCollections.fswas stripping debug points from the loop body during lowering. Fixed by preserving them viaDebugPointsactive pattern. Tests:ForArrowDebugPoints.fs(3 tests: list, array, and for-do-yield contrast).