Warn FS3888 when consumer-visible attributes are missing from .fsi#19880
Open
T-Gro wants to merge 65 commits into
Open
Warn FS3888 when consumer-visible attributes are missing from .fsi#19880T-Gro wants to merge 65 commits into
T-Gro wants to merge 65 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev Caution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
AI-Tooling-Check-Bypassed
abonie
reviewed
Jun 4, 2026
abonie
reviewed
Jun 4, 2026
abonie
left a comment
abonie
left a comment
Member
There was a problem hiding this comment.
I'm okay approving as is, but worth considering if we can add a code fix for this error and/or make it a warning first (then upgrade to error later on)
…(1) happy path (issue #19560) Change the signature conformance enforcement introduced in PR #19880 from a hard error to a warning prefixed with 'This will become an error in future versions of F#' so existing libraries are not broken in-place. Refactor the enforcement so the happy path is a single O(1) bitmask check: combine every enforced flag with bitwise OR into one WellKnownValAttributes / WellKnownEntityAttributes mask, and only walk the per-attribute list (also O(1) per row) when the mask matches. Extend the enforced set beyond NoDynamicInvocation to cover every attribute whose presence on the signature is observed by the typecheck or compilation of separate consumer code. Per-Val: NoDynamicInvocation, RequiresExplicitTypeArguments, Conditional, NoEagerConstraintApplication, GeneralizableValue, WarnOnWithoutNullArgument, CLIEvent. Per-Entity (now also enforced on nested modules, not just types): RequireQualifiedAccess, AutoOpen, NoComparison, NoEquality, AbstractClass, Sealed (three-state), CLIMutable, AllowNullLiteral (three-state), DefaultAugmentation (three-state), Obsolete, CompilerMessage, Experimental, Unverifiable, EditorBrowsable, AttributeUsage, and CompilationRepresentation(UseNullAsTrueValue). Adding a new enforced attribute is now a one-line edit to a single in-code list; the all-up mask and per-attribute diagnostic are derived automatically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
Let's make it an error guarded with a language version? |
…19560) Replace the multi-line attribute-list bodies in SignatureConformance.fs with a tiny DSL that declares one rule per line: - EnforcedPhase (TypeCheck | CodeGen | TypeCheckAndCodeGen | Indirect) documents which consumer compile-stage actually reads the attribute, so reviewers can argue per-row. - valOne / valPair / entOne / entPair are the row constructors. Pair handles three-state bools (_True ||| _False). - V / E are short local type aliases for WellKnownValAttributes and WellKnownEntityAttributes so each row fits on one line. The list/mask shape and runtime semantics are unchanged: the all-up mask is still computed by folding the rows, the happy path is still a single O(1) HasWellKnownAttribute(mask) check, and the slow path still iterates the (small) row list emitting one warning per missing attribute. All existing tests pass (26/26 in Conformance.Signatures*). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y (issue #19560) Drop the EnforcedPhase tag, the per-row trailing comments, and the val/ent/pair helpers. The policy is now a plain list of (flag, name) tuples — one row per line — with V/E type aliases for terseness and a single fold to compute the O(1) early-exit mask. Behaviour unchanged (26/26 Conformance.Signatures tests pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ame (issue #19560) Each row is now a single enum value. The diagnostic name is derived from the enum case (`AutoOpenAttribute` -> `AutoOpen`, `SealedAttribute_True ||| _False` -> `Sealed` via lowest-set-bit). Dropped the CompilationRepresentation_PermitNull row because (a) the enum case does not match the user-written attribute name, and (b) it is IL-codegen-only. Behaviour unchanged for the 15 covered cases (all enforcement tests still pass). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…19560) Previously: 1 mask check on impl (fast), then a per-flag loop on EACH enforced row even when sig already carried the bits — slow for libs that did the right thing. Now: mask check on impl; on hit, mask check on sig (forces caching), then a single bitwise diff missing = impl & mask & ~(sig & mask). Per- attribute loop only runs when missing != 0 (true mismatch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#19560) Add a small `Flags` module in WellKnownAttribs.{fs,fsi} with set-op primitives over any uint64-backed enum: isEmpty, union, intersect, except, intersects, isSubsetOf. All inline, all reducing to single uint64 ALU ops after JIT. Use them in SignatureConformance.fs so the enforcement reads as set operations: let implOnEnforced = impl.Flags |> Flags.intersect enforcedMask let sigOnEnforced = sig.Flags |> Flags.intersect enforcedMask if not (implOnEnforced |> Flags.isSubsetOf sigOnEnforced) then let missing = implOnEnforced |> Flags.except sigOnEnforced for flag in policy do if flag |> Flags.intersects missing then warn flag Behaviour unchanged; 26/26 Conformance.Signatures tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ntInSig (issue #19560) The new names encode the asymmetry of the check directly: the impl side's enforced bits are what's required from the sig, and we compare against what is actually present in the sig. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560) Wrap the `HasWellKnownAttribute ... |> ignore` cache-populating side- effect into named helpers `enforcedFlagsOnVal` / `enforcedFlagsOnEntity` that return the flags intersected with the enforcement mask. Call sites now read as plain set operations with no `|> ignore` leak. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560) The Val and Entity enforcement blocks were structurally identical bar flag type, per-subject flag fetcher, policy list, and display-name accessor. Lift them into a single inline generic helper `checkEnforcedAttribs` parameterised on all four. Val and Entity specialisations are one-line partial applications. Inline + 'F : enum<uint64> inference keeps the bit math zero-cost. 26/26 Conformance.Signatures tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ormance module + range/placement tests (issue #19560) - Add `LanguageFeature.ErrorOnMissingSignatureAttribute` (preview). When the language version supports it, FS3888 escalates from warning to error; otherwise it stays a suppressible warning. - Extract the policy and matching logic into nested `module private AttributeConformance =` inside SignatureConformance. The Checker now calls `AttributeConformance.checkVal` / `checkEntity` and is no longer entangled with the bit math. - Point the diagnostic squiggle at the offending attribute in the .fs (via `Attrib.Range`) instead of the value/type identifier, so the IDE highlights exactly the attribute the user must mirror. - Add tests for: module-level attribute, diagnostic placement on the attribute (range start/end on the attribute's line, not the val/type identifier line), and language-feature-driven escalation to error. 19/19 enforcement tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#19560) Adds `AddMissingAttributeToSignatureCodeFixProvider` for FS3888. The diagnostic is reported on the attribute in the .fs; the fix inserts the same attribute text into the .fsi above the matching declaration, preserving the sig line's indentation. Cross-document mechanics (no prior art in this repo - `RenameParam- ToMatchSignature` is sig-aware but rewrites the local .fs only): - Locate the symbol the attribute attaches to via lexer lookup at the next non-whitespace position after the diagnostic span. - Resolve the sig location via `FSharpSymbol.SignatureLocation`. - Find the .fsi document in the solution by file path. - Compute insertion: start of the sig declaration's line + leading whitespace from that line for the new attribute line. - Apply via `CodeAction.Create` with a `ChangedSolution` producer (Roslyn's `Document.WithText` -> `Project.Solution`). Modeled after `MissingReference.fs` (the only existing fix that overrides `RegisterCodeFixesAsync` directly and uses `cancellableTask` + `CancellableTask.startAsTask`). VS integration is Windows-only; cannot be built on macOS due to missing .NETFramework 4.7.2 reference assemblies. Windows CI will verify the build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ssue #19560) The diagnostic range (Attrib.Range / SynAttribute.Range) covers ONE attribute body WITHOUT the surrounding `[< >]` brackets and without sibling attributes in a `[<A; B>]` list. So the verbatim copy gives `NoDynamicInvocation(false)` - the code-fix now wraps with `[< >]` before inserting into the .fsi. The skip-forward loop after the diagnostic span now also skips `>`, `]`, `;` to land on the val/type/member ident correctly when the diagnostic targets one attribute inside a `[<A; B>]` list. Tests (vsintegration/tests/.../CodeFixes/AddMissingAttributeToSignatureTests.fs) cover: module-level (AutoOpen on nested module), type-level (RequireQualifiedAccess on union), function-level (NoDynamicInvocation on val), attribute-with-argument (AllowNullLiteral(false) - exercises verbatim arg copying). The existing CodeFixTestFramework assumes IFSharpCodeFixProvider with single-doc TextChange list, which doesn't fit a cross-document fix. Tests use a small inline harness that invokes RegisterCodeFixesAsync directly, captures the CodeAction, applies it via ApplyChangesOperation and diffs the .fsi document in the resulting Solution. VS integration cannot be built on macOS (missing .NETFramework 4.7.2 reference assemblies); Windows CI verifies the build + tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ribute tests (issue #19560) Addresses adversarial review findings: 1. Symbol-lookup was unreliable (skip-forward landed on `let`/`type` keywords or sibling attributes in [<A; B>] / [<A>][<B>] cases). Replace with check-results-based enumeration: iterate `GetAllUsesOfAllSymbolsInFile`, filter for `IsFromDefinition && Symbol.SignatureLocation.IsSome`, pick the first whose range starts on/after the diagnostic attribute's end position. Locates the val/ type/module regardless of attribute syntax. 2. EOL: use the .fsi file's existing line break (from the target line's `EndIncludingLineBreak` span) instead of `Environment.NewLine`, so LF-only files stay LF and CRLF files stay CRLF. 3. Path comparison: case-insensitive only on Windows; case-sensitive on POSIX so Roslyn matches behaviour of the filesystem. 4. Equivalence key now includes the sig location to prevent collision when the same attribute text is missing on multiple unrelated decls. 5. Tests extended with multi-attribute coverage: - Two stacked [<A>]\n[<B>] both missing -> two independent fixes. - Two on one line [<A; B>] both missing -> two independent fixes. Each insertion is its own [<X>] (per-attribute wrap by design). - Mixed enforced + non-enforced on same line - only enforced inserted. - .fsi already carries a non-enforced attribute -> new one added, existing one preserved. - Test harness now exposes `tryFixSigAt diagIndex` so multi-diag scenarios can be exercised. Compiler-side: 30/30 Conformance.Signatures tests still pass. VS integration build is Windows-only (.NETFramework 4.7.2 reference assemblies missing on macOS); Windows CI verifies the code-fix + tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…issue #19560) CI build of FSharp.Editor failed on Windows: vsintegration/src/FSharp.Editor/CodeFixes/AddMissingAttributeToSignature.fs(201,68): error FS0039: The type 'FSharpSymbolUse' is not defined in 'FSharp.Compiler.Symbols'. The type lives in 'FSharp.Compiler.CodeAnalysis' (declared at src/Compiler/Service/FSharpCheckerResults.fs:212 in namespace FSharp.Compiler.CodeAnalysis). Updated the fully-qualified type annotation accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…issue #19560) The CodeAction.Create lambda used cancellableTask {} with a nested match + let! pattern. Compiling under LangVersion=preview the F# compiler rejected line 248 with FS3401 'resumable code __resumableEntry may only be used in inlined code'. The state-machine generator for cancellableTask struggled with the nested match-arm shape combined with capturing the outer cancellationToken parameter. Refactored to a named helper function with a plain 'task {}' CE that takes the CancellationToken explicitly and returns Task<Solution> directly, which matches the CodeAction.Create overload signature unambiguously and avoids resumable-code issues. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ssue #19560) For attribute placed on a method inside 'type T() = ...': type T() = [<NoDynamicInvocationAttribute>] member _.F(x: int) = x + 1 The F# checker reports a definition-symbol use for the implicit primary constructor at the member's line in addition to the member itself. The constructor's SignatureLocation in the .fsi points at the 'new: unit -> T' line, while the member's points at the 'member F: ...' line. The previous tie-breaking by (line, col, end-line, end-col, name) picked the constructor (its use has a lower column index), so the attribute was inserted above 'new:' instead of above the member sig. Filter constructors out of the candidate set so the member is always selected when an attribute is attached to a member. Also runs fantomas auto-format and uses a plain task CE rather than cancellableTask in the CodeAction lambda to avoid FS3401 resumable-code issues under LangVersion=preview (already in previous commit; verified green). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…19560) Previous filter (constructors only) did not fix the failing tests for attribute-on-member-inside-type. The F# checker returns symbol uses whose SignatureLocation points at the .fs file (self-bindings, parameters, etc.) rather than the .fsi sig. When such a symbol was picked as the minBy winner, tryFindSigDocument resolved to the .fs doc and the fix would attempt to insert into the .fs (no-op on the .fsi the test reads). Add a filter that requires SignatureLocation.FileName to differ from the diagnostic document's FilePath (case-insensitive). This guarantees the picked candidate truly points cross-document into the signature file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lution (issue #19560) Previous code used document.Project.Solution.Workspace.CurrentSolution to look up the .fsi document at apply time. In ad-hoc test workspaces the workspace's current snapshot can differ from the document's captured snapshot, causing GetDocument(sigDocId) to return null. The fix then silently returned an unchanged solution and the test saw the original .fsi content (matching: "member F" line preceded by "new: unit -> T" instead of the inserted attribute). Use document.Project.Solution directly. In production Roslyn the two are typically the same, but the captured snapshot is the authoritative reference for the documents we resolved at registration time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion Throws with sigDocId / sigRange details when either GetDocument(sigDocId)=null OR tryFSharpRangeToTextSpan returns None. This will surface the failure mode in test output so we can fix the real bug. To be reverted once the cause is identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
If the picked symbol's SignatureLocation FileName equals the diagnostic document's FilePath, the fix would modify the .fs (not the .fsi). The test would silently see the .fsi unchanged. Surface this case with a failwithf so the test output shows the picked symbol and paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…bols (issue #19560) ROOT CAUSE (confirmed via instrumented CI run f20774e): For an attribute on an instance member like: type T() = [<NoDynamicInvocationAttribute>] member _.F(x: int) = x + 1 The F# checker reports a definition-symbol use for the wildcard self identifier _ at line 5 col 11 with IsFromDefinition=true and SignatureLocation=Some pointing to its own .fs position. Because col 11 is lower than the member-identifier F (col ~14), the minBy tie-break selected _, and the code-fix then attempted to insert the attribute into the .fs (sigDoc resolved to the same .fs document by path lookup). The test reads the .fsi back and sees it unchanged. Add a filter: SignatureLocation.FileName must differ from the .fs document's FilePath (case-insensitive). This excludes wildcard self-identifiers and any other 'self-pointing' definition symbol. The actual cross-file F member symbol survives the filter and is picked correctly. Also adds a guard on the outer match so the same condition is re-checked when binding sigRange — defense in depth in case the filter passes but the picked symbol's SignatureLocation still points back at the .fs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e M3 (issue #19560) Test scenario: // (in impl, NOT in sig) [<System.ObsoleteAttribute("Attribute is in implementation but not signature")>] type C3 = A | B module M3 = ... My new FS3888 enforcement reports Obsolete missing from the .fsi signature for these entities. The .fs comment 'expect no warning' is now stale for the entity cases (type / module) since Obsolete is on the enforced-entity attribute list. Val cases (x3) are NOT affected because Obsolete is intentionally NOT in the enforced-val list — vals inherit their attribute from the signature, but the .fs comment remains accurate there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
T-Gro
commented
Jun 8, 2026
…, fix compiler signatures (issue #19560) - SignatureConformance.emitter now gates on LanguageFeature.ErrorOnMissingSignatureAttribute (errorR under preview, warning otherwise) instead of always warning. - FSComp.txt FS3888 message no longer pretends 'will become an error in future versions of F#' - it IS an error under preview. - Strip every PR-introduced suppression: NoWarn / WarningsNotAsErrors / OtherFlags --nowarn:3888 in FSharp.Compiler.Service.fsproj and FSharp.Core.fsproj; #nowarn "3888" in il.fs / sformat.fs / range.fs / FileSystem.fs / XmlDoc.fs / SymbolPatterns.fs / ServiceCompilerDiagnostics.fs / ServiceDeclarationLists.fs. - Fix the asymmetries the suppressions were hiding by changing real product code: - Mirror to .fsi where the impl attribute is the intended contract: NoEquality;NoComparison on ServiceDeclarationLists.MethodGroupItem and il.ILGenericParameterDef; RequireQualifiedAccess on ServiceNavigation.Navigation / NavigateTo, ExternalSymbol.FindDeclExternalParam, SymbolPatterns.FSharpSymbolPatterns, ServiceCompilerDiagnostics.CompilerDiagnostics; AutoOpen on sformat.TaggedText and range.Position. - Remove from .fs where the attribute was a dead/sloppy addition: RequireQualifiedAccess on il.ILAttribute (DU cases used unqualified in EraseUnions.fs / ilread.fs) and on XmlDoc (meaningless on classes); Experimental on FileSystem ByteMemory / IAssemblyLoader / DefaultAssemblyLoader / IFileSystem / DefaultFileSystem (would unnecessarily warn every existing FCS consumer). - Migrate consumers of bare 'equals' to 'Range.equals' in files that 'open FSharp.Compiler.Text' (now that TaggedText is properly auto-opened): DiagnosticsLogger.fs, Driver/CompilerDiagnostics.fs, Driver/CompilerConfig.fs, CodeGen/IlxGen.fs, Service/ServiceNavigation.fs. - Test updates: drop stale 'will become an error' message checks; add two tests covering the langversion gate (error under preview, warning under 9.0). Update neg31.bsl to match the new message text. After this commit, ./build.sh -c Release runs with FS3888 as a real langversion-gated error and 0 warnings, 0 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace 'Code that consumes this signature depends on this attribute, so add it to the signature. Tooling skips the implementation when a signature file is present.' with the wording suggested in review: 'in the signature, which takes precedence for tooling and consumers. Add the attribute to the signature, to ensure the attribute is not ignored by the compiler.' Adjusts FSComp.txt, all 13 xlf translations (auto-regenerated by build), and the neg31.bsl baseline. Test assertions only match attribute names so they are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…de-fix (issue #19560) SignatureConformance.fs: * Drop sigVal.Accessibility.IsPublic / sigEntity.Accessibility.IsPublic gate. Internal symbols also drive cross-file and InternalsVisibleTo typechecking, so the same .fs/.fsi attribute divergence applies and was silently exempt. * Expand enforcedVals: + ExtensionAttribute. * Expand enforcedEntities: + StructuralEquality, StructuralComparison, CustomEquality, CustomComparison, ReferenceEquality, IsByRefLike, IsReadOnly, Extension, Measure, Struct, Class, Interface. Scope limited to attributes that affect typechecking, name resolution, overload resolution, or codegen contract observed by consumers; pure-runtime attributes (DllImport, ReflectedDefinition, SkipLocalsInit, COM, ...) are intentionally not enforced. Tests: * Internal type with attribute mismatch still fires FS3888. * Internal val with attribute mismatch still fires FS3888. * StructuralEquality / IsReadOnly / Struct mismatch sites. Reverse code-fix: * vsintegration/src/FSharp.Editor/CodeFixes/RemoveExtraAttributeFromImplementation.fs same-document edit, complements AddMissingAttributeToSignature. Handles lone [<X>] (deletes the bracket and trailing line break + indent), first/middle/last sibling in [<A; B; C>] (deletes the body and one neighbouring '; '), declines on unrecognized bracket layouts rather than risking corrupt edits. * 5 unit tests in vsintegration/tests/FSharp.Editor.Tests covering lone, first-sibling, second-sibling, attribute-on-type-with-body, attribute-with-arguments. Release notes updated for FSharp.Compiler.Service and VisualStudio. The F# self-build still goes through with 0 warnings, 0 errors after both changes - adding the new attributes and dropping the public-only gate did not surface any new mismatches in the compiler codebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
auduchinok
reviewed
Jun 9, 2026
Audit of src/Compiler/Checking/ consumption sites surfaced two more typechecking- affecting val attributes: - ParamArrayAttribute - consumed in ConstraintSolver, MethodCalls, CheckExpressions to decide whether a varargs call site like foo(1, 2, 3) is legal. Divergence silently rejects valid consumer code (the .fsi-derived signature doesn't advertise the ParamArray). - LiteralAttribute - consumed in PostInferenceChecks and ConstraintSolver. A literal binding is usable in 'match' patterns as a constant. If the .fsi omits it, the consumer's pattern match becomes an invalid literal pattern. Self-build still 0 warnings, 0 errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per #19880 (comment) (auduchinok): keep only comments that explain non-evident logic. Cut policy/intent prose, restated-from-code descriptions, and 'why we chose X' war stories that obscure the code. Net: 123 fewer comment lines across SignatureConformance.fs, WellKnownAttribs (Flags module), the cross-document and reverse code-fix providers, AbstractIL/il.fs, and the component tests. No behavior change. All 36 signature-conformance tests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
T-Gro
changed the title
Drop bloat across release notes: the inline enforced-attribute list, the
'internal vs public' aside, and the runtime-skip clause are not consumer-
facing release-note material - they belong in the diff or the issue.
- FSharp.Compiler.Service: 25-line paragraph -> 3 sentences.
- Language preview: trim one-line entry; mention what FS3888 is so the
bullet stands on its own.
- VisualStudio: collapse two 4-line code-fix bullets into one self-standing
bullet ("copy the attribute into the .fsi, or remove it from the .fs").
- FSharp.Core: drop the entry entirely - it was an internal mirror change
with no consumer-visible behavior delta.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Captures recurring review feedback distilled from 3 weeks of sessions: - Good names beat comments. Don't restate the code, don't narrate the algorithm, don't justify decisions inline, no war-story comments. - Compact idiomatic F#: pattern matching, active patterns, extracted helpers, low cyclomatic complexity, new file > bloating a huge file. - Tests: parametrized > copy-pasted, module-level shared constants, helpers like parseAndCheck. - PR scope: not paid by LOC. Cleanup commits separate from feature commits. No 'phase tag' / 'transitional measure' breadcrumbs. Auto-applied via applyTo frontmatter to src/Compiler, vsintegration/src, and the F# test suites. Verified the YAML parses identically to the existing ExpertReview / ComponentTests instruction files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#19560) main brought in new bare 'equals m range0' call sites in CodeGen/IlxGen.fs (DebugPoint handling for both Expr and LinearExpr) and Checking/NameResolution.fs (TypeVar range comparison) after this branch added [<AutoOpen>] to TaggedText.fsi. With TaggedText auto-opened, bare 'equals' now resolves to the TaggedText value, not Range.equals. Qualify the three sites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rom-main attribs (issue #19560) After merge from main, several new types/attributes surfaced FS3888 across the F# self-build. Adjustments: SignatureConformance.fs: * Skip the check when the sig declares the entity with hidden representation (e.g. `type internal T` opaque in .fsi vs DU in .fs). Attribute-mirroring is syntactically impossible on opaque sig declarations. * Drop AutoOpen from the enforced set: a legitimately asymmetric idiom on internal modules (auto-open within the project, opaque for InternalsVisibleTo consumers and for the .fsi-visible surface). * Drop StructuralEquality / StructuralComparison from the enforced set: they are documentary, matching the F# default for DU/record/struct types; their presence on .fs and absence on .fsi has no observable consumer effect. Mirror impl attribs into the matching .fsi where the sig was simply missing them: NoEquality/NoComparison on DisplayEnv (TypedTreeOps.FreeVars.fsi) and ExprRewritingEnv (TypedTreeOps.Transforms.fsi); RequireQualifiedAccess on LexerEndlineContinuation (ParseHelpers.fsi). il.fs: remove the dead [<RequireQualifiedAccess>] from ILSecurityDecl (the DU case is used unqualified in ilwrite.fs). Tests: * AutoOpen tests now assert no FS3888 (asymmetric is intended). * StructuralEquality test now asserts no FS3888 (documentary attribute). Self-build clean after `git clean -xfd artifacts` (stale binaries from prior attempts had the old AutoOpen attribute baked in - clean rebuild required). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dAccess>] (issue #19560) The two failing tests in WindowsCompressedMetadata vs_release used [<AutoOpen>] on a module to trigger FS3888 and verify the cross-document fix. AutoOpen was intentionally removed from the enforced attribute set (legitimately asymmetric on internal modules), so the diagnostic no longer fires for these inputs and the code-fix has nothing to do. Swap to [<RequireQualifiedAccess>] which IS enforced - the codefix-on-module coverage is preserved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…odules) (issue #19560) The previous hidden-repr skip used `not sigEntity.IsHiddenReprTycon` to avoid firing on opaque type declarations (`type internal T` in .fsi vs DU in .fs). But IsHiddenReprTycon also returns true for modules, which incorrectly silenced RequireQualifiedAccess enforcement on modules - that broke the `Module-level: ...` VS code-fix tests because the diagnostic never fired so the fix had nothing to do. Refine: check modules unconditionally, only skip non-module hidden-repr tycons. Adds a conformance test that verifies RequireQualifiedAccess on a nested module in impl but not sig actually fires FS3888. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ules (issue #19560) After the previous commit started enforcing RQA on modules (regression from the over-broad IsHiddenReprTycon skip), the F# self-build surfaced four internal modules with RQA on .fs but not .fsi: - DiagnosticsLogger.fsi: module OperationResult - PrettyNaming.fsi: module internal CustomOperations - WarnScopes.fsi: module internal WarnScopes - FileSystem.fsi: module internal FileSystemUtils All call sites were already qualified, so adding RQA to the .fsi is a no-op behaviour-wise. Self-build clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… qualify call sites (issue #19560) These two DU types had RQA in the .fs (intentional - the cases Encoded/Decoded and the single ILSecurityDecl case are generic enough that consumers should qualify). I had removed RQA earlier to silence FS3888 without migrating the unqualified call sites - that defeated the whole point of the attribute. Restored RQA on both types in .fs and .fsi; migrated: - src/Compiler/CodeGen/EraseUnions.fs Encoded(...) -> ILAttribute.Encoded(...) - src/Compiler/AbstractIL/ilprint.fs ILSecurityDecl(...) -> ILSecurityDecl.ILSecurityDecl(...) - src/Compiler/AbstractIL/ilread.fs ILSecurityDecl(...) -> ILSecurityDecl.ILSecurityDecl(...) - src/Compiler/AbstractIL/ilwrite.fs ILSecurityDecl(...) -> ILSecurityDecl.ILSecurityDecl(...) XmlDoc is a class, not a DU; RQA on classes is a no-op so the earlier removal stays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560) I had removed it from sformat.fs IEnvironment earlier to silence FS3888, which is precisely backwards - the attribute was deliberately there. Restore on both .fs and .fsi. The earlier attempt at this same fix appeared to break the test framework, but that turned out to be stale artifacts from a prior build with [<AutoOpen>] on Layout. A clean rebuild verifies the fix is clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… set Replace the vague 'legitimately asymmetric idiom' justification with the empirically-verified reasons: AutoOpen on .fs alone is a no-op for F# consumers - the .fsi governs the FSharpSignatureData consumers read, the IL attribute is ignored. (Proven with a 4-variant fs-only/fsi-only/both/neither test against an external consumer.) StructuralEquality/StructuralComparison are documentary at the consumer boundary - the F# default for DU/record/struct already generates the same Equals/GetHashCode/CompareTo and IStructural* interfaces. The attribute IS load-bearing as a compile-time assertion (FS1176/FS1177 when a field doesn't satisfy equality/comparison) but that fires at the .fs's own compile, not at consumers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
3 participants
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 #19560
Tooling and consumer code typecheck against the
.fsi, not the.fs.Compiler-semantic attributes (e.g.
[<NoDynamicInvocation>],[<AutoOpen>],[<IsByRefLike>]) that live only on the implementation are silently dropped,so the contract the consumer typechecks against diverges from the one emitted
at runtime. FS3888 now flags every paired
.fs/.fsisymbol whose impl carriesa typecheck-affecting attribute the signature lacks; under the
ErrorOnMissingSignatureAttributepreview language feature it is an error.Two VS lightbulbs are offered on the diagnostic: add the attribute to the
.fsi, or remove it from the.fs.