Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions .claude/rules/common-library.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,23 @@ The `Microsoft.Dynamics.Nav.CodeAnalysis` SDK treats many types, properties, and

## Settings System

Analyzers access settings via the `IFileSystem` overload (preferred):
Analyzers pass the compilation captured from `CompilationStart` and the current callback's cancellation token. Compilation actions can use their own `context.Compilation`. Do not substitute `SemanticModel.Compilation`: the SDK can supply a different object there, splitting the configuration snapshot across callbacks.
```csharp
var settings = ALCopsSettingsProvider.GetSettings(context.SemanticModel.Compilation.FileSystem);
var settings = ALCopsSettingsProvider.GetSettings(compilation, context.CancellationToken);
int threshold = settings.CognitiveComplexityThreshold;
```

### Lookup hierarchy

Settings are resolved using `.editorconfig`-style upward traversal. The first `alcops.json` found wins (no merging):
Settings are resolved using `.editorconfig`-style upward traversal. The first local `alcops.json` found wins:

1. **App folder** (where `app.json` lives) — checked via `IFileSystem.OpenRead("alcops.json")`
2. **Parent directories** — walks up the physical filesystem indefinitely until root or an inaccessible directory
3. **Assembly location** — directory where `ALCops.Common.dll` is located
4. **Defaults** — built-in default values from `ALCopsSettings`

The selected local file can declare an external base through `Extends.Source`. Exactly one anonymously accessible HTTP(S) URL or absolute local file path is supported. HTTP(S) URLs with a non-empty `Uri.UserInfo` are rejected before any request is made, and the user info is omitted from the diagnostic source. `ALCopsSettingsInheritanceResolver` merges the referenced JSON before deserialization: local scalar values and arrays replace inherited values, while nested objects merge property by property. A referenced configuration that declares `Extends` is rejected, so inheritance chains are not followed. A declared base and its local overrides form one configuration: if inheritance fails, both are discarded in favor of built-in defaults, with CM0001 explaining why.

This allows a multi-root workspace to share a single `alcops.json` at the workspace root:
```
/workspace/
Expand All @@ -66,13 +68,18 @@ This allows a multi-root workspace to share a single `alcops.json` at the worksp

### Public API

`ALCopsSettingsProvider` exposes two entry points: `GetSettings(IFileSystem?)` (what analyzers use) and `GetLoadResult(IFileSystem?)` (settings **plus** recorded `SettingsLoadFailure`s; `GetSettings` is a thin wrapper over it, and the CM0001 analyzer is its only failure consumer). Behavior: virtual FS check → parent traversal → assembly fallback. Load results (including failures) are cached in a `ConcurrentDictionary` keyed by `IFileSystem.GetDirectoryPath()`; a `MemoryFileSystem` returning `""` bypasses the cache. JSON parsing is case-insensitive and allows comments and trailing commas.
`GetSettings(Compilation, CancellationToken)` returns settings; `GetLoadResult(Compilation, CancellationToken)` adds recorded failures for CM0001. A weak compilation cache keeps one immutable load-result snapshot across callbacks. The workspace cache, keyed by `IFileSystem.GetDirectoryPath()`, retains successful loads and deterministic configuration failures indefinitely. Retryable HTTP failures stay there for 30 seconds after the failed request completes: new compilations inside that window reuse defaults and CM0001, preventing repeated waits during offline editing. Cache hits do not extend the window. After expiry, the first new compilation requesting settings retries under the workspace lock; a failed retry starts a fresh cooldown. Existing compilation snapshots never expire. Result and expiry are published as one immutable object, with a monotonic clock so wall-clock changes cannot affect retries. Timed monitor acquisition lets a waiting caller cancel independently; cancellation exceptions are never memoized or start a cooldown. The `IFileSystem` overloads omit compilation snapshots and are used by isolated provider tests; an empty directory path bypasses the workspace cache.

`ALCopsSettingsDocument` owns both TFM JSON implementations, parsing each local/base document once and reusing it for type validation, unknown-key scanning and merge. Comments, trailing commas and case-insensitive keys share one policy. Local type validation deliberately precedes network access; inherited type validation precedes the merge so an override cannot hide an invalid base value.

### Error handling

- Inaccessible directory during parent traversal: stops traversal (treats as boundary)
- Unreadable or malformed `alcops.json` (invalid syntax, unknown enum values, wrong types): returns defaults — that fallback contract is unchanged — and records an `Unreadable`/`Invalid` failure that `Analyzers/ConfigurationCouldNotBeLoaded` reports as CM0001. An unreadable app-folder file does **not** fall through to a parent-directory file.
- Unknown top-level keys (typo'd setting names): recognized settings still apply; one `UnknownSetting` failure per key. The known-key set is reflection-derived from `ALCopsSettings` properties (case-insensitive, `$schema` allowlisted), so new settings extend it automatically.
- Empty, whitespace-only, comment-only or JSON-null local input means defaults without CM0001. An inherited document must still be an object; malformed comments, other root types and invalid settings remain failures.
- HTTP request failures (transport errors, timeout, non-success status or buffer-limit rejection) retry on a new compilation after the workspace cooldown. Caller cancellation propagates through the provider and HTTP body read, without CM0001, a cached result or a new cooldown. Synchronous NAV callbacks must still wait for an uncached source or an eligible retry; the async HTTP helper uses `ConfigureAwait(false)` throughout, independent of a host synchronization context.
- Unavailable, unreadable, chained, or invalid `Extends.Source`: applies built-in defaults for the entire configuration, including all local overrides, and records an `Unreadable`/`Invalid` failure for CM0001. HTTP requests time out after five seconds and buffer at most 1 MiB (1,048,576 bytes); the limit also covers chunked responses and bodies without Content-Length. The configured source is trusted by the project; no additional host or address restrictions are imposed.
- `MemoryFileSystem` (in tests, `GetDirectoryPath()` returns `""`): only checks virtual FS, no parent traversal
- Only `IFileSystem` members present at the AL 12 interface floor may be called (`Exists`, `OpenRead`, `GetDirectoryPath`, …). `GetAbsolutePath` is not among them — the netstandard2.1 binary would throw `MissingMethodException` on old compilers.

Expand All @@ -85,7 +92,7 @@ Users configure settings by placing an `alcops.json` file in their AL project ro
}
```

Settings are cached per directory path for the analyzer session lifetime. There is no public cache-invalidation API, so an edited `alcops.json` takes effect only after the language server restarts; tests inject an isolated `IFileSystem` (typically `MemoryFileSystem` or a purpose-built `RelativeFileSystem`) to avoid contaminating the cache.
Successful settings and deterministic configuration failures remain cached for the analyzer session. A local edit or corrected malformed source therefore requires a process restart. HTTP request failures can recover on a new compilation after the cooldown without restarting; there is no timer, file watcher or background refresh. The internal cache instance owns both lifetimes and accepts an elapsed-millisecond clock, allowing retry tests to advance time without changing a global clock or sleeping. Production uses Stopwatch for all supported TFMs. Tests also inject isolated workspace paths to avoid contaminating the process cache.

## Coding Standards

Expand Down Expand Up @@ -118,8 +125,8 @@ Settings are cached per directory path for the analyzer session lifetime. There
### How to Add a New Setting
1. Add a new property with a default value to `ALCopsSettings.cs`.
2. No changes needed to `ALCopsSettingsProvider.cs` for scalar / string / list / dictionary properties — JSON deserialization picks them up automatically.
3. **For enum-typed properties**, add a converter registration to `ALCopsSettingsProvider.cs`: `JsonStringEnumConverter` in `_jsonOptions.Converters` (net8+) and `StringEnumConverter` in `_jsonSettings.Converters` (netstandard2.1). Both are case-insensitive by default. Then add a schema-parity guard test that compares `Enum.GetNames(typeof(YourEnum))` with the `enum` array in `alcops.schema.json` (see `StatementBlockSpacingSchema` in `src/ALCops.FormattingCop.Test/Rules/StatementBlocksSeparatedByBlankLine/` for a template).
4. **For nested-class properties with a default instance** (e.g. `public MySettings MyGroup { get; set; } = new();`): JSON deserializers ignore NRT annotations and happily set the property to `null` when the JSON contains `"MyGroup": null`, which then NREs on the first consumer access — violating the "malformed alcops.json → defaults" contract. Keep the public property non-nullable and normalize in `ALCopsSettingsProvider.DeserializeSettings` after the deserialize call: `settings.MyGroup ??= new MySettings();`. Consumers then use the property directly without `!` or a duplicate fallback.
3. **For enum-typed properties**, `ALCopsSettingsDocument` registers `JsonStringEnumConverter` (net8+) and `StringEnumConverter` (netstandard2.1); both are case-insensitive. Add a schema-parity guard test that compares `Enum.GetNames(typeof(YourEnum))` with the `enum` array in `alcops.schema.json` (see `StatementBlockSpacingSchema` in `src/ALCops.FormattingCop.Test/Rules/StatementBlocksSeparatedByBlankLine/` for a template).
4. **For nested-class properties with a default instance** (e.g. `public MySettings MyGroup { get; set; } = new();`): JSON deserializers ignore NRT annotations and happily set the property to `null` when the JSON contains `"MyGroup": null`, which then NREs on the first consumer access — violating the "malformed alcops.json → defaults" contract. Keep the public property non-nullable and normalize in `ALCopsSettingsDocument.DeserializeSettings` after the deserialize call: `settings.MyGroup ??= new MySettings();`. Consumers then use the property directly without `!` or a duplicate fallback.
- Add a regression fixture that injects `{"MyGroup": null}` and asserts the analyzer falls back to defaults without NRE (see `StatementBlockSpacingNull` test case in `StatementBlocksSeparatedByBlankLine.cs` for a template). An explicit `null` is normalized, not reported as CM0001.
5. Document the new setting in the project README and update `alcops.schema.json` (`.claude/rules/settings-schema.md`).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ paths:

Checks that ToolTip text ends with an allowed punctuation character. The allowed set is configurable through `ToolTipAllowedPunctuations` in `alcops.json`.

Registers `RegisterSyntaxNodeAction` on `PageField`, `PageAction`, `Field` and `PageAnalysisView`; main type `ToolTipPunctuation` (shared with the other ToolTip rules).
Registers syntax-node actions from CompilationStart on `PageField`, `PageAction`, `Field` and `PageAnalysisView`; main type `ToolTipPunctuation` (shared with the other ToolTip rules).

## Design decisions

| Decision | Rationale |
|---|---|
| Implemented inside the shared `ToolTipPunctuation` analyzer rather than its own class | One extraction of the ToolTip text serves all ToolTip punctuation and phrasing checks. |
| Allowed punctuation comes from `ToolTipAllowedPunctuations` via `ALCopsSettingsProvider.GetSettings(compilation.FileSystem)` | Makes the set configurable per workspace/app on the existing settings infrastructure. |
| Allowed punctuation uses the settings snapshot for the compilation captured at CompilationStart, with the current callback's cancellation token | Keeps every rule and CM0001 on one snapshot despite the SDK's different SemanticModel.Compilation object; interrupted HTTP loads stop promptly. |
| Missing, empty or fully invalid settings fall back to the dot (`.` / `dot`) | Preserves the pre-configuration AC0014 behaviour instead of disabling the check. |
| The message lists the configured punctuation names, not the characters | Gives guidance that matches the user's own configuration. |

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Warns when an `alcops.json` configuration file is found but cannot be fully appl

Registers `RegisterCompilationAction` (no node or symbol kinds; reports at `Location.None`); main type `ConfigurationCouldNotBeLoaded`.

**References:** [#328](https://github.com/ALCops/Analyzers/issues/328); [discussion #483](https://github.com/ALCops/Analyzers/discussions/483) (remote `Extends` configuration) defers its failure diagnostics here.
**References:** [#328](https://github.com/ALCops/Analyzers/issues/328); [discussion #483](https://github.com/ALCops/Analyzers/discussions/483) (remote `Extends` configuration); [inheritance review](https://github.com/ALCops/Analyzers/pull/500#pullrequestreview-5112244814) (atomic fallback and HTTP response-size limit).

## Design decisions

Expand All @@ -28,7 +28,14 @@ Registers `RegisterCompilationAction` (no node or symbol kinds; reports at `Loca
| One diagnostic per unknown key | Each typo is independently fixable; `Unreadable`/`Invalid` are inherently single. |
| An unreadable app-folder file does **not** fall through to parent-directory traversal | The app-level file was intended to win; silently applying a parent file would mask the problem. Behavior change relative to pre-CM0001. |
| Virtual-file source path built from `GetDirectoryPath()` + file name, not `IFileSystem.GetAbsolutePath` | `GetAbsolutePath` does not exist on `IFileSystem` at the oldest SDK the netstandard2.1 binary runs on (AL 12); calling it would throw `MissingMethodException` there. |
| `MessageFormat` carries a free-text reason (`The ALCops configuration '{0}' could not be fully loaded: {1}`) | Future failure kinds (remote `Extends`: unreachable URL, timeout, illegal chain) reuse the same descriptor via new `SettingsLoadFailureKind` members. |
| `MessageFormat` carries a free-text reason (`The ALCops configuration '{0}' could not be fully loaded: {1}`) | Remote `Extends` failures such as unreachable URLs, timeouts, credential-bearing URLs, and illegal chains reuse the same descriptor through the existing `Unreadable` and `Invalid` failure kinds. |
| A failed declared `Extends` discards both the base and local overrides, returning complete built-in defaults | Applying only the overrides would leave a partially configured project; the warning makes the complete fallback visible. Unknown setting names remain non-fatal, preserving recognized values as for local configuration. |
| HTTP responses are bounded through `HttpClient.MaxResponseContentBufferSize`, while keeping the five-second timeout | The byte limit is enforced during buffering even without Content-Length, so a large response cannot bypass the limit by using chunked transfer. Checking string length after downloading would already have allocated the oversized body. |
| A rejected URL omits `Uri.UserInfo` from its recorded source | The CM0001 message renders that source directly in IDE diagnostics and build logs; rejecting network access must not expose the credentials in the error message. |
| HTTP request failures remain stable within a compilation and are cached per workspace for 30 seconds after each failed request | A temporary outage must not permanently poison the cache or cause another synchronous fetch on every editor pass. After the cooldown a new compilation can retry; cache hits do not postpone recovery. A monotonic clock and atomic result/expiry publication keep concurrent requests consistent. All consumers use the compilation from CompilationStart so the SDK's different SemanticModel.Compilation object cannot split that snapshot. See the [retry-cadence review](https://github.com/ALCops/Analyzers/pull/500#issuecomment-5566473333). |
| Caller cancellation propagates without a diagnostic or cached result; cache waiters can cancel independently | Editing or reloading a workspace must stop both the pending HTTP body read and unnecessary waits behind another caller. Synchronous SDK callbacks still wait for the first uncached load; all async continuations avoid context capture. |
| JSON parsing, key lookup, merge and type validation share ALCopsSettingsDocument | Local and external configuration use the same comment/trailing-comma/casing policy on both serializer stacks. Retaining local type validation before network access prevents unnecessary requests for invalid local values. |
| Null, empty or comment-only local input uses defaults silently; an inherited document still requires an object | An empty local settings file declares no policy. A declared base is required input and must pass its own validation. |

## Deliberate non-reports

Expand All @@ -39,7 +46,7 @@ Registers `RegisterCompilationAction` (no node or symbol kinds; reports at `Loca
## Known issues

- TOCTOU between `Exists` and `OpenRead`: a file deleted in between is reported as `Unreadable` with a file-not-found message. Rare, accepted.
- The settings cache has no invalidation, so a stale entry (file fixed after first load) keeps reporting until the analyzer process restarts; this is the same staleness the settings themselves already have.
- Successfully loaded settings and deterministic configuration errors have no invalidation. Correcting those still requires restarting the analyzer process; failed HTTP requests instead retry on a new compilation after the workspace cooldown. The first uncached request and each eligible retry can still wait up to five seconds.

## SDK facts

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ paths:

Reports missing blank lines around statement blocks: before/after control-flow constructs (`if`, `case`, `repeat`, `while`, `for`, `foreach`) and before scope-leaving statements (`exit`, built-in `Error(...)` and `FieldError(...)`). It is highly opinionated and therefore disabled by default; enable it explicitly and configure it via `alcops.json`.

Registers `RegisterSyntaxNodeAction` on the control-flow statement kinds and `ExitStatement`, plus `RegisterOperationAction` on `InvocationExpression` for the built-in terminators classified by `ALCops.Common.FlowTerminatingBuiltIns`; main type `StatementBlocksSeparatedByBlankLine`.
Registers syntax-node actions from CompilationStart for control-flow statements, ExitStatement and InvocationExpression. Standalone calls are bound to operations and classified by `ALCops.Common.FlowTerminatingBuiltIns`; main type `StatementBlocksSeparatedByBlankLine`.

## Design decisions

Expand All @@ -23,6 +23,7 @@ Registers `RegisterSyntaxNodeAction` on the control-flow statement kinds and `Ex
| Each statement gap has exactly one configuration-aware diagnostic owner: an adjacent block owns its "before" gap only when that check actually runs, otherwise the previous block's "after" check or the scope-leaver's "before" check does | Avoids duplicate diagnostics (e.g. a block followed by `exit`) while still reporting next to one-liners or when `ControlFlowBefore` is disabled. |
| Scope-leaving calls are the shared `FlowTerminatingBuiltIns` set (`Dialog.Error`, `Table.FieldError`, `FieldRef.FieldError`), including incomplete calls whose receiver binds to those types | One semantic definition shared with PC0038 and LC0089; the exact class-and-method match keeps user-defined `Error`/`FieldError` procedures out, and accepting the invalid binding avoids flicker while a call is being typed. |
| The diagnostic names the terminating call that was found (`Error()`, `FieldError()`) | The message must match the statement the developer is looking at, not always say `Error()`. |
| Callbacks capture the compilation from CompilationStart; standalone invocations use GetOperation after cheap syntax checks | Settings and CM0001 must share one snapshot. The NAV SDK exposes no RegisterOperationAction on CompilationStart and supplies a different compilation through semantic-model contexts. Binding the invocation retains the same semantic classifier without relying on callback order. |

## Deliberate non-reports

Expand Down
Loading
Loading