Skip to content

[Repo Assist] Fix decodeILCustomAttribData: read correct byte-width for non-Int32 enum fixed args#475

Merged
dsyme merged 2 commits intomasterfrom
repo-assist/fix-decode-enum-attr-size-20260313-c6d109c01b403825
Mar 16, 2026
Merged

[Repo Assist] Fix decodeILCustomAttribData: read correct byte-width for non-Int32 enum fixed args#475
dsyme merged 2 commits intomasterfrom
repo-assist/fix-decode-enum-attr-size-20260313-c6d109c01b403825

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Problem

In ECMA-335 §II.23.3, custom attribute fixed arguments are serialised using the width of the enum's underlying primitive type: 1 byte for byte/sbyte enums, 2 bytes for int16/uint16 enums, 8 bytes for int64/uint64 enums, etc.

decodeILCustomAttribData had a catch-all arm for ILType.Value _ (enum types) that always read 4 bytes (sigptr_get_i32), regardless of the actual underlying width:

// Before – always reads 4 bytes for any enum type, violating ECMA-335
| ILType.Value _ ->
    let n, sigptr = sigptr_get_i32 bytes sigptr
    CustomAttributeTypedArgument(typ, box n), sigptr

This caused corrupt decoding of custom attribute data whenever a reference assembly used a non-int32-backed enum in an attribute constructor or named argument. For byte-backed enums this desynchronises the buffer pointer, causing all subsequent attribute arguments to be misread.

Fix

Added a resolveEnumUnderlyingILType helper inside txCustomAttributesDatum that:

  1. Resolves the ILType.Value via txILType to get the actual CLR Type
  2. Calls Enum.GetUnderlyingType to find the backing primitive
  3. Maps back to an ILType (e.g., ilg.typ_Byte, ilg.typ_Int64, etc.)
  4. Falls back to ilg.typ_Int32 on any exception (e.g. unresolvable assembly reference)

The ILType.Value _ arm now calls parseVal underlyingTy sigptr using the resolved type:

// After – dispatches to the correct parseVal arm based on actual underlying type
| ILType.Value _ ->
    let underlyingTy = resolveEnumUnderlyingILType typ
    let v, sigptr = parseVal underlyingTy sigptr
    CustomAttributeTypedArgument(typ, fst (unbox v)), sigptr
```

## Impact & Safety

- **Zero regression risk**: for `int32` enums (the vast majority), `resolveEnumUnderlyingILType` returns `ilg.typ_Int32` — identical behaviour to before.
- The fallback ensures correctness even when the enum's assembly can't be resolved.
- Both the fixed-arg and named-arg code paths through `ILType.Value _` are fixed.

## Test Status

```
Passed!  - Failed: 0, Passed: 110, Skipped: 0, Total: 110, Duration: 6 s

All 110 existing tests pass. The fix is exercised via the GetCustomAttributesData() API on TargetTypeDefinition instances — end-to-end coverage of the decode path.

Generated by Repo Assist

Generated by Repo Assist ·

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@346204513ecfa08b81566450d7d599556807389f

… enum fixed args

Previously the catch-all ILType.Value arm in parseVal always read 4 bytes
(int32) for any enum-typed fixed argument in a custom attribute blob.
ECMA-335 requires that enum values are stored using the width of the enum's
underlying type, so a byte-backed enum attribute argument encodes in 1 byte
and an int64-backed one in 8 bytes.

Fix: add a resolveEnumUnderlyingILType resolver that is threaded through
decodeILCustomAttribData. At the txCustomAttributesDatum call site the
resolver uses txILType to get the runtime System.Type for the enum, calls
Enum.GetUnderlyingType, and maps back to the correct ILType. Any resolution
failure falls back to ilg.typ_Int32 (preserving prior behaviour).

For the common case (int32 enums) the resolver returns ilg.typ_Int32 and
behaviour is identical to before. The fix benefits type providers that
reference assemblies containing attributes with byte, int16, uint16,
uint32, int64, or uint64 enum parameters.

All 110 existing tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dsyme dsyme merged commit 2980382 into master Mar 16, 2026
2 checks passed
dsyme pushed a commit that referenced this pull request Mar 17, 2026
… attribute test (#477)

🤖 *This is an automated PR from Repo Assist, an AI assistant for this
repository.*

## Summary

Two categories of testing improvement for generative type providers,
both targeting previously untested code paths.

### 1 — Generative event tests (`GenerativeEventsTests.fs`, new file, 4
tests)

`ProvidedEvent` was used in `BasicGenerativeProvisionTests.fs` to
construct a type, but no test ever loaded the resulting generated
assembly and verified that events survived the round-trip. The new file
adds:

| Test | What it checks |
|------|----------------|
| `Generative instance event is present` | Event exists in generated
assembly, `EventHandlerType` is correct, add/remove are instance methods
|
| `Instance event add/remove methods have correct signatures` |
`add_Changed`/`remove_Changed` names, single `EventHandler` parameter |
| `Generative static event is present` | Static event exists, add/remove
are static methods |
| `Generative type has expected event count` | Total event count is 2
(one instance + one static) |

### 2 — Named-argument custom attribute test
(`BasicGenerativeProvisionTests.fs`, 1 test)

Existing custom-attribute tests only exercised positional constructor
arguments (`ConstructorArguments`). The `NamedArguments` encoding path
in `defineCustomAttrs` (ProvidedTypes.fs lines 15696–15697) was
reachable but untested. The new test:

- Adds `GenerativeProviderWithNamedArgAttrs` — a generative type
provider that attaches `DebuggerDisplayAttribute("{Value}")` **with a
named property argument** `Name = "MyProp"` to a provided property.
- Verifies both the constructor arg `"{Value}"` and the named arg `Name
= "MyProp"` survive `GetGeneratedAssemblyContents` → `Assembly.Load` →
`GetCustomAttributesData()`.

## Test Status

```
Passed! - Failed: 0, Passed: 117, Skipped: 0, Total: 117
```
(117 tests; was 110 before this PR's additions, with the delta from PRs
#470 / #475 already merged.)




> Generated by [Repo
Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23172533836)
·
[◷](https://github.com/search?q=repo%3Afsprojects%2FFSharp.TypeProviders.SDK+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests)
>
> To install this [agentic
workflow](https://github.com/githubnext/agentics/tree/346204513ecfa08b81566450d7d599556807389f/workflows/repo-assist.md),
run
> ```
> gh aw add
githubnext/agentics@3462045
> ```

<!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, id:
23172533836, workflow_id: repo-assist, run:
https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23172533836
-->

<!-- gh-aw-workflow-id: repo-assist -->

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dsyme pushed a commit that referenced this pull request Apr 1, 2026
🤖 *This PR was created by [Repo
Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23774627661),
an automated AI assistant.*

## Summary

Release notes for version **8.4.0**, covering all changes merged since
8.3.0 (February 26, 2026).

## Changes included in 8.4.0

**Bug fixes:**
- `GetEnumUnderlyingType()` now correctly handles non-Int32 enum
underlying types (#470)
- `decodeILCustomAttribData` reads correct byte-width for non-Int32 enum
fixed args (ECMA-335) (#475)
- Generative delegate type support fixed; `GetInterface` implemented on
`ProvidedTypeDefinition` and `TargetTypeDefinition` (#479)
- Thread-safety races in `TargetTypeDefinition` member-wrapper caches
fixed (#482)

**Performance:**
- Member wrapper objects cached in `TargetTypeDefinition` to avoid
repeated allocations (#471)
- `FullName`, `BaseType` and `GetInterfaces()` cached in
`TargetTypeDefinition` (#485)

**Refactoring:**
- `mkCacheInt32`/`mkCacheGeneric` simplified to use
`ConcurrentDictionary` (#486)

**CI:**
- GitHub Actions updated from v1 to v4 (#476)

## Test Status

✅ All 126 tests pass on the current master (`dotnet test
tests/FSharp.TypeProviders.SDK.Tests.fsproj -c Release`)

*Please bump the NuGet package version to 8.4.0 in the project file
before merging if that step isn't automated.*




> Generated by 🌈 Repo Assist at
[{run-started}](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23774627661).
[Learn
more](https://github.com/githubnext/agentics/blob/main/docs/repo-assist.md).
>
> To install this [agentic
workflow](https://github.com/githubnext/agentics/tree/1f672aef974f4246124860fc532f82fe8a93a57e/workflows/repo-assist.md),
run
> ```
> gh aw add
githubnext/agentics@1f672ae
> ```

<!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto,
id: 23774627661, workflow_id: repo-assist, run:
https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23774627661
-->

<!-- gh-aw-workflow-id: repo-assist -->

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dsyme pushed a commit that referenced this pull request Apr 13, 2026
… call); add 5 tests for warning + enum round-trip (#501)

🤖 *This PR was created by [Repo
Assist](https://github.com/githubnext/agentics/blob/main/docs/repo-assist.md),
an automated AI assistant.*

## Summary

This PR bundles a **bug fix** and **new tests** (Task 9: Testing
Improvements).

### Bug fix: `ProvidedTypeDefinition.Logger` was silently broken

The static `Logger` member was implemented as a computed property:

```fsharp
// Before — creates a fresh ref cell on every access; impossible to set
static member Logger: (string -> unit) option ref = ref None
```

Every read of `ProvidedTypeDefinition.Logger` returned a *different*
`ref` object, so calling `ProvidedTypeDefinition.Logger := Some f` would
write into a temporary cell that is immediately discarded. The Logger
was effectively inoperative: the "all static parameters optional"
warning (added in 8.3.0 / PR #428) could never fire via the Logger, and
type-creation trace messages were silently dropped.

**Fix**: introduces a `static let loggerRef` field and makes `Logger`
return it:

```fsharp
static let loggerRef: (string -> unit) option ref = ref None
...
static member Logger: (string -> unit) option ref = loggerRef
```

### New tests (5 tests, 131 total)

**`BasicErasedProvisionTests.fs` — 3 new tests for the all-optional
warning:**
- `DefineStaticParameters warns when all static parameters have
defaults` — regression for the warning path (which was previously
unreachable due to the Logger bug)
- `DefineStaticParameters does not warn when at least one parameter has
no default`
- `DefineStaticParameters does not warn when there are no static
parameters`

**`GenerativeEnumsProvisionTests.fs` — 2 new tests for non-Int32 enum
round-trip via target context:**
- `Byte enum underlying type is correct when read via target context
(ReadRelatedAssembly)` — exercises
`TargetTypeDefinition.GetEnumUnderlyingType()` for a `byte`-backed enum
- `Int64 enum underlying type is correct when read via target context
(ReadRelatedAssembly)` — same for `int64`-backed enums

These complement the existing runtime-path enum tests (`Assembly.Load`)
by covering the design-time IL-reader path, which was fixed in PR
#470/#475.

## Trade-offs

- The Logger ref is now module-level shared state. Tests that manipulate
it should save/restore the previous value (all three new tests do this
via `try/finally`).

## Test Status

✅ All 131 tests pass (`dotnet test
tests/FSharp.TypeProviders.SDK.Tests.fsproj -c Release`)




> Generated by 🌈 Repo Assist, see [workflow
run](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/24270385678).
[Learn
more](https://github.com/githubnext/agentics/blob/main/docs/repo-assist.md).
>
> To install this [agentic
workflow](https://github.com/githubnext/agentics/blob/7ee2b60744abf71b985bead4599640f165edcd93/workflows/repo-assist.md),
run
> ```
> gh aw add
githubnext/agentics@7ee2b60
> ```

<!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto,
id: 24270385678, workflow_id: repo-assist, run:
https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/24270385678
-->

<!-- gh-aw-workflow-id: repo-assist -->

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant