dotnet / dotnet/fsharp

Duplicate FS3261/FS3388/FS3262 warnings from two-phase overload resolution

Open
#19,782 0 comments 0 reactions 0 assignees View on GitHub
Area-Compiler-Checking Area-Diagnostics Area-Nullness
Dominant language
F#
Stars
4.3k
Forks
876
Avg merge
4d 11h
Merged PRs (30d)
131

Description

### Problem

`TcMethodApplication` resolves method calls in two phases. For single-candidate calls (the common case), Phase 2 re-runs constraint checks that Phase 1 already committed, producing duplicate warnings and wasted work.

```fsharp
// --checknulls --langversion:preview
let f (c: C | null) = c.P // FS3261 emitted TWICE
```

### Root cause

**Phase 1** (`UnifyUniqueOverloading`, `ConstraintSolver.fs:4008`): For exactly 1 candidate, runs `CanMemberSigsMatchUpToCheck` with `NoTrace` to guide argument type inference. Solutions committed permanently. Warnings emitted via `CommitOperationResult` (`CheckExpressions.fs:10465`).

**Phase 2** (`ResolveOverloadingForCall`, `CheckExpressions.fs:10487`): Always called. For single candidate, skips `ResolveOverloadingCore` entirely (line 3654: `| _, [calledMeth] when not isOpConversion -> Some calledMeth, CompleteD, NoTrace`), but the final commit block (line 3733) unconditionally re-runs the full `CanMemberSigsMatchUpToCheck`. Warnings emitted via `RaiseOperationResult` (line 10527).

### What's redundant vs. new in Phase 2's final commit

| Sub-step | Redundant? | Why |
|---|---|---|
| TDC1: `TypesEquiv` (instantiation) | Yes | `calledTyArgs = minst` reused from Phase 1 (line 10455→10479). `FreshenMethInfo` in `CalledMeth` ctor only freshens property setters, not the method's type params. |
| TDC2: `TypesMustSubsume` (obj-arg) | Yes | `callerObjArgTys` computed once (line 10394), shared by both phases. Source of duplicate FS3261/3262. |
| TDC7: `ReturnTypesMustSubsumeOrConvert` | Yes | Same `returnTy`, same method signature. Source of duplicate FS3388. |
| TDC3-6: `ArgsMustSubsumeOrConvert` + paramArray + named + setters | **No** | Phase 1 used synthetic `mkSynUnit` placeholders. Phase 2 has real typed `Expr` args and `enforceNullableOptionalsKnownTypes=true`. |

### Affected warnings

FS3261 (nullness mismatch), FS3262 (strict-null-required), FS3388 (return-type TDC). Any warning from TDC1/2/7 inside `CanMemberSigsMatchUpToCheck`.

### Fix

Add a `CanMemberSigsMatchArgsOnly` helper (or guarded branch in the final commit) that runs only TDC3-6. Thread `alreadyValidatedByPhase1: bool` (= existing `uniquelyResolved` value) through `ResolveOverloadingForCall` → `ResolveOverloading`.

**Guards** (conservative — exclude from fast path):
- `not isOpConversion` — op_Implicit/op_Explicit have special return-type handling
- `cx.IsNone` — SRTP trait resolution not run through Phase 1
- `calledMethTrace = NoTrace` — single-candidate path only

**Why safe:**
- Phase 1 warnings hit the logger before Phase 2 starts (`CommitOperationResult` at line 10465)
- Multi-candidate path untouched (uses `WithTrace` replay)
- Phase 1 failure → exception at 10465 → `alreadyValidatedByPhase1` never true on error path
- Arg checking (genuinely new work) still runs fully

**Constraint on correctness:** Missing a warning (0 instead of 1) is drastically worse than duplicating (2 instead of 1). This fix only skips checks whose results are already committed and whose warnings are already emitted. No warning can be lost.

### Rollout

Consider gating behind `LanguageFeature` flag for one release. Validate with `--times` on large compilations. Update baselines containing duplicated warning lines via `TEST_UPDATE_BSL=1`.

### Related

- #19658, #17409 — nullness warning range/context improvements

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.