Conversation
|
|
||
| ### Changed | ||
|
|
||
| - We now expose stable backward-compatible APIs, independent of the FSharp.Compiler.Service API, which you are encouraged to use. If you use the stable API, then upgrading FSharp.Analyzers.SDK will not require you to rebuild your analyzers until you need to consume features from later versions of the SDK. (For example, if a new F# language version introduces new syntax that you wish to parse, then you will have to upgrade your analyzer to a later stable API version which understands that new syntax.) |
There was a problem hiding this comment.
As of f0e4c3d this is the only part of the code that I wrote. (It's also all I've even read, so far.)
| <ProjectReference Include="..\..\src\FSharp.Analyzers.SDK.Testing\FSharp.Analyzers.SDK.Testing.fsproj" /> | ||
| <ProjectReference Include="..\..\src\FSharp.Analyzers.SDK\FSharp.Analyzers.SDK.fsproj" /> | ||
| <ProjectReference Include="..\OptionAnalyzer\OptionAnalyzer.fsproj" /> | ||
| <ProjectReference Include="..\OptionAnalyzer.V1\OptionAnalyzer.V1.fsproj" /> |
There was a problem hiding this comment.
Cloned the existing OptionAnalyzer but now coding it against the stable API rather than against the raw FCS API.
| @@ -0,0 +1,818 @@ | |||
| module internal FSharp.Analyzers.SDK.AdapterV1 | |||
There was a problem hiding this comment.
FSharp.Analyzers.SDK has an FCS-derived view of the world. In order to call out to the user-supplied analyzer, it goes through an adapter to represent the world in terms of the stable V1 types.
| Release|x86 = Release|x86 | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE |
There was a problem hiding this comment.
This appears to be the default, not that it seems to be documented anywhere. Anyway, Claude chose for inscrutable reasons to move it down the file.
There was a problem hiding this comment.
We should move to slnx anyway
| |> Array.tryFind (fun n -> n.GetType().Name = typeof<'TAttribute>.Name) | ||
| |> Option.map unbox<'TAttribute> | ||
| |> Option.bind (fun attr -> | ||
| try |
There was a problem hiding this comment.
This was always wrong: if a user defines an attribute with the same name as an FCS one, this would break on main.
| GenericParameters: GenericParameterInfo list | ||
| } | ||
|
|
||
| and TypedExpr = |
There was a problem hiding this comment.
Claude was of the opinion, and I agree, that the AST-walking visitor style is less ergonomic than simply exposing the DUs. We could expose tree-walking style APIs, though, if we wanted to make migration easier.
| else | ||
| [] | ||
|
|
||
| // Legacy analyzers: version check required. |
There was a problem hiding this comment.
Not so keen on calling this "legacy" - it's really "bleeding-edge will-break-at-the-drop-of-a-hat".
| <Tailcalls>true</Tailcalls> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <InternalsVisibleTo Include="OptionAnalyzer.Test" /> |
There was a problem hiding this comment.
Oh nooooo - I think this is correct, though; we don't want to make the adapter modules visible, but we do want to test them.
| /// A V1 analyzer defined as a method with inferred return type (returns [] without | ||
| /// explicit type annotation). This exercises the same codepath that the legacy loader | ||
| /// handles via its generic-return-type fallback. | ||
| [<CliAnalyzer "InferredReturnAnalyzer">] |
There was a problem hiding this comment.
Tested in the test "LoadAnalyzers discovers V1 analyzer with inferred return type"
|
|
||
| let mutable projectOptions: FSharpProjectOptions = Unchecked.defaultof<_> | ||
|
|
||
| [<OneTimeSetUp>] |
There was a problem hiding this comment.
I switched to OneTimeSetUp, by the way, which I think is more appropriate.
Ref: #288
The change is backward-compatible, as is demonstrated by the fact that the old
OptionAnalyzercontinues to work (though we don't demonstrate that the ongoing forward-compatibility guarantee is satisfied). The old API is now interpreted as the "bleeding-edge" API, and analyzers written against it are liable to be broken by any version upgrade just as they are right now.We supply adapters for every stable API we offer: FSharp.Analyzers.SDK reads in the source using FCS's most recent types, and then projects them back down to each stable API as necessary.
Claude explicitly chose to make the CliContext and EditorContext be the same type. I don't have a strong opinion about that; I've never written an editor analyzer.
Open problem: what should the versioned namespaces be called?
Right now I haven't read this code at all; consider it completely unsuited for review unless you're seriously interested in LLM slop. It's Claude Opus 4.6.
Here's the plan Claude was following.
# Versioned Analyzer API for FSharp.Analyzers.SDKContext
When the SDK changes version, all analyzers must be rebuilt. The root causes:
Client.fs:261-263rejects any analyzer whose SDK reference doesn't match Major.Minor exactly.CliContext,EditorContext, walkers,Message.Range,Fix.FromRangeall expose FCS types (FSharpCheckFileResults,FSharpExpr,FSharpType,range, etc.) directly. An FCS upgrade changes the SDK's public surface, breaking all analyzers even if the SDK API itself didn't change.The goal: allow an analyzer compiled against SDK V(N) to continue working when loaded by SDK V(M) where M > N, without recompilation.
Architecture Overview
Append-only versioned namespaces (
FSharp.Analyzers.SDK.V1,.V2, ...), each containing a complete, frozen set of SDK-owned types that analyzer authors code against. No FCS types cross the boundary into a versioned namespace.FSharp.Analyzers.SDK.dllassembly. Analyzer authors reference the SDK NuGet package and open the specific version namespace.Messageresults into the current internal representation.FSharp.Analyzers.SDK.CliAnalyzerAttribute, etc.) continues to work as-is for backward compatibility, still subject to the strict version check.Data over behaviour
The current walker base classes (
SyntaxCollectorBase,TypedTreeCollectorBase) are abstract classes with virtual methods -- a behavioral abstraction. For V1, replace this with a data-oriented typed tree: the SDK converts FCS'sFSharpImplementationFileContentsinto an SDK-owned DU (TypedExpr,TypedDeclaration) that analyzer authors traverse by pattern matching. A conveniencevisitfunction can be layered on top for those who prefer the callback style, but the primary API is the data structure.This aligns with "compute a description of what to do, then do it" and makes the tree inspectable, serialisable, and property-testable.
V1 Type Design
Source range (decoupling from FCS
range)This is the single most impactful decoupling:
rangecurrently appears inMessage,Fix, and ~10 walker callback signatures.Output types
Structurally identical to the current types but using
SourceRangeinstead of FCSrange.TAST mirror types
SDK-owned records projecting the FCS class properties that analyzers read. The set of properties is chosen by surveying the walker callback parameters and the OptionAnalyzer sample.
Key types (illustrative, not exhaustive field lists):
EntityInfoFSharpEntityTypeInfoFSharpTypeMemberOrFunctionOrValueInfoFSharpMemberOrFunctionOrValueFieldInfoFSharpFieldUnionCaseInfoFSharpUnionCaseGenericParameterInfoFSharpGenericParameterObjectExprOverrideInfoFSharpObjectExprOverrideMemberFlagsSynMemberFlagsHandling recursive type graphs: The conversion function uses an internal memoisation cache keyed by FCS object reference identity. When an entity/type is encountered a second time during the same conversion walk, a stub is returned (FullName populated, structural children set to empty/None). This prevents infinite recursion while preserving the most important data (names, which is what analyzers typically read from transitive references).
Typed expression tree (DU)
Replace the walker's 40 abstract methods with a single DU. Each case mirrors one branch of
visitExprinTASTCollecting.fs:264-439:This mirrors exactly the structure that
visitExprandvisitDeclarationinTASTCollecting.fsalready decompose. TheUnknowncase is a catch-all for any FCS expression form not present in V1 (forward-compatible).Typed tree conversion function (the primary API)
The
TypedTreeHandleis an opaque wrapper aroundFSharpImplementationFileContentsthat lives in the V1 context. Analyzer authors callconvertTastand then traverse the returnedTypedDeclaration listby pattern matching.Optional convenience: a
visitfunction that walks the tree and calls a handler, for those who prefer the callback pattern:Context types
Where:
TypedTreeHandleis opaque; passed toconvertTast.ProjectOptionsInfois an SDK-owned record:{ ProjectFileName; SourceFiles; ReferencedProjectsPaths; OtherOptions }.SymbolUseInfocaptures{ Symbol: SymbolInfo; Range: SourceRange; IsFromDefinition; IsFromPattern; ... }.FSharpParseFileResults,FSharpCheckFileResults, orFSharpCheckProjectResults. Their useful data is pre-extracted into the context fields above (symbol uses, diagnostics, ignore ranges). If additional data is needed, add specific fields in V2.EditorContext: deferred to V2. V1 covers CLI analyzers only.
Attributes and analyzer type
Note:
V1.CliAnalyzerAttributeis a separate type from the currentFSharp.Analyzers.SDK.CliAnalyzerAttribute. The loader distinguishes them by checking the attribute's declaring namespace.Migration Mechanism
Input projection (current FCS -> V1)
Internal module
FSharp.Analyzers.SDK.Migration.V1:rangeToV1: FSharp.Compiler.Text.range -> V1.SourceRange-- field-by-field copyentityToV1: FSharpEntity -> V1.EntityInfo-- read properties, memoise by identitytypeToV1: FSharpType -> V1.TypeInfo-- recursive, memoisedmemberToV1: FSharpMemberOrFunctionOrValue -> V1.MemberOrFunctionOrValueInfoexprToV1: FSharpExpr -> V1.TypedExpr-- mirrorsvisitExprstructurecontextToV1: CliContext -> V1.CliContext-- constructs V1 context, wraps TypedTree in handleThe
exprToV1function is structurally identical to the existingvisitExprmatch expression (TASTCollecting.fs:266-439), but returnsTypedExprDU values instead of calling walker methods. This is the key observation: the conversion function is almost a copy of the existing code with a different output type.Output migration (V1 -> current)
rangeFromV1: V1.SourceRange -> range--Range.mkRange+Position.mkPosmessageFromV1: V1.Message -> Message-- map fields, convert severity and rangefixFromV1: V1.Fix -> Fix-- map fieldsFuture versions (V2, V3, ...)
When V2 is created:
TypedExpr(new FCS expression forms), or new context fields.V2 -> currentis direct.V1 -> currentstays as-is (V1 types are frozen, the V1 migration code is frozen).Loader Changes
File:
src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.Client.fsVersion detection
The
isAnalyzerfunction (Client.fs:31-34) checks attributes by type name. Extend it to check for V1 attributes:Adapter wrapping
When a V1 analyzer is detected, wrap it as a current-version analyzer:
The adapted analyzer is stored in the same
registeredAnalyzersdictionary as current-version analyzers. From the runner's perspective, all analyzers areAnalyzer<CliContext>.Version check relaxation
For V1+ analyzers (detected by versioned attributes), skip the Major.Minor version check entirely. The versioned types provide the compatibility guarantee. The strict check remains for unversioned (legacy) analyzers.
AST Support
Deferred to V2. Rationale:
SynExpr~70 cases,SynPat~20,SynType~20, plus auxiliaries) are much larger than the TAST expression types (~40 cases).When V2 adds AST support, it will define an SDK-owned
SyntaxExprDU (and friends) mirroring the Syn* types, with aconvertAstfunction analogous toconvertTast.Implementation Phases
Phase 1: V1 type definitions
New files in
src/FSharp.Analyzers.SDK/:V1.Types.fs-- SourceRange, Severity, Fix, Message, EntityInfo, TypeInfo, MemberOrFunctionOrValueInfo, FieldInfo, UnionCaseInfo, GenericParameterInfo, MemberFlags, ObjectExprOverrideInfo, TypedExpr, TypedDeclaration, SymbolUseInfo, SymbolInfo, ProjectOptionsInfo, CliContext, TypedTreeHandle, AnalyzerIgnoreRangeV1.Types.fsi-- public API surfaceV1.Attributes.fs-- CliAnalyzerAttribute, Analyzer type aliasPhase 2: Conversion functions
V1.Migration.fs-- internal module: rangeToV1, entityToV1, typeToV1, memberToV1, exprToV1, contextToV1, rangeFromV1, messageFromV1The
exprToV1function is modelled directly onTASTCollecting.fs:visitExpr(same match structure, different output).Phase 3: V1 TAST API
V1.TASTCollecting.fs--convertTast(unwraps TypedTreeHandle, calls conversion), optionalvisitTypedTreeconvenience functionPhase 4: Loader integration
Client.fs: add V1 attribute detection, adapter wrapping, relaxed version check for versioned analyzers.Phase 5: Testing
samples/OptionAnalyzer.V1/-- validates the API is usable.projectToV1(fcsValue).Field = fcsValue.Field(conversion preserves data).rangeFromV1(rangeToV1(r)) = r(round-trip).messageFromV1(messageToV1(m))preserves all fields.Client, run both on the same file, verify both produce correct results.Phase 6: Freeze V1
Transition for Existing Analyzer Authors
FSharp.Analyzers.SDK.V1instead ofFSharp.Analyzers.SDK.V1.CliAnalyzerAttributeandV1.Analyzer.V1.CliContext,V1.Message,V1.SourceRangeetc.V1.TASTCollecting.convertTastto get the typed tree and pattern-matches onTypedExpr.FSharp.Compiler.Service(no moreopen FSharp.Compiler.*).Example: the OptionAnalyzer rewritten for V1:
Risks and Mitigations
Critical Files
src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fs-- current types to mirrorsrc/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fsi-- current public API surfacesrc/FSharp.Analyzers.SDK/TASTCollecting.fs--visitExpr(lines 264-439) is the template for theexprToV1conversion functionsrc/FSharp.Analyzers.SDK/TASTCollecting.fsi-- current walker API to replacesrc/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.Client.fs-- loader (version check at lines 258-278, attribute detection at lines 31-34)src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.Client.fsi-- client API surfacesamples/OptionAnalyzer/Library.fs-- reference analyzer to port as validationsrc/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fsproj-- add new V1 source files