Fix NullReferenceException in Entity on unlinked placeholder entities - #20274
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
Warning No PR link found in some release notes, please consider adding it.
|
NullReferenceException in Entity on unlinked placeholder entities
There was a problem hiding this comment.
@T-Gro this is another fix (I can move to another PR if you want)
There was a problem hiding this comment.
The Trailing Whitespace extension removed all trailing whitespaces.
I hope it is not an issue.
There was a problem hiding this comment.
So if you hide whitespaces you will see the real several lines changed
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.
d9c55d0 to
026148d
Compare
Summary
Fixes #20269 by making typed-tree remapping and classification resilient to placeholder/unlinked entities created while the file is in a broken mid-edit state. Instead of allowing a single null placeholder to crash the entire IDE classification pipeline, the compiler now treats absent module-type augmentation data as null and skips the remap/guard path safely.
Changes
TyconAugmentationandentity_modul_typevalues.TyconAugmentationshape.WeakMapcache update compatible across target frameworks while preserving existing behavior.NullReferenceExceptionbreaks IDE syntax coloring on unlinked placeholder entities (e.g. broken code mid-edit) #20269.