Repository navigation
Fix NullReferenceException in Entity on unlinked placeholder entities - #20274
Conversation
✅ Release notes checked
|
xperiandri
changed the title
NullReferenceException in Entity on unlinked placeholder entities
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
Thanks for digging into this — resilient partial classification during mid-edit is a real, worthwhile goal, and the guarded remapTyconAug / entity_modul_type remap in TypedTreeOps.Remapping.fs are the right shape. A few blocking items before this can go in:
It doesn't build. Marking entity_modul_type and entity_tycon_tcaug as | null in TypedTree.fsi propagates nullability to consumers that weren't updated, and nullness warnings are errors here (outside Proto). A local -c Release build fails with FS3261 at:
TypedTreeOps.Transforms.fs(991)—d.entity_modul_type.Valueis the same unguarded deref you fixed inRemapping.fs, missed here.TypedTreePickle.fs(2818)—p_maybe_lazy p_modul_typ x.entity_modul_typepasses a nullable to a non-nullable target.TypedTreePickle.fs(2837–2852)— eightTyconAugmentation | nullincompatibilities in the tcaug pickle/unpickle path.
That's why every Build_And_Test_* leg is red. If you make these fields nullable you have to sweep all consumers, including the pickler.
~1360 lines of the TypedTree.fs diff are trailing-whitespace churn — only ~20 lines are real change. This trips CheckCodeFormatting, buries the actual fix, and will conflict with everything. Please revert the whitespace-only edits (and the lone one in lib.fs) so the diff is just the guards.
Scope of the type change. These fields were already effectively null for NewUnlinked() placeholders (Unchecked.defaultof<_>), so | null documents an existing latent invariant — but flipping two core Entity fields to nullable forces null-checks everywhere and is a big hammer for an IDE-resilience fix. Worth considering whether the dangling unlinked entity reaching the classification path is itself the bug to fix, rather than making the field type nullable tree-wide.
NewUnlinked() value changes are unnecessary and risky. Switching the placeholder to entity_logical_name = "<unknown>", entity_tycon_repr = TNoRepr, entity_typars = NotLazy [] changes observable pre-Link state on the metadata-unpickling hot path. IsLinked keys off entity_attribs (still defaultof), so linkage detection survives, but none of this is needed for the NRE fix — suggest reverting to Unchecked.defaultof<_>.
No regression test. The linked issue has a clean repro (break a type decl mid-edit); please encode it so this can't silently come back.
lib.fs WeakMap.TryAdd behind #if NETSTANDARD2_0 is correct and a nice cleanup, but it's unrelated to the NRE — fine to keep, just calling it out.
One more: the issue body references files/functions not in this diff (vsintegration/src/FSharp.Editor/*, NewModified*, FreeVars, a NullRef-TypedTree-Classification-Fix.md). Reconciling the description with the actual change will help reviewers follow along.
github-project-automation
Bot
moved this from New
to In Progress
in F# Compiler and Tooling
d9c55d0 to
026148d
Compare
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
One note on the null-fallback design (complements the root-cause point).
a9fc80b to
24f2fc9
Compare
github-actions
Bot
added
⚠️ Affects-Bootstrap
This comment has been minimized.
This comment has been minimized.
Always return initialized value for `Entity.entity_modul_type` and `Entity.entity_tycon_tcaug`.
Always return initialized value for `Entity.entity_modul_type` and `Entity.entity_tycon_tcaug`.
c3d9be5 to
4a4d62e
Compare
Always return initialized value for `Entity.entity_modul_type` and `Entity.entity_tycon_tcaug`.
4a4d62e to
ba36e07
Compare
The previous run was SIGKILL'd by the OOM-killer mid-suite (0 real test failures; 229 tests never ran). Empty commit to re-run the pipeline. Same exit-137 flake also hit unrelated PRs #20274 and #20235 at the same time. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Always return initialized value for `Entity.entity_modul_type` and `Entity.entity_tycon_tcaug`.
ba36e07 to
3f31b99
Compare
Head branch was pushed to by a user without write access
913f953 to
61d2a01
Compare
|
Description correction: the old pickle writer dereferenced the unlinked entity and failed before completing serialization. It did not silently serialize the entity. |
|
Current head |
61d2a01 to
7febdef
Compare
7febdef to
8e7f1ae
Compare
2cefc33 to
4080eed
Compare
|
🔍 Tooling Safety Check — Affects-Bootstrap, Affects-Compiler-Output, Affects-Design-Time
|
GetSemanticClassification runs under DiagnosticsScope.Protect, whose recovery returns an empty array for the whole file. One resolution naming a never-linked placeholder (from dotnet#20269) cost the file all of its colouring. Classify each resolution under its own RecoverableException handler so one bad symbol costs one token. OperationCanceledException still propagates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
594fc44 to
6705591
Compare
github-project-automation
Bot
moved this from In Progress
to Done
in F# Compiler and Tooling
* Keep single-file project options in a named record `FSharpProjectOptionsReactor.singleFileCache` stored each script or single-file entry as a 5-tuple `Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription`, destructured positionally at every consumer. It is now a private record, `SingleFileCacheEntry`, so the consumers name the fields they use and `addToCacheAndSubscribe` is a copy-and-update of the incoming entry. A reference record was chosen deliberately: a struct entry saves one heap object per cache write but, on .NET Framework where FSharp.Editor runs, copies 48 bytes out of the dictionary on every hit and forces `ConcurrentDictionary` to allocate a new node on update. The benchmark in the PR shows the reference record matching the reference tuple in time and allocations on both runtimes. Follows up on #20413 and the discussion in #20274. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit b19bedc) * Make ConnectionPointSubscription and the text-view lookups voption `ConnectionPointSubscription`, `subscribeToTextViewEvents`, `Document.TryGetIVsTextView`/`TryGetTextViewAndCaretPos`, and every `SingleFileCacheEntry.Subscription` touch point in `FSharpProjectOptionsManager.fs` used `option`; none of these values escape the editor's VS-interop layer. Switched to `voption`: the `Some` wrapper around the subscribed `IDisposable` is elided on every cache write, at the cost of copying one more field on `Hit`. The benchmark in the PR body has both readings for the reference record: with `option` and with `voption`, on a real subscription rather than the always-`None` path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit ddd0acb160452a6b84a1128f07d8aec4e988e0ba) * Add release note for #20455 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…ties (dotnet#20274) * Recover per resolution in GetSemanticClassification GetSemanticClassification runs under DiagnosticsScope.Protect, whose recovery returns an empty array for the whole file. One resolution naming a never-linked placeholder (from dotnet#20269) cost the file all of its colouring. Classify each resolution under its own RecoverableException handler so one bad symbol costs one token. OperationCanceledException still propagates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix dotnet#20274 (comment) * Revert `p_tcaug` improvement --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Keep single-file project options in a named record `FSharpProjectOptionsReactor.singleFileCache` stored each script or single-file entry as a 5-tuple `Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription`, destructured positionally at every consumer. It is now a private record, `SingleFileCacheEntry`, so the consumers name the fields they use and `addToCacheAndSubscribe` is a copy-and-update of the incoming entry. A reference record was chosen deliberately: a struct entry saves one heap object per cache write but, on .NET Framework where FSharp.Editor runs, copies 48 bytes out of the dictionary on every hit and forces `ConcurrentDictionary` to allocate a new node on update. The benchmark in the PR shows the reference record matching the reference tuple in time and allocations on both runtimes. Follows up on dotnet#20413 and the discussion in dotnet#20274. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit b19bedc) * Make ConnectionPointSubscription and the text-view lookups voption `ConnectionPointSubscription`, `subscribeToTextViewEvents`, `Document.TryGetIVsTextView`/`TryGetTextViewAndCaretPos`, and every `SingleFileCacheEntry.Subscription` touch point in `FSharpProjectOptionsManager.fs` used `option`; none of these values escape the editor's VS-interop layer. Switched to `voption`: the `Some` wrapper around the subscribed `IDisposable` is elided on every cache write, at the cost of copying one more field on `Hit`. The benchmark in the PR body has both readings for the reference record: with `option` and with `voption`, on a real subscription rather than the always-`None` path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit ddd0acb160452a6b84a1128f07d8aec4e988e0ba) * Add release note for dotnet#20455 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Fixes #20269
A broken, mid-edit file could leave an unlinked placeholder entity in the typed tree. Reading it during semantic classification threw a
NullReferenceExceptionthat discarded every colour in the file instead of just the one unresolved symbol, and the metadata writer would silently serialize such an entity instead of refusing it.