feat(PC0029): trace global variables and cover all record receiver forms - #544
Merged
Merged
Conversation
… AlreadySequentialGuid
NoDiagnostic and HasFix test methods now call RequireMinimumVersion("16.0")
matching HasDiagnostic, so all three skip consistently on SDKs below the
CreateSequentialGuid() threshold.
AlreadySequentialGuid.al verifies that Guid.CreateSequentialGuid() assigned
to a key field is not flagged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
When CreateGuid() is assigned to a codeunit or table global variable, the analyzer now walks all sibling method and trigger bodies of the containing object to determine whether the value flows to a key field. SymbolFlowTracer accepts an optional containingSymbol so that bare-self field access inside a table resolves correctly: a global variable's own ContainingSymbol is the object (whose ContainingType is null), while the method symbol provides the expected receiver context. DescendantNodes().OfType<MethodOrTriggerDeclarationSyntax>() covers page/report control triggers that are not object members. Including the current body handles same-procedure global assignments without a special case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ever mention it
Replace the private one-level UnwrapConversion (only IConversionExpression)
with Common's UnwrapConversions() which also peels IParenthesizedExpression,
so (CreateGuid()) assigned to a key field is now detected.
Add a syntax pre-filter: body.ToString().IndexOf("CreateGuid") skips bodies
that never mention the method, avoiding an unnecessary bind.
Fix the UseSequentialGuidScope schema description to say "field that is part
of a table key" instead of "primary key fields" (all declared keys count).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 12, 2026
… items The rule doc now states that object-scope variables are traced through every method and trigger body of the containing object, why the tracer receives the sibling method as containing symbol, and why the analyzer pre-filters bodies on the CreateGuid text and uses the shared conversion unwrap. Caller-direction tracing, RecordRef.SetTable and a dedicated suppression mechanism are recorded as deliberate non-reports. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… name TraceGlobalVariable bound every method and trigger body of the containing object before running the tracer. A case-insensitive text check on the variable name now precedes the bind, mirroring the CreateGuid pre-filter of the main callback: bodies that never reference the global cost a string scan instead of a full bind, and the tracer still decides by symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Arthurvdv
marked this pull request as ready for review
September 12, 2026 22:48
…d TableNo OnRun HasDiagnostic fixtures for every receiver form the plan identified: - Bare self and Validate in table triggers (OnInsert) - Implicit primary key (table without keys section): named variable, Rec self, Validate - Namespace-qualified record variable (with and without explicit keys) - TableNo codeunit OnRun: Rec self (implicit + explicit keys), bare self, Validate - Table extension: bare, Rec, this, Validate - Page Rec and bare self, page extension Rec - Request page Rec, report Rec via request page, xmlport Rec via request page - Report dataitem: named variable and bare self NoDiagnostic fixtures: - Page with SourceTableTemporary = true (temporary suppression) - Implicit primary key: assignment to non-key field (field 2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…section
IsFieldInAnyKey iterated only table.Keys (declared keys). A table
without a keys { } section has no declared keys; its synthesized
primary key (lowest-Id Guid field) is exposed solely through
table.PrimaryKey (TableTypeSymbol.cs:109-125, SynthesizedKeySymbol).
Check PrimaryKey.Fields first, then the declared Keys loop.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t primary key and namespaces The record-receiver-forms guide becomes receiver-forms.md and now states where each object kind gets its Rec (a synthesized global in tables and the page family, a synthesized local inside a TableNo codeunit's OnRun, none in dataitems), that bare self binds with a null instance only inside tables and tableextensions, that the four forms apply to fields, record methods and user procedures alike, that ITableTypeSymbol.Keys omits the synthesized primary key of a table without a keys section, and that namespaced record types bind to the same symbol. testing.md, REVIEW.md, the new-analyzer, fix-false-positive and new-codefix skills and the regression catalog point at the guide and require the matching fixtures. The PC0029 rule doc records the PrimaryKey check and the fixture coverage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Five report sites built the same "value flows to key field" sentence inline; a KeyFieldReason helper on the result struct keeps the wording in one place so a change cannot leave the sites inconsistent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
In a code-block action the body is already bound whenever any analyzer in the run registers operation actions, and the member semantic model caches bound nodes, so scanning body.ToString() could never skip the bind. It only replaced the walker traversal with a body-sized string allocation on every callback. The walker now runs unconditionally; the rule doc records why no pre-filter exists. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.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
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.
Why
Issue #515 holds the eight deferred PC0029 ideas. Global variable tracing was approved; the remaining items are closed as won't-do (table below). While testing, three false negatives surfaced: a bare-self key-field write inside a table trigger, a namespaced codeunit with a fully qualified
Recordvariable, andRec."Primary Key"in theOnRuntrigger of a codeunit withTableNo. Fixtures showed that the receiver forms were already handled and that the real gap was the implicit primary key: a table declared without akeyssection has a synthesized primary key thatITableTypeSymbol.Keysnever lists.What
CreateGuid()is assigned to an object-scope Guid variable, the analyzer walks the sibling method and trigger bodies of the same object (text-prefiltered on the variable name) to see whether the value reaches a key field.ITableTypeSymbol.PrimaryKeybefore the declaredKeys, so tables without akeyssection are covered, including tables from referenced apps.UnwrapConversionis replaced by Common'sUnwrapConversions(), which also peelsIParenthesizedExpression.NoDiagnosticandHasFixcallRequireMinimumVersion("16.0")likeHasDiagnostic; theAlreadySequentialGuidfixture is back.UseSequentialGuidScopenow says "field that is part of a table key" instead of "primary key fields".Receiver-form coverage
26 new fixtures pin every context where
Recor self exists: bare self in table triggers, all self forms in a tableextension, page and pageextensionRec(explicit and bare), request pageRec, report and xmlportRecvia their request page, report dataitem (named and bare),TableNocodeunitOnRun(Rec, bare,Validate), a namespaced file with a fully qualified record type, tables without akeyssection, and two NoDiagnostic guards (temporary page source table throughRec; a non-key field of a table withoutkeys). Before the analyzer fix, exactly the five implicit-primary-key cases failed; everything else passed, so no change toGetReceiverTableTypewas needed.Guide and follow-ups
.claude/rules/record-receiver-forms.mdis renamed toreceiver-forms.mdand now documents where each object kind gets itsRec(global vs. theOnRunlocal vs. none), that bare self binds with a null instance only inside tables and tableextensions, that the four forms apply to fields, record methods and user procedures, the implicit primary key, and namespaces.testing.md,REVIEW.md, the three skills and the regression catalog require the matching fixtures.Rule210SuboptimalIndexandRule222share theKeysblind spot).Decision table (from the #515 triage)
SetTable()tracingexit(CreateGuid()), caller assigns to key)var) tracking back to callersAlreadySequentialGuidNoDiagnostic fixtureCreateSequentialGuid(); every PC0029 fixture now runs on runtime 16.0#pragma warning disable PC0029already covers it (documented on alcops.dev)Test notes
netstandard2.1,net8.0,net10.0);dotnet formatclean;Validate-Rules.ps1passes.Docs companion: ALCops/alcops.dev#184
Closes #515
🤖 Generated with Claude Code