Skip to content

Commit

Permalink
Add and emit ref kind for ref readonly parameters (#68179)
Browse files Browse the repository at this point in the history
* Add and emit ref kind for ref readonly parameters

* Verify single method IL

* Verify attribute type presence

* Test primary constructors

* Skip PE verify for records

* Add prototype comments

* Derive from `System.Attribute`

* Verify function pointer metadata

* Merge ref kind attribute parameter helpers

* Tighten conditions for `ref readonly` ref kind

* Move `ref readonly` code closer to `in` code

* Check `ref readonly` before `in` attribute

* Remove IL verification

* Update PROTOTYPE comments

* Validate ref readonly PE parameter

* Ignore return parameter

* Add more tests

* Test bad ref readonly parameter

* Verify use-site errors everywhere

* Fix PE verification

* Simply code

* Improve tests

* Simplify use site errors checking

* Use singular

* Consolidate naming and location

* Tighten checks
  • Loading branch information
jjonescz authored Jun 21, 2023
1 parent 8ca7c0a commit eb63b9f
Show file tree
Hide file tree
Showing 38 changed files with 854 additions and 54 deletions.
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ private BoundLambda ReallyBind(NamedTypeSymbol delegateType, bool inExpressionTr
}

var lambdaParameters = lambdaSymbol.Parameters;
ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, lambdaParameters, diagnostics, modifyCompilation: false);
ParameterHelpers.EnsureRefKindAttributesExist(compilation, lambdaParameters, diagnostics, modifyCompilation: false);

if (returnType.HasType)
{
Expand Down
24 changes: 24 additions & 0 deletions src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ internal abstract class PEAssemblyBuilderBase : PEModuleBuilder, Cci.IAssemblyRe

private SynthesizedEmbeddedAttributeSymbol _lazyEmbeddedAttribute;
private SynthesizedEmbeddedAttributeSymbol _lazyIsReadOnlyAttribute;
private SynthesizedEmbeddedAttributeSymbol _lazyRequiresLocationAttribute;
private SynthesizedEmbeddedAttributeSymbol _lazyIsByRefLikeAttribute;
private SynthesizedEmbeddedAttributeSymbol _lazyIsUnmanagedAttribute;
private SynthesizedEmbeddedNullableAttributeSymbol _lazyNullableAttribute;
Expand Down Expand Up @@ -94,6 +95,7 @@ internal sealed override ImmutableArray<NamedTypeSymbol> GetEmbeddedTypes(Bindin

builder.AddIfNotNull(_lazyEmbeddedAttribute);
builder.AddIfNotNull(_lazyIsReadOnlyAttribute);
builder.AddIfNotNull(_lazyRequiresLocationAttribute);
builder.AddIfNotNull(_lazyIsUnmanagedAttribute);
builder.AddIfNotNull(_lazyIsByRefLikeAttribute);
builder.AddIfNotNull(_lazyNullableAttribute);
Expand Down Expand Up @@ -292,6 +294,19 @@ protected override SynthesizedAttributeData TrySynthesizeIsReadOnlyAttribute()
return base.TrySynthesizeIsReadOnlyAttribute();
}

protected override SynthesizedAttributeData TrySynthesizeRequiresLocationAttribute()
{
if ((object)_lazyRequiresLocationAttribute != null)
{
return new SynthesizedAttributeData(
_lazyRequiresLocationAttribute.Constructors[0],
ImmutableArray<TypedConstant>.Empty,
ImmutableArray<KeyValuePair<string, TypedConstant>>.Empty);
}

return base.TrySynthesizeRequiresLocationAttribute();
}

protected override SynthesizedAttributeData TrySynthesizeIsUnmanagedAttribute()
{
if ((object)_lazyIsUnmanagedAttribute != null)
Expand Down Expand Up @@ -356,6 +371,15 @@ private void CreateEmbeddedAttributesIfNeeded(BindingDiagnosticBag diagnostics)
createParameterlessEmbeddedAttributeSymbol);
}

if ((needsAttributes & EmbeddableAttributes.RequiresLocationAttribute) != 0)
{
CreateAttributeIfNeeded(
ref _lazyRequiresLocationAttribute,
diagnostics,
AttributeDescription.RequiresLocationAttribute,
createParameterlessEmbeddedAttributeSymbol);
}

if ((needsAttributes & EmbeddableAttributes.IsByRefLikeAttribute) != 0)
{
CreateAttributeIfNeeded(
Expand Down
23 changes: 23 additions & 0 deletions src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,18 @@ internal SynthesizedAttributeData SynthesizeIsReadOnlyAttribute(Symbol symbol)
return TrySynthesizeIsReadOnlyAttribute();
}

internal SynthesizedAttributeData SynthesizeRequiresLocationAttribute(ParameterSymbol symbol)
{
if ((object)Compilation.SourceModule != symbol.ContainingModule)
{
// For symbols that are not defined in the same compilation (like NoPia), don't synthesize this attribute.
Debug.Assert(false); // PROTOTYPE: Test this code path.
return null;
}

return TrySynthesizeRequiresLocationAttribute();
}

internal SynthesizedAttributeData SynthesizeIsUnmanagedAttribute(Symbol symbol)
{
if ((object)Compilation.SourceModule != symbol.ContainingModule)
Expand Down Expand Up @@ -1763,6 +1775,12 @@ protected virtual SynthesizedAttributeData TrySynthesizeIsReadOnlyAttribute()
return Compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_IsReadOnlyAttribute__ctor);
}

protected virtual SynthesizedAttributeData TrySynthesizeRequiresLocationAttribute()
{
// For modules, this attribute should be present. Only assemblies generate and embed this type.
return Compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_RequiresLocationAttribute__ctor);
}

protected virtual SynthesizedAttributeData TrySynthesizeIsUnmanagedAttribute()
{
// For modules, this attribute should be present. Only assemblies generate and embed this type.
Expand Down Expand Up @@ -1796,6 +1814,11 @@ internal void EnsureIsReadOnlyAttributeExists()
EnsureEmbeddableAttributeExists(EmbeddableAttributes.IsReadOnlyAttribute);
}

internal void EnsureRequiresLocationAttributeExists()
{
EnsureEmbeddableAttributeExists(EmbeddableAttributes.RequiresLocationAttribute);
}

internal void EnsureIsUnmanagedAttributeExists()
{
EnsureEmbeddableAttributeExists(EmbeddableAttributes.IsUnmanagedAttribute);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private void EnsureAttributesExist(TypeCompilationState compilationState)
moduleBuilder.EnsureIsReadOnlyAttributeExists();
}

ParameterHelpers.EnsureIsReadOnlyAttributeExists(moduleBuilder, Parameters);
ParameterHelpers.EnsureRefKindAttributesExist(moduleBuilder, Parameters);

if (moduleBuilder.Compilation.ShouldEmitNativeIntegerAttributes())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,7 @@ internal FieldSymbol DefineCallSiteStorageSymbol(NamedTypeSymbol containerDefini
}
}

// PROTOTYPE: RefKindVector does not support RefReadOnlyParameter.
RefKindVector byRefs;
if (hasByRefs)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,12 @@ private void AddParameterRefKind(RefKind refKind)
AddKeyword(SyntaxKind.InKeyword);
AddSpace();
break;
case RefKind.RefReadOnlyParameter:
AddKeyword(SyntaxKind.RefKeyword);
AddSpace();
AddKeyword(SyntaxKind.ReadOnlyKeyword);
AddSpace();
break;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,8 @@ static bool hasDefaultScope(bool useUpdatedEscapeRules, AnonymousTypeField field

static bool isValidTypeArgument(bool useUpdatedEscapeRules, AnonymousTypeField field, ref bool needsIndexedName)
{
needsIndexedName = needsIndexedName || field.IsParams || field.DefaultValue is not null;
// PROTOTYPE: Instead of using indexed name for delegates with `ref readonly` parameters, expand RefKindVector to use more bits?
needsIndexedName = needsIndexedName || field.IsParams || field.DefaultValue is not null || field.RefKind == RefKind.RefReadOnlyParameter;
return hasDefaultScope(useUpdatedEscapeRules, field) &&
field.Type is { } type &&
!type.IsPointerOrFunctionPointer() &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,11 @@ internal void EnsureIsReadOnlyAttributeExists(BindingDiagnosticBag? diagnostics,
EnsureEmbeddableAttributeExists(EmbeddableAttributes.IsReadOnlyAttribute, diagnostics, location, modifyCompilation);
}

internal void EnsureRequiresLocationAttributeExists(BindingDiagnosticBag? diagnostics, Location location, bool modifyCompilation)
{
EnsureEmbeddableAttributeExists(EmbeddableAttributes.RequiresLocationAttribute, diagnostics, location, modifyCompilation);
}

internal void EnsureIsByRefLikeAttributeExists(BindingDiagnosticBag? diagnostics, Location location, bool modifyCompilation)
{
EnsureEmbeddableAttributeExists(EmbeddableAttributes.IsByRefLikeAttribute, diagnostics, location, modifyCompilation);
Expand Down Expand Up @@ -629,6 +634,13 @@ internal bool CheckIfAttributeShouldBeEmbedded(EmbeddableAttributes attribute, B
WellKnownType.System_Runtime_CompilerServices_RefSafetyRulesAttribute,
WellKnownMember.System_Runtime_CompilerServices_RefSafetyRulesAttribute__ctor);

case EmbeddableAttributes.RequiresLocationAttribute:
return CheckIfAttributeShouldBeEmbedded(
diagnosticsOpt,
locationOpt,
WellKnownType.System_Runtime_CompilerServices_RequiresLocationAttribute,
WellKnownMember.System_Runtime_CompilerServices_RequiresLocationAttribute__ctor);

default:
throw ExceptionUtilities.UnexpectedValue(attribute);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ internal enum EmbeddableAttributes
NativeIntegerAttribute = 0x40,
ScopedRefAttribute = 0x80,
RefSafetyRulesAttribute = 0x100,
RequiresLocationAttribute = 0x200,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ internal int MethodHashCode()
public override bool IsImplicitlyDeclared => true;
internal override MarshalPseudoCustomAttributeData? MarshallingInformation => null;
internal override bool IsMetadataOptional => false;
internal override bool IsMetadataIn => RefKind == RefKind.In;
internal override bool IsMetadataIn => RefKind is RefKind.In or RefKind.RefReadOnlyParameter;
internal override bool IsMetadataOut => RefKind == RefKind.Out;
internal override ConstantValue? ExplicitDefaultConstantValue => null;
internal override bool IsIDispatchConstant => false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,32 +39,32 @@ private enum WellKnownAttributeFlags
private struct PackedFlags
{
// Layout:
// |.|u|ss|fffffffff|n|rr|cccccccc|vvvvvvvv|
// |u|ss|fffffffff|n|rrr|cccccccc|vvvvvvvv|
//
// v = decoded well known attribute values. 8 bits.
// c = completion states for well known attributes. 1 if given attribute has been decoded, 0 otherwise. 8 bits.
// r = RefKind. 2 bits.
// r = RefKind. 3 bits.
// n = hasNameInMetadata. 1 bit.
// f = FlowAnalysisAnnotations. 9 bits (8 value bits + 1 completion bit).
// s = Scope. 2 bits.
// u = HasUnscopedRefAttribute. 1 bit.
// Current total = 30 bits.
// Current total = 32 bits.

private const int WellKnownAttributeDataOffset = 0;
private const int WellKnownAttributeCompletionFlagOffset = 8;
private const int RefKindOffset = 16;
private const int FlowAnalysisAnnotationsOffset = 20;
private const int ScopeOffset = 28;
private const int FlowAnalysisAnnotationsOffset = 21;
private const int ScopeOffset = 29;

private const int RefKindMask = 0x3;
private const int RefKindMask = 0x7;
private const int WellKnownAttributeDataMask = 0xFF;
private const int WellKnownAttributeCompletionFlagMask = WellKnownAttributeDataMask;
private const int FlowAnalysisAnnotationsMask = 0xFF;
private const int ScopeMask = 0x3;

private const int HasNameInMetadataBit = 0x1 << 18;
private const int FlowAnalysisAnnotationsCompletionBit = 0x1 << 19;
private const int HasUnscopedRefAttributeBit = 0x1 << 30;
private const int HasNameInMetadataBit = 0x1 << 19;
private const int FlowAnalysisAnnotationsCompletionBit = 0x1 << 20;
private const int HasUnscopedRefAttributeBit = 0x1 << 31;

private const int AllWellKnownAttributesCompleteNoData = WellKnownAttributeCompletionFlagMask << WellKnownAttributeCompletionFlagOffset;

Expand Down Expand Up @@ -230,6 +230,7 @@ private PEParameterSymbol(
ParameterHandle handle,
Symbol nullableContext,
int countOfCustomModifiers,
bool isReturn,
out bool isBad)
{
Debug.Assert((object)moduleSymbol != null);
Expand Down Expand Up @@ -280,6 +281,10 @@ private PEParameterSymbol(
{
refKind = RefKind.Out;
}
else if (!isReturn && moduleSymbol.Module.HasRequiresLocationAttribute(handle))
{
refKind = RefKind.RefReadOnlyParameter;
}
else if (moduleSymbol.Module.HasIsReadOnlyAttribute(handle))
{
refKind = RefKind.In;
Expand Down Expand Up @@ -375,19 +380,20 @@ private static PEParameterSymbol Create(
var typeWithModifiers = TypeWithAnnotations.Create(type, customModifiers: CSharpCustomModifier.Convert(customModifiers));

PEParameterSymbol parameter = customModifiers.IsDefaultOrEmpty && refCustomModifiers.IsDefaultOrEmpty
? new PEParameterSymbol(moduleSymbol, containingSymbol, ordinal, isByRef, typeWithModifiers, handle, nullableContext, 0, out isBad)
: new PEParameterSymbolWithCustomModifiers(moduleSymbol, containingSymbol, ordinal, isByRef, refCustomModifiers, typeWithModifiers, handle, nullableContext, out isBad);
? new PEParameterSymbol(moduleSymbol, containingSymbol, ordinal, isByRef, typeWithModifiers, handle, nullableContext, 0, isReturn: isReturn, out isBad)
: new PEParameterSymbolWithCustomModifiers(moduleSymbol, containingSymbol, ordinal, isByRef, refCustomModifiers, typeWithModifiers, handle, nullableContext, isReturn: isReturn, out isBad);

bool hasInAttributeModifier = parameter.RefCustomModifiers.HasInAttributeModifier();

if (isReturn)
{
// A RefReadOnly return parameter should always have this modreq, and vice versa.
Debug.Assert(parameter.RefKind != RefKind.RefReadOnlyParameter);
isBad |= (parameter.RefKind == RefKind.RefReadOnly) != hasInAttributeModifier;
}
else if (parameter.RefKind == RefKind.In)
else if (parameter.RefKind is RefKind.In or RefKind.RefReadOnlyParameter)
{
// An in parameter should not have this modreq, unless the containing symbol was virtual or abstract.
// An in/ref readonly parameter should not have this modreq, unless the containing symbol was virtual or abstract.
isBad |= isContainingSymbolVirtual != hasInAttributeModifier;
}
else if (hasInAttributeModifier)
Expand All @@ -412,10 +418,11 @@ public PEParameterSymbolWithCustomModifiers(
TypeWithAnnotations type,
ParameterHandle handle,
Symbol nullableContext,
bool isReturn,
out bool isBad) :
base(moduleSymbol, containingSymbol, ordinal, isByRef, type, handle, nullableContext,
refCustomModifiers.NullToEmpty().Length + type.CustomModifiers.Length,
out isBad)
isReturn: isReturn, out isBad)
{
_refCustomModifiers = CSharpCustomModifier.Convert(refCustomModifiers);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ internal void GetDeclarationDiagnostics(BindingDiagnosticBag addTo)
GetReturnTypeAttributes();

var compilation = DeclaringCompilation;
ParameterHelpers.EnsureIsReadOnlyAttributeExists(compilation, Parameters, addTo, modifyCompilation: false);
ParameterHelpers.EnsureRefKindAttributesExist(compilation, Parameters, addTo, modifyCompilation: false);
ParameterHelpers.EnsureNativeIntegerAttributeExists(compilation, Parameters, addTo, modifyCompilation: false);
ParameterHelpers.EnsureScopedRefAttributeExists(compilation, Parameters, addTo, modifyCompilation: false);
ParameterHelpers.EnsureNullableAttributeExists(compilation, this, Parameters, addTo, modifyCompilation: false);
Expand Down
Loading

0 comments on commit eb63b9f

Please sign in to comment.