Fix #834: emit NullableAttribute / NullableContextAttribute for T? reference parameters - #842
Merged
Conversation
…eference parameters (#834) The reflection metadata emitter did not stamp per-parameter or method-level nullability attributes, so C# consumers with `nullable enable` saw G# extension parameters typed `T?` as non-annotated and produced spurious CS8602 "dereference of a possibly null reference" warnings. Add a DFS pre-order `NullableFlagsBuilder` that walks a TypeSymbol and produces the C#-compatible nullable byte sequence (0=oblivious, 1=not-annotated, 2=annotated; outer reference type contributes one byte, value types contribute none for the outer position but still recurse into their generic args). Cache well-known ctor refs for `NullableAttribute(byte)`, `NullableAttribute(byte[])`, and `NullableContextAttribute(byte)` and add encoder helpers that attach them to ParameterHandle / MethodDefinitionHandle. `EmitFunction` now computes nullable flags for the return position and each parameter up front, synthesises the sequence-0 Param row when the return needs an attribute, and after `AddMethodDefinition`: * picks a method-level common byte (Roslyn-style majority, ties favour the assembly default 1) and emits `NullableContextAttribute(common)` when it deviates from the assembly default; * emits the return `NullableAttribute` when its flags deviate from the effective default; * emits per-parameter `NullableAttribute` only on positions whose flags deviate from the effective default. Generic `T?` over class-constrained / unconstrained type parameters keeps its symbolic shape and emits byte 2 as expected; `T?` over struct-constrained type parameters lowers to `Nullable<T>` (a value type) and contributes no nullable bytes. This closes the consumer-side CS8602 workaround documented in ADR-0084 and called out by #806; the `#pragma warning disable CS8602` in `test/Extensions.Tests/OptionalExtensionsTests.cs` is removed and the project now compiles clean against the G#-emitted `Gsharp.Extensions.dll`. Closes #834 Related: #806, ADR-0084, parent #706
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.
Summary
Closes #834.
The reflection metadata emitter did not emit per-parameter
[System.Runtime.CompilerServices.NullableAttribute]ormethod-level
[NullableContextAttribute]annotations. As a result,G# extension methods that declared a
T?reference parametershipped as plain
Tfrom the C# compiler's perspective, andconsumer projects with
<Nullable>enable</Nullable>got spuriousCS8602: Dereference of a possibly null referencewarnings.This PR fixes the emitter so C# consumers see the nullability
annotation directly, matching the IL Roslyn would emit for the
equivalent C# source.
Root cause
ReflectionMetadataEmitter.EmitFunctiononly stamped theassembly-level
NullableContextAttribute(1)(everything isnon-nullable). It never inspected individual parameter / return
types for
T?nodes, and never emitted the per-positionNullableAttribute(2)(or the method-levelNullableContextAttribute(2)Roslyn uses when all positions sharethe same annotation).
Fix
NullableFlagsBuilder— DFS pre-order walker overTypeSymbolproducing the C#-compatible byte sequence(
0=oblivious,1=not-annotated,2=annotated). The outerreference type contributes one byte; value types contribute none
for the outer position but still recurse into their generic
arguments. Handles
NullableTypeSymbol,TypeParameterSymbol(struct-constrained → no byte; otherwisebyte
1),ByRefTypeSymbol,ArrayTypeSymbol,SliceTypeSymbol,TupleTypeSymbol,EnumSymbol,StructSymbol(IsClassbranch),InterfaceSymbol,ImportedTypeSymbol, and theNullabilityAnnotatedTypeSymbolpass-through.WellKnownReferences— cached ctor refs forNullableAttribute(byte),NullableAttribute(byte[]), andNullableContextAttribute(byte); lazy lookups gracefully no-opif the attribute types can't be resolved.
CustomAttributeEncoder— newEmitNullableAttributeOnParameter(ParameterHandle, ImmutableArray<byte>)(auto-picks the
bytevsbyte[]ctor) andEmitNullableContextAttributeOnMethod(MethodDefinitionHandle, byte)helpers.
EmitFunction— now computes return + per-param flags upfront, synthesises the sequence-0 Param row whenever the return
needs an attribute, and after
AddMethodDefinition:favour the assembly default
1) and emitsNullableContextAttribute(common)when it deviates from theassembly default;
NullableAttributewhen its flags deviatefrom the effective default;
NullableAttributeonly on positionswhose flags deviate from the effective default.
T?over aclass-constrained / unconstrained TP keeps itssymbolic shape and emits byte
2;T?over astruct-constrainedTP lowers to
Nullable<T>(value type) and contributes nonullable bytes, matching Roslyn.
Test coverage
New file
test/Core.Tests/CodeAnalysis/Emit/Issue834NullableAttributeEmitTests.cs(8 tests, reflection-based, all green):
NullableAttribute(2)onT?for class-constrainedT.NullableContextAttribute(2)on a method where every positionshares the
T?annotation.NullableAttribute(2)onstring?parameter.NullableAttribute(or just module-levelNullableContextAttribute(1)) on plain non-null reference param.NullableAttribute({0,2,1})(or the appropriate flat shape) onIEnumerable[T]?receivers — proves the DFS pre-order shape.only stamp deviating positions.
T?overstruct-constrainedTemits no nullable attribute(lowers to
Nullable<T>).Cross-language: the file-level
#pragma warning disable CS8602in
test/Extensions.Tests/OptionalExtensionsTests.cs(theworkaround the original
#806shipped with) is removed and theproject still compiles clean — proves the fix flows through to a
real C# consumer using
nullable enable.Existing IL fingerprint baseline (
refactoring-baseline.json) wasregenerated through the documented
RegenerateBaselineworkflow toabsorb the additional attribute rows (123 hashes refreshed).
ADR-0084 paragraph that documented the original CS8602 pragma
workaround now points to this PR as the closure.
Validation
dotnet build GSharp.sln -nologo -clp:NoSummary— clean.dotnet test GSharp.sln --no-restore --nologo— all green(Core 3279 + 1 skipped, Compiler 1337, Extensions 104,
Interpreter 305, LanguageServer 266).
IlVerifier.Verifycall sites inCompiler.Tests(the localdotnet-ilverifytool was restoredwith
dotnet tool restore); the suite is clean.cd website && npm ci && npm run build— Docusaurus buildsucceeds with no broken links.
Related
NullableAttributeforT?reference parameters #834Gsharp.Extensionsfrom C# to G# #806 (shipped the C#→G# Extensions port that surfacedthe CS8602 friction)