Add modern F# idiom instructions for Copilot / Claude Code - #20399
Add modern F# idiom instructions for Copilot / Claude Code#20399xperiandri wants to merge 9 commits into
Conversation
✅ No release notes required |
github-actions
Bot
added
the
⚠️ Affects-Agent-Config
This comment has been minimized.
This comment has been minimized.
21874a8 to
51316c8
Compare
|
Regarding
Maybe it is worth mentionning "unless something indicates not to do it", here is an example: |
|
@smoothdeveloper could you add a suggestion into a particular place of changed files? I have not got your comment. |
|
|
||
| - Prefer the `_.Property` shorthand in pipeline position: `tys |> List.map _.Type`. Complex expressions (`fun x -> x.Name = name`, `fun x -> x.A, x.B`) cannot use it. Never add a space – `_.MethodCall ()` breaks parsing. Unrelated to the `member _.Foo` self-identifier. | ||
| - Eta-reduce: `Seq.map (fun x -> someFunction x)` must become `Seq.map someFunction`. | ||
| - Several pipeline stages in a row over a `List`/`Array` allocate an intermediate collection each – route the chain through `Seq` and materialize once at the end. |
There was a problem hiding this comment.
🤖🕵️⏱️🔥
Suggestion: recommend one explicit pass here, not a Seq chain. A Seq chain still allocates one enumerator per stage and adds virtual dispatch.
// not: xs |> Seq.map f |> Seq.filter p |> Seq.sum
(0, xs) ||> List.fold (fun acc x -> let y = f x in if p y then acc + y else acc)Measured, 100 items, 3 stages:
| form | bytes | time |
|---|---|---|
List chain |
6309 B | 1009 ns |
Seq chain |
440 B | 936 ns |
single fold |
0 B | 280 ns |
The single pass allocates nothing and runs about 3 times faster than Seq. WDYT about recommending one traversal (fold, loop, or one comprehension) instead of Seq?
There was a problem hiding this comment.
Fixed in 2bf70a2: the guidance now prefers a single traversal and keeps Seq only as the readability fallback.
Auto-resolved by the GitHub Copilot app.
|
|
||
| ## Warning suppression | ||
|
|
||
| Inline `#nowarn "NN"` / `#warnon "NN"` pairs around the smallest possible scope – they are valid anywhere in an `.fs` file, not only at the top. File-level suppression is a last resort. |
There was a problem hiding this comment.
🤖🕵️ This file also applies to src/FSharp.Build, which targets F# 9. Nested #warnon produces FS3350 there. Restrict this rule to F# 10 projects or keep an F# 9-compatible form.
There was a problem hiding this comment.
Fixed in ea440d8: added a note that scoped #nowarn/#warnon is an F# 10 feature (LanguageFeature.ScopedNowarn, gated at languageVersion100), and that src/FSharp.Build (pinned to LangVersion 9) needs suppression at the top of the file instead — that's FS3350 there otherwise.
| - Prefer interpolated strings over `sprintf` and `String.Format`. Use `$"""…"""` when the text itself contains quotes. | ||
| - Format specifiers are valid in interpolated strings and help type inference: `$"count %d{n}"`. | ||
| - `nameof` over a string literal that names a value, member or type. | ||
| - An explicit `StringComparison` on every `Equals`, `StartsWith`, `EndsWith`, `Contains`, `IndexOf` and `Compare`, and an explicit comparer on every `HashSet<string>` and `Dictionary<string, _>`. `Ordinal` by default, `OrdinalIgnoreCase` for identifiers and paths. Culture-sensitive comparison is a decision, never a default. |
There was a problem hiding this comment.
🤖🕵️ For netstandard2.0:
text.IndexOf(value, StringComparison.Ordinal) >= 0Identifiers are case-sensitive; do not mandate OrdinalIgnoreCase for identifiers or paths.
There was a problem hiding this comment.
Fixed in 3a18c10: dropped Contains from the StringComparison list (no such overload on netstandard2.0 — confirmed src/ has zero calls of that shape) and added your IndexOf(value, StringComparison.Ordinal) >= 0 form, with XmlDocInheritance.fs as the existing example. Also narrowed OrdinalIgnoreCase to genuinely case-insensitive comparisons, not identifiers/paths.
| - Prefer a single traversal – one `fold`, loop, or comprehension – to a chain of transformations: it allocates nothing per element, where a chain allocates at every stage. | ||
| - When the chain reads better than one pass, route it through `Seq` and materialize once at the end – a `List`/`Array` chain allocates a whole intermediate collection per stage, a `Seq` chain only an enumerator. | ||
| - Concatenate with `[ yield! xs; yield! ys ]` / `seq { yield! xs; yield! ys }` rather than `@` or `Seq.append` – `@` forces both sides to lists and is O(n). | ||
| - Cast sequence items with `Seq.cast<Target>`, not `Seq.map (fun item -> item :> Target)`. |
There was a problem hiding this comment.
🤖🕵️ Seq.cast<T> performs runtime item casts; the shown upcast is statically checked. Reserve Seq.cast for intentionally untyped inputs and use covariance or a type annotation for upcasts.
There was a problem hiding this comment.
Fixed in b6f7901: Seq.cast is now scoped to genuinely untyped input, with the existing untyped uses in src/ (ServiceParsedInputOps.fs, fsihelp.fs) as examples, and a note that it unboxes per item and is unchecked ([1; 2; 3] |> Seq.cast<string> compiles and throws InvalidCastException at run time).
One correction on the suggested remedy: "covariance" doesn't compile here — strings :> seq<obj> fails with FS0193 (verified), because F#'s explicit upcast operator doesn't consult .NET's generic variance. The safe, allocation-free way to retype an already-covariant sequence is actually :?> (confirmed it returns the same instance for seq<string>/string list/string[], and correctly throws for seq<int> since variance only applies to reference type arguments) — but that reads as an unchecked downcast to anyone skimming it, so I didn't want to put it in the style guide without a caveat. Filed fsharp/fslang-suggestions#1470 to ask that :> itself be allowed to use variance, which would make this a non-issue. For now the rule just says "an upcast the compiler checks" and leaves it at that.
2bf70a2 to
f130d0c
Compare
|
🔍 Tooling Safety Check — Affects-Agent-Config
|
343b071 to
057d991
Compare
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ If this fixes an issue or implements an RFC/suggestion, link it (Fixes #... when applicable). Otherwise, give a short management-level summary in simplified technical English: what user scenario improves and what this achieves.
Please apply this PR-description guidance. Remove the implementation inventory already visible in Files, but keep necessary scope, compatibility, and dependency caveats.
github-project-automation
Bot
moved this from New
to In Progress
in F# Compiler and Tooling
LLMs are trained predominantly on pre-existing F# code, which skews toward older language versions and idioms (option over voption, sprintf over interpolation, tuples over struct tuples, isNull over match). Codify the constructs this codebase actually favors so agent-generated code reaches for the current idiom by default.
task{} does have a foothold on netstandard2.0: it's used in
Service/FSharpProjectSnapshot.fs and Service/FSharpWorkspaceQuery.fs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A single fold/loop/comprehension allocates nothing per element; a List/Array chain allocates an intermediate collection per stage and a Seq chain an enumerator per stage. Keep the Seq route as the fallback for when the chain reads better than one pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contains has no StringComparison overload on netstandard2.0 - src/ has no such call and uses IndexOf(value, Ordinal) >= 0 instead. Identifiers are case-sensitive, so OrdinalIgnoreCase was wrong guidance for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seq.cast takes a non-generic IEnumerable and unboxes per item, so it is unchecked: [1; 2; 3] |> Seq.cast<string> compiles and throws at run time. Every Seq.cast in src/ already sits on an untyped source; none of them retype a typed sequence, which is what the rule used to recommend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LanguageFeature.ScopedNowarn maps to languageVersion100, and this file also governs src/FSharp.Build, which is pinned to LangVersion 9 - a mid-file #warnon is FS3350 there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per @smoothdeveloper: CompilerImports.fs keeps an explicit lambda because the Type Provider SDK reflects over the closure and needs the captured field named tcImports; a method group would synthesize the receiver name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FSharp.Editor carries voption-returning counterparts of the FSharp.Core collection functions (Seq.tryFindV, Array.tryPickV, List.tryFindV, ...) in Common/Extensions.fs, and they are easy to miss: the module is [<AutoOpen>], so nothing at a call site names it. Without this, generated code reaches for the option-returning original and then converts, which is exactly what the voption rule above is trying to avoid. Also drops Seq.tryHead/List.tryFind as the examples of "an API hands you 'T option" in that rule - both do have a V counterpart in vsintegration, so they contradicted the new bullet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n last Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
436016c to
6d5b844
Compare
3 participants
AI coding agents are trained mostly on pre-existing F# code, so they default to older idioms —
optionovervoption,sprintf/String.Formatover string interpolation,isNullover matching onnull— that don't match what this codebase has already converged on, producing AI-assisted contributions that need stylistic rework even when the logic is correct.Copilot already reads
.github/instructions/*.instructions.md; this adds the missing one for F# construct choice, plus a mirror so other agents (e.g. Claude Code) pick up the same guidance.