Nullness FS3261: pinpoint receiver range, name the member & binding in dot-access warnings#19814
Open
T-Gro wants to merge 16 commits into
Open
Nullness FS3261: pinpoint receiver range, name the member & binding in dot-access warnings#19814T-Gro wants to merge 16 commits into
T-Gro wants to merge 16 commits into
Conversation
…ning (#19658, sprint 1/3) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… sprint 2/3) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ness warning (#19658, sprint 3/3) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tests - Extract withObjArgContext helper in ResolveOverloading and UnifyUniqueOverloading, replacing 3 inline match/rewrap duplicates - Trim MemberAccessOnNullable doc comment to 1 line (named labels self-document) - Remove redundant replaceNullnessOfTy (showNullnessAnnotations=false handles display) - Tighten release notes: 'dotted method or property access' (not indexer/records) - Add 5 new edge-case tests: chained access, mutable receiver, overloaded method, extension method, static call negative test - Clean up existing 3 tests: remove EntryPoint boilerplate, minimal source code Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sumesType (#19658) The MemberAccessOnNullable context is meant only for the receiver's outer nullness check. It was leaking into recursive subsumption/equality checks on tuple components, fun-type domains, and especially generic type arguments, producing a dot-access warning with the wrong type (the inner type-arg) and pinning unrelated deep nullness warnings to the receiver range. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#19658) Defense-in-depth: avoid setting ContextInfo.MemberAccessOnNullable on csenv when nullness checking is off, so future code that branches on this context case without re-checking g.checkNullness can't accidentally fire. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…line (#19658) Matches the convention of all sibling FS3261 handlers. When the receiver and the member access are on different source lines (fluent chains), the warning now hyperlinks the member range so IDEs surface both locations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ames (#19658) - tryGetBindingName recurses through Expr.Op(TOp.Coerce, ...) so binding names surface for interface-upcast receivers. - vref.IsCompilerGenerated check prevents internal names (_arg1, matchValue, copyOfStruct, ...) from leaking into user-facing messages. - Document why ConstraintSolverNullnessWarningOnDotAccess uses showNullnessAnnotations = Some false (avoid redundant '| null' phrasing). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…19658) - Property setter and piped lambda receiver: verify new message fires. - Indexer, F# record field, anonymous record, SRTP: pin current behavior of paths intentionally not routed through TcMethodApplication. Marked KNOWN GAP in test names. Widening coverage is tracked under #17409. - Anonymous record gap is documented via the FS3260 'does not support a nullness qualification' error since '{| ... |} | null' is rejected by the type system; this still pins that the dot-access path cannot fire. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the anonymous (range * string * string option) option tuple with a named ObjArgInfo record for readability, and extract the withObjArgContext closure (duplicated in ResolveOverloadingCore, ResolveOverloading, and UnifyUniqueOverloading) into a single top-level applyObjArgContext function. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inside the else-branch recursion, shadow csenv with the stripped variant so MemberAccessOnNullable context is dropped by default for deep subsumption descents. Keep the original under csenvOuter for the two sites that genuinely describe the outer ty1/ty2 pair: the same-tycon outer-nullness check, and the FindUniqueFeasibleSupertype recursion which still subsumes the original ty1. This eliminates the fragility of having to remember to use csenvInner on every new recursive branch — the safe behavior is now the default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…el (#19658) Merge seven template-identical Issue 19658 dot-access nullness tests into a single [<Theory>][<InlineData>] driven by (source, range, member, binding, type) tuples. The mechanical repetition added noise without helping readability. Rename the four 'KNOWN GAP' tests to neutral 'falls back to generic nullness warning' / 'rejects nullable annotation' wording. These tests pin the intentional scope boundary (paths NOT routed through TcMethodApplication) and are not bugs. Update the comment block above them to reflect this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#19658) Internal API change: ResolveOverloading and ResolveOverloadingCore no longer take an objArgInfo parameter. The public entry points ResolveOverloadingForCall and UnifyUniqueOverloading still accept the ObjArgInfo and set ContextInfo.MemberAccessOnNullable on the constructed ConstraintSolverEnv. CanMemberSigsMatchUpToCheck call sites locally strip the context for the non-obj-arg callbacks so deep arg/return mismatches keep using the generic FS3261 message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…es (#19658) CODE-QUALITY fixup: the strip-MemberAccessOnNullable pattern was inlined at 5 sites (SolveTypeSubsumesType + 4 overload resolution sites in ResolveOverloading). Extract a single helper near MakeConstraintSolverEnv and call it at all five callsites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use minimalStringOfTypeWithNullness instead of suppressing nullness annotations. The type 'string' was misleading — the actual type is 'string | null'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Entity.Typars was a method taking a range parameter that was passed to LazyWithContext.Force as context for error recovery. For IL-imported entities shared via FrameworkImportsCache, the first caller's range was cached permanently, creating non-deterministic typar ranges in parallel compilations. Analysis of git history (back to the first open-source commit) shows the range parameter was never meaningful: - F# entities use NotLazy for entity_typars — Force ignores the context - IL entities receive the import-time range via NewILTycon, stored in entity_range — the Force-time range was redundant Change Entity.Typars from a method to a property that uses entity.Range as the Force context. Remove the now-redundant TyparsNoRange member. Clean up cascading unused range parameters in callers. 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.
|
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
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.
Fixes #19658
Architecture
New
ContextInfo.MemberAccessOnNullablethreads receiver range + member name + optional binding name through constraint solving. When FS3261 fires during dot-access, a dedicatedConstraintSolverNullnessWarningOnDotAccessemits a targeted diagnostic instead of the generic "types do not have compatible nullability".Before → After
1. Simple method call on nullable receiver
x.PadLeft(10)—Nullness warning: The types 'string' and 'string | null' do not have compatible nullability.x—Nullness warning: Possible dereference of a null value when accessing member 'PadLeft' on the nullable value 'x' of type 'string | null'.2. Chained call — warning targets the actual nullable hop
x.Trim().Lengthandx.Trim()—Nullness warning: The types 'string' and 'string | null' do not have compatible nullability.x—Nullness warning: Possible dereference of a null value when accessing member 'Trim' on the nullable value 'x' of type 'string | null'.3. Multi-line receiver gets "See also" pointing at call site
.ToUpper()—Nullness warning: The types 'string' and 'string | null' do not have compatible nullability.data—Nullness warning: Possible dereference of a null value when accessing member 'ToUpper' on the nullable value 'data' of type 'string | null'. See also <line with .ToUpper()>4. Property chain — warning pinpoints where nullability starts
foo.Prop.ToString()—Nullness warning: The types 'Bar' and 'Bar | null' do not have compatible nullability.foo.Prop—Nullness warning: Possible dereference of a null value when accessing member 'ToString' on a nullable expression of type 'Bar | null'.Entity.Typars cleanup (included because it fixes a parallel test race exposed by the new tests)
Entity.Typarschanged from methodTypars(m: range)to property. Therangeparameter was unused for F# types (alwaysNotLazy— typars carry their own ranges from unpickle) and caused a parallel test race for IL-imported types (shared BCL entities viaFrameworkImportsCache+LazyWithContextcaching first caller's range). Now usesentity.Rangedeterministically. Removed redundantTyparsNoRange.