Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ public override Binder VisitConversionOperatorDeclaration(ConversionOperatorDecl

public override Binder VisitFieldDeclaration(FieldDeclarationSyntax parent)
{
return VisitCore(parent.Parent).SetOrClearUnsafeRegionIfNecessary(parent.Modifiers, isFieldDeclaration: true);
return VisitCore(parent.Parent).SetOrClearUnsafeRegionIfNecessary(parent.Modifiers);
}

public override Binder VisitEventDeclaration(EventDeclarationSyntax parent)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ void checkConstraintLanguageVersionAndRuntimeSupportForConversion(SyntaxNode syn
Debug.Assert(elementField is { });

diagnostics.ReportUseSite(elementField, syntax);
AssertNotUnsafeMemberAccess(elementField); // https://github.com/dotnet/roslyn/issues/82546: Support unsafe fields?
ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, elementField, syntax);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, elementField, syntax);

Do we actually access the field explicitly here? I don't think we do. Therefore, it feels strange that we check "safety" rules for the field.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that motivation for reporting use-site was mostly about reporting bad field types/declarations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, my question is whether it was an explicit design decision to check underlying field for "safety".


if (destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9819,7 +9819,7 @@ BoundExpression bindInlineArrayElementAccess(ExpressionSyntax node, BoundExpress

CheckFeatureAvailability(node, MessageID.IDS_FeatureInlineArrays, diagnostics);
diagnostics.ReportUseSite(elementField, node);
AssertNotUnsafeMemberAccess(elementField); // https://github.com/dotnet/roslyn/issues/82546: Support unsafe fields?
ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, elementField, node);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, elementField, node);

Similar question here


Comment thread
jjonescz marked this conversation as resolved.
TypeSymbol resultType;

Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Flags.cs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ internal Binder WithAdditionalFlagsAndContainingMemberOrLambda(BinderFlags flags
return new BinderWithContainingMemberOrLambda(this, this.Flags | flags, containing);
}

internal Binder SetOrClearUnsafeRegionIfNecessary(SyntaxTokenList modifiers, bool isIteratorBody = false, bool isFieldDeclaration = false)
internal Binder SetOrClearUnsafeRegionIfNecessary(SyntaxTokenList modifiers, bool isIteratorBody = false)
{
// In C# 13 and above, iterator bodies define a safe context even when nested in an unsafe context.
// In C# 12 and below, we keep the (spec violating) behavior that iterator bodies inherit the safe/unsafe context
Expand All @@ -107,9 +107,9 @@ internal Binder SetOrClearUnsafeRegionIfNecessary(SyntaxTokenList modifiers, boo
}

// Under the updated memory safety rules, the `unsafe` modifier on a type or member declaration
// does not introduce an unsafe context (no interior meaning), except for field declarations.
// does not introduce an unsafe context (no interior meaning).
// `unsafe { }` blocks also introduce an unsafe context.
return !withoutUnsafe && modifiers.Any(SyntaxKind.UnsafeKeyword) && (isFieldDeclaration || !this.Compilation.SourceModule.UseUpdatedMemorySafetyRules)
return !withoutUnsafe && modifiers.Any(SyntaxKind.UnsafeKeyword) && !this.Compilation.SourceModule.UseUpdatedMemorySafetyRules
? new Binder(this, this.Flags | BinderFlags.UnsafeRegion)
Comment thread
jjonescz marked this conversation as resolved.
: this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ private static TypeSymbol GetCapturedVariableFieldType(SynthesizedContainer fram

public override RefKind RefKind => RefKind.None;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ internal override bool SuppressDynamicAttribute

public override RefKind RefKind => RefKind.None;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ public override string Name
public override FlowAnalysisAnnotations FlowAnalysisAnnotations
=> FlowAnalysisAnnotations.None;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

internal override bool HasSpecialName
{
get { return false; }
Expand Down
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,6 @@ internal virtual FieldSymbol AsMember(NamedTypeSymbol newOwner)
/// </summary>
internal abstract bool IsRequired { get; }

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None; // https://github.com/dotnet/roslyn/issues/82546: Support unsafe fields?

#region Use-Site Diagnostics

internal override UseSiteInfo<AssemblySymbol> GetUseSiteInfo()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,13 @@ internal sealed class PEFieldSymbol : FieldSymbol
private struct PackedFlags
{
// Layout:
// |........................|qq|rr|v|fffff|
// |......................|uu|qq|rr|v|fffff|
//
// f = FlowAnalysisAnnotations. 5 bits (4 value bits + 1 completion bit).
// v = IsVolatile 1 bit
// r = RefKind 2 bits
// q = Required members. 2 bits (1 value bit + 1 completion bit).
// u = Requires unsafe. 2 bits (1 value bit + 1 completion bit).

private const int HasDisallowNullAttribute = 0x1 << 0;
private const int HasAllowNullAttribute = 0x1 << 1;
Expand All @@ -48,6 +49,8 @@ private struct PackedFlags

private const int HasRequiredMemberAttribute = 0x1 << 8;
private const int RequiredMemberCompletionBit = 0x1 << 9;
private const int RequiresUnsafeBit = 0x1 << 10;
private const int RequiresUnsafeCompletionBit = 0x1 << 11;

private int _bits;

Expand Down Expand Up @@ -112,6 +115,24 @@ public bool TryGetHasRequiredMemberAttribute(out bool hasRequiredMemberAttribute
hasRequiredMemberAttribute = false;
return false;
}

public bool SetRequiresUnsafe(bool requiresUnsafe)
{
int bitsToSet = RequiresUnsafeCompletionBit | (requiresUnsafe ? RequiresUnsafeBit : 0);
return ThreadSafeFlagOperations.Set(ref _bits, bitsToSet);
}

public bool TryGetRequiresUnsafe(out bool requiresUnsafe)
{
if ((_bits & RequiresUnsafeCompletionBit) != 0)
{
requiresUnsafe = (_bits & RequiresUnsafeBit) != 0;
return true;
}

requiresUnsafe = false;
return false;
}
}

private readonly FieldDefinitionHandle _handle;
Expand Down Expand Up @@ -582,15 +603,17 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
{
if (RoslynImmutableInterlocked.VolatileRead(in _lazyCustomAttributes).IsDefault)
{
var attributes = loadAndFilterAttributes(out var hasRequiredMemberAttribute);
var attributes = loadAndFilterAttributes(out var hasRequiredMemberAttribute, out var hasRequiresUnsafeAttribute);
_packedFlags.SetHasRequiredMemberAttribute(hasRequiredMemberAttribute);
_packedFlags.SetRequiresUnsafe(ComputeRequiresUnsafe(hasRequiresUnsafeAttribute));
Comment thread
jjonescz marked this conversation as resolved.
ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, attributes);
}
return _lazyCustomAttributes;

ImmutableArray<CSharpAttributeData> loadAndFilterAttributes(out bool hasRequiredMemberAttribute)
ImmutableArray<CSharpAttributeData> loadAndFilterAttributes(out bool hasRequiredMemberAttribute, out bool hasRequiresUnsafeAttribute)
{
hasRequiredMemberAttribute = false;
hasRequiresUnsafeAttribute = false;

var containingModule = ContainingPEModule;
if (!containingModule.TryGetNonEmptyCustomAttributes(_handle, out var customAttributeHandles))
Expand All @@ -611,6 +634,12 @@ ImmutableArray<CSharpAttributeData> loadAndFilterAttributes(out bool hasRequired
continue;
}

if (containingModule.AttributeMatchesFilter(handle, AttributeDescription.RequiresUnsafeAttribute))
{
hasRequiresUnsafeAttribute = true;
continue;
}

builder.Add(new PEAttributeData(containingModule, handle));
}

Expand Down Expand Up @@ -713,5 +742,43 @@ internal override bool IsRequired
return hasRequiredMemberAttribute;
}
}

private bool RequiresUnsafe
{
get
{
if (!_packedFlags.TryGetRequiresUnsafe(out bool requiresUnsafe))
{
bool hasRequiresUnsafeAttribute = ContainingPEModule.Module.HasAttribute(_handle, AttributeDescription.RequiresUnsafeAttribute);
requiresUnsafe = ComputeRequiresUnsafe(hasRequiresUnsafeAttribute);
_packedFlags.SetRequiresUnsafe(requiresUnsafe);
}

return requiresUnsafe;
}
}

private bool ComputeRequiresUnsafe(bool hasRequiresUnsafeAttribute)
{
return ContainingModule.UseUpdatedMemorySafetyRules
? hasRequiresUnsafeAttribute
// This might be expensive, so we cache it in _packedFlags.
: !IsFixedSizeBuffer && Type.ContainsPointerOrFunctionPointer();
}

internal sealed override CallerUnsafeMode CallerUnsafeMode
{
get
{
if (!RequiresUnsafe)
{
return CallerUnsafeMode.None;
}

return ContainingModule.UseUpdatedMemorySafetyRules
? CallerUnsafeMode.Explicit
: CallerUnsafeMode.Implicit;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ public override Symbol AssociatedSymbol
}
}

internal sealed override CallerUnsafeMode CallerUnsafeMode => _underlyingField.CallerUnsafeMode;

public override int TupleElementIndex => _underlyingField.TupleElementIndex;

internal override UseSiteInfo<AssemblySymbol> GetUseSiteInfo()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ protected override void DecodeWellKnownAttributeImpl(ref DecodeWellKnownAttribut
{
MarshalAsAttributeDecoder<FieldWellKnownAttributeData, AttributeSyntax, CSharpAttributeData, AttributeLocation>.Decode(ref arguments, AttributeTargets.Field, MessageProvider.Instance);
}
else if (attribute.IsTargetAttribute(AttributeDescription.RequiresUnsafeAttribute))
Comment thread
333fred marked this conversation as resolved.
{
diagnostics.Add(ErrorCode.ERR_RequiresUnsafeAttributeInSource, arguments.AttributeSyntaxOpt.Location);
}
else if (ReportExplicitUseOfReservedAttributes(in arguments,
ReservedAttributes.DynamicAttribute
| ReservedAttributes.IsReadOnlyAttribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ protected SourceEnumConstantSymbol(SourceMemberContainerTypeSymbol containingEnu

public sealed override RefKind RefKind => RefKind.None;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
{
return TypeWithAnnotations.Create(this.ContainingType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,21 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
ref attributes,
this.DeclaringCompilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_RequiredMemberAttribute__ctor));
}

if (CallerUnsafeMode == CallerUnsafeMode.Explicit)
{
AddSynthesizedAttribute(ref attributes, moduleBuilder.TrySynthesizeRequiresUnsafeAttribute());
}
}

internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions, BindingDiagnosticBag diagnostics)
{
if (CallerUnsafeMode == CallerUnsafeMode.Explicit)
{
DeclaringCompilation.EnsureRequiresUnsafeAttributeExists(diagnostics, ErrorLocation, modifyCompilation: true);
}

base.AfterAddingTypeMembersChecks(conversions, diagnostics);
}

internal override void PostDecodeWellKnownAttributes(ImmutableArray<CSharpAttributeData> boundAttributes, ImmutableArray<AttributeSyntax> allAttributeSyntaxNodes, BindingDiagnosticBag diagnostics, AttributeLocation symbolPart, WellKnownAttributeData decodedData)
Expand Down Expand Up @@ -173,6 +188,24 @@ public override int FixedSize
}
}

internal sealed override CallerUnsafeMode CallerUnsafeMode
{
get
{
if (ContainingModule.UseUpdatedMemorySafetyRules)
{
return (Modifiers & DeclarationModifiers.Unsafe) != 0 &&
AssociatedSymbol is null &&
!IsConst
? CallerUnsafeMode.Explicit
: CallerUnsafeMode.None;
}

return !IsFixedSizeBuffer && Type.ContainsPointerOrFunctionPointer()
? CallerUnsafeMode.Implicit : CallerUnsafeMode.None;
}
}

internal static DeclarationModifiers MakeModifiers(NamedTypeSymbol containingType, SyntaxToken firstIdentifier, SyntaxTokenList modifiers, bool isRefField, BindingDiagnosticBag diagnostics, out bool modifierErrors)
{
bool isInterface = containingType.IsInterface;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ public override Symbol AssociatedSymbol
}
}

internal sealed override CallerUnsafeMode CallerUnsafeMode => _underlyingField.CallerUnsafeMode;

internal override NamedTypeSymbol FixedImplementationType(PEModuleBuilder emitModule)
{
// This occurs rarely, if ever. The scenario would be a generic struct
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public override ImmutableArray<Location> Locations

public override RefKind RefKind => RefKind.None;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ public override Location TryGetFirstLocation()

public override RefKind RefKind => _property.RefKind;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => _property.RefCustomModifiers;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ internal override bool SuppressDynamicAttribute
get { return true; }
}

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

public override RefKind RefKind => RefKind.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ internal override bool SuppressDynamicAttribute
get { return true; }
}

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
{
return _type;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public SynthesizedLambdaCacheFieldSymbol(NamedTypeSymbol containingType, TypeSym

internal override bool SuppressDynamicAttribute => true;

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

IMethodSymbolInternal ISynthesizedMethodBodyImplementationSymbol.Method => _topLevelMethod;

// When the containing top-level method body is updated we don't need to attempt to update the cache field
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ internal override bool SuppressDynamicAttribute
}
}

internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None;

public override RefKind RefKind => RefKind.None;

public override ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ public sealed override Symbol ContainingSymbol

public sealed override RefKind RefKind => _underlyingField.RefKind;

internal sealed override CallerUnsafeMode CallerUnsafeMode => _underlyingField.CallerUnsafeMode;

public sealed override ImmutableArray<CustomModifier> RefCustomModifiers => _underlyingField.RefCustomModifiers;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound)
Expand Down
Loading
Loading