Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag structure we have for all source methods #68158

Merged
merged 37 commits into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
3e67b7c
Pack bits in SourceOrdinaryMethodSymbol into an existing bitflag stru…
CyrusNajmabadi May 10, 2023
ce343a6
Compute IsVarArg up front
CyrusNajmabadi May 10, 2023
1319147
Remove unused usings
CyrusNajmabadi May 10, 2023
eee77ee
Remove
CyrusNajmabadi May 10, 2023
d2a9c02
Remove passing of flag
CyrusNajmabadi May 11, 2023
824b5b1
move up and make non-virtual
CyrusNajmabadi May 11, 2023
1932ed3
"move up and make sealed"
CyrusNajmabadi May 11, 2023
9f7d914
move up and make sealed
CyrusNajmabadi May 11, 2023
a0295b7
use properties instead of flags
CyrusNajmabadi May 11, 2023
27e28b7
Merge remote-tracking branch 'upstream/main' into revertRevert
CyrusNajmabadi May 11, 2023
54c18cf
Make parameters non-optional
CyrusNajmabadi May 11, 2023
d3a9476
Pass all args
CyrusNajmabadi May 11, 2023
c74ca1a
move more down
CyrusNajmabadi May 11, 2023
e73807b
move more down
CyrusNajmabadi May 11, 2023
2ad5f4c
Pass all args
CyrusNajmabadi May 11, 2023
5eb5236
move more down
CyrusNajmabadi May 11, 2023
08eee6e
move more down
CyrusNajmabadi May 11, 2023
5640b00
move more down
CyrusNajmabadi May 11, 2023
0e4f6fb
move more down
CyrusNajmabadi May 11, 2023
d02d68e
Name consistently
CyrusNajmabadi May 11, 2023
5d757af
Set flag consistently
CyrusNajmabadi May 11, 2023
94a74c9
Rename
CyrusNajmabadi May 11, 2023
fb5eaa9
Merge remote-tracking branch 'upstream/main' into revertRevert
CyrusNajmabadi May 12, 2023
48a1a41
Fix usage of hasBody where it means hasBlockBody
CyrusNajmabadi May 12, 2023
d4bfe7e
Change to true
CyrusNajmabadi May 12, 2023
43aab35
Change to true
CyrusNajmabadi May 12, 2023
efe37fc
Fix vararg check
CyrusNajmabadi May 12, 2023
2e34e56
Change to true
CyrusNajmabadi May 12, 2023
ff7192b
NRT
CyrusNajmabadi May 12, 2023
a27e807
Pass values explicitly
CyrusNajmabadi May 12, 2023
c11f580
Add assert
CyrusNajmabadi May 12, 2023
aa120d2
Flip depending on if something is abstract or not.
CyrusNajmabadi May 12, 2023
31a6277
Merge remote-tracking branch 'upstream/main' into revertRevert
CyrusNajmabadi May 13, 2023
fc9d571
Update check
CyrusNajmabadi May 13, 2023
77474b1
Fix ocmment
CyrusNajmabadi May 15, 2023
d3b2b40
Fix ocmment
CyrusNajmabadi May 15, 2023
b76e78e
Update src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructor…
CyrusNajmabadi May 16, 2023
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 @@ -218,7 +218,6 @@ protected override ImmutableArray<TypeSymbol> ExtraSynthesizedRefParameters

internal override bool InheritsBaseMethodAttributes => true;
internal override bool GenerateDebugInfo => !this.IsAsync;
internal override bool IsExpressionBodied => false;

internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ protected SynthesizedMethodBaseSymbol(NamedTypeSymbol containingType,
returnsVoid: baseMethod.ReturnsVoid,
isExtensionMethod: false,
isNullableAnalysisEnabled: false,
isMetadataVirtualIgnoringModifiers: false);
isMetadataVirtualIgnoringModifiers: false,
isExpressionBodied: false);
}

protected void AssignTypeMapAndTypeParameters(TypeMap typeMap, ImmutableArray<TypeParameterSymbol> typeParameters)
Expand Down Expand Up @@ -226,10 +227,5 @@ public sealed override bool IsImplicitlyDeclared
{
get { return true; }
}

internal override bool IsExpressionBodied
{
get { return false; }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SourceConstructorSymbol : SourceConstructorSymbolBase
{
private readonly bool _isExpressionBodied;
private readonly bool _hasThisInitializer;
Copy link
Member Author

Choose a reason for hiding this comment

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

we could consider moving this bit down as well. That would save another unnecessary 32bits in constructors. however, constructors seem to be orders of magnitude less common than methods, so it's not high on my priority list.


public static SourceConstructorSymbol CreateConstructorSymbol(
Expand All @@ -35,14 +34,14 @@ private SourceConstructorSymbol(
base(containingType, location, syntax, SyntaxFacts.HasYieldOperations(syntax))
{
bool hasBlockBody = syntax.Body != null;
_isExpressionBodied = !hasBlockBody && syntax.ExpressionBody != null;
bool hasBody = hasBlockBody || _isExpressionBodied;
bool isExpressionBodied = !hasBlockBody && syntax.ExpressionBody != null;
bool hasBody = hasBlockBody || isExpressionBodied;
Copy link
Member Author

Choose a reason for hiding this comment

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

NIt: we have all sorts of logic for computing these values in different places. We also name the variable inconsistently all over the place (hasBody sometimes means 'has any body' and sometimes means 'has block body'). It woudl be good to make this all consistent.


_hasThisInitializer = syntax.Initializer?.Kind() == SyntaxKind.ThisConstructorInitializer;

bool modifierErrors;
var declarationModifiers = this.MakeModifiers(syntax.Modifiers, methodKind, hasBody, location, diagnostics, out modifierErrors);
this.MakeFlags(methodKind, declarationModifiers, returnsVoid: true, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled);
this.MakeFlags(methodKind, declarationModifiers, returnsVoid: true, isExpressionBodied: isExpressionBodied, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled);

if (syntax.Identifier.ValueText != containingType.Name)
{
Expand Down Expand Up @@ -163,14 +162,6 @@ internal override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclara
return OneOrMany.Create(((ConstructorDeclarationSyntax)this.SyntaxNode).AttributeLists);
}

internal override bool IsExpressionBodied
{
get
{
return _isExpressionBodied;
}
}

internal override bool IsNullableAnalysisEnabled()
{
return _hasThisInitializer ?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#nullable disable

using System;
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Expand Down Expand Up @@ -32,7 +33,8 @@ internal SourceCustomEventAccessorSymbol(
syntax.Keyword.GetLocation(), explicitlyImplementedEventOpt, aliasQualifierOpt,
isAdder: syntax.Kind() == SyntaxKind.AddAccessorDeclaration,
isIterator: SyntaxFacts.HasYieldOperations(syntax.Body),
isNullableAnalysisEnabled: isNullableAnalysisEnabled)
isNullableAnalysisEnabled: isNullableAnalysisEnabled,
isExpressionBodied: syntax is { Body: null, ExpressionBody: not null })
{
Debug.Assert(syntax != null);
Debug.Assert(syntax.Kind() == SyntaxKind.AddAccessorDeclaration || syntax.Kind() == SyntaxKind.RemoveAccessorDeclaration);
Expand Down Expand Up @@ -91,16 +93,5 @@ internal override bool GenerateDebugInfo
{
get { return true; }
}

internal override bool IsExpressionBodied
{
get
{
var syntax = GetSyntax();
var hasBody = syntax.Body != null;
var hasExpressionBody = syntax.ExpressionBody != null;
return !hasBody && hasExpressionBody;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ protected SourceDelegateMethodSymbol(
: base(delegateType, syntax.GetReference(), location: syntax.Identifier.GetLocation(), isIterator: false)
{
_returnType = returnType;
this.MakeFlags(methodKind, declarationModifiers, _returnType.IsVoidType(), isExtensionMethod: false, isNullableAnalysisEnabled: false);
this.MakeFlags(
methodKind, declarationModifiers, _returnType.IsVoidType(), isExpressionBodied: false, isExtensionMethod: false, isNullableAnalysisEnabled: false);
}

internal sealed override ExecutableCodeBinder TryGetBodyBinder(BinderFactory binderFactoryOpt = null, bool ignoreAccessibility = false) => throw ExceptionUtilities.Unreachable();
Expand Down Expand Up @@ -176,11 +177,6 @@ public sealed override bool IsImplicitlyDeclared
}
}

internal override bool IsExpressionBodied
{
get { return false; }
}

internal override bool GenerateDebugInfo
{
get { return false; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
internal sealed class SourceDestructorSymbol : SourceMemberMethodSymbol
{
private TypeWithAnnotations _lazyReturnType;
private readonly bool _isExpressionBodied;

internal SourceDestructorSymbol(
SourceMemberContainerTypeSymbol containingType,
Expand All @@ -28,25 +27,26 @@ internal SourceDestructorSymbol(

bool modifierErrors;
var declarationModifiers = MakeModifiers(syntax.Modifiers, location, diagnostics, out modifierErrors);
this.MakeFlags(methodKind, declarationModifiers, returnsVoid: true, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled);

bool hasBlockBody = syntax.Body != null;
bool isExpressionBodied = !hasBlockBody && syntax.ExpressionBody != null;

this.MakeFlags(methodKind, declarationModifiers, returnsVoid: true, isExpressionBodied: isExpressionBodied, isExtensionMethod: false, isNullableAnalysisEnabled: isNullableAnalysisEnabled);

if (syntax.Identifier.ValueText != containingType.Name)
{
diagnostics.Add(ErrorCode.ERR_BadDestructorName, syntax.Identifier.GetLocation());
}

bool hasBlockBody = syntax.Body != null;
_isExpressionBodied = !hasBlockBody && syntax.ExpressionBody != null;

if (hasBlockBody || _isExpressionBodied)
if (hasBlockBody || isExpressionBodied)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi May 11, 2023

Choose a reason for hiding this comment

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

could be replaced with HasAnyBody. Not sure how such refactorings are viewed though.

{
if (IsExtern)
{
diagnostics.Add(ErrorCode.ERR_ExternHasBody, location, this);
}
}

if (!modifierErrors && !hasBlockBody && !_isExpressionBodied && !IsExtern)
if (!modifierErrors && !hasBlockBody && !isExpressionBodied && !IsExtern)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi May 11, 2023

Choose a reason for hiding this comment

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

could be replaced with !HasAnyBody. Not sure how such refactorings are viewed though.

{
diagnostics.Add(ErrorCode.ERR_ConcreteMissingBody, location, this);
}
Expand Down Expand Up @@ -142,14 +142,6 @@ public override string Name
get { return WellKnownMemberNames.DestructorName; }
}

internal override bool IsExpressionBodied
{
get
{
return _isExpressionBodied;
}
}

internal override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarations()
{
// destructors can't have return type attributes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public SourceEventAccessorSymbol(
string aliasQualifierOpt,
bool isAdder,
bool isIterator,
bool isNullableAnalysisEnabled)
bool isNullableAnalysisEnabled,
bool isExpressionBodied)
: base(@event.containingType, syntaxReference, location, isIterator)
{
_event = @event;
Expand All @@ -56,6 +57,7 @@ public SourceEventAccessorSymbol(
isAdder ? MethodKind.EventAdd : MethodKind.EventRemove,
@event.Modifiers,
returnsVoid: false, // until we learn otherwise (in LazyMethodChecks).
isExpressionBodied: isExpressionBodied,
isExtensionMethod: false,
isNullableAnalysisEnabled: isNullableAnalysisEnabled,
isMetadataVirtualIgnoringModifiers: @event.IsExplicitInterfaceImplementation && (@event.Modifiers & DeclarationModifiers.Static) == 0);
Expand Down Expand Up @@ -246,11 +248,5 @@ protected string GetOverriddenAccessorName(SourceEventSymbol @event, bool isAdde

return null;
}

internal override bool IsExpressionBodied
{
// Events cannot be expression-bodied
Copy link
Member Author

Choose a reason for hiding this comment

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

def a wrong statement.

get { return false; }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,31 @@ protected struct Flags
{
// We currently pack everything into a 32 bit int with the following layout:
//
// | |n|vvv|yy|s|r|q|z|wwwww|
// | |a|b|e|n|vvv|yy|s|r|q|z|kk|wwwww|
//
// w = method kind. 5 bits.
// k = ref kind. 2 bits.
// z = isExtensionMethod. 1 bit.
// q = isMetadataVirtualIgnoringInterfaceChanges. 1 bit.
// r = isMetadataVirtual. 1 bit. (At least as true as isMetadataVirtualIgnoringInterfaceChanges.)
// s = isMetadataVirtualLocked. 1 bit.
// y = ReturnsVoid. 2 bits.
// v = NullableContext. 3 bits.
// n = IsNullableAnalysisEnabled. 1 bit.
// e = IsExpressionBody. 1 bit.
// b = HasAnyBody. 1 bit.
// a = IsVarArg. 1 bit
private int _flags;

private const int MethodKindOffset = 0;
private const int MethodKindSize = 5;
private const int MethodKindMask = (1 << MethodKindSize) - 1;

private const int RefKindOffset = MethodKindOffset + MethodKindSize;
private const int RefKindSize = 2;
private const int RefKindMask = (1 << RefKindSize) - 1;

private const int IsExtensionMethodOffset = MethodKindOffset + MethodKindSize;
private const int IsExtensionMethodOffset = RefKindOffset + RefKindSize;
private const int IsExtensionMethodSize = 1;

private const int IsMetadataVirtualIgnoringInterfaceChangesOffset = IsExtensionMethodOffset + IsExtensionMethodSize;
Expand All @@ -56,24 +65,33 @@ protected struct Flags

private const int NullableContextOffset = ReturnsVoidOffset + ReturnsVoidSize;
private const int NullableContextSize = 3;
private const int NullableContextMask = (1 << NullableContextSize) - 1;

private const int IsNullableAnalysisEnabledOffset = NullableContextOffset + NullableContextSize;
#pragma warning disable IDE0051 // Remove unused private members
private const int IsNullableAnalysisEnabledSize = 1;
#pragma warning restore IDE0051 // Remove unused private members

private const int MethodKindMask = (1 << MethodKindSize) - 1;
private const int IsExpressionBodiedOffset = IsNullableAnalysisEnabledOffset + IsNullableAnalysisEnabledSize;
private const int IsExpressionBodiedSize = 1;

private const int HasAnyBodyOffset = IsExpressionBodiedOffset + IsExpressionBodiedSize;
private const int HasAnyBodySize = 1;

private const int IsVarArgOffset = HasAnyBodyOffset + HasAnyBodySize;
#pragma warning disable IDE0051 // Remove unused private members
private const int IsVarArgSize = 1;
#pragma warning restore IDE0051 // Remove unused private members

private const int HasAnyBodyBit = 1 << HasAnyBodyOffset;
private const int IsExpressionBodiedBit = 1 << IsExpressionBodiedOffset;
private const int IsExtensionMethodBit = 1 << IsExtensionMethodOffset;
private const int IsMetadataVirtualIgnoringInterfaceChangesBit = 1 << IsMetadataVirtualIgnoringInterfaceChangesOffset;
private const int IsMetadataVirtualBit = 1 << IsMetadataVirtualIgnoringInterfaceChangesOffset;
private const int IsMetadataVirtualLockedBit = 1 << IsMetadataVirtualLockedOffset;
private const int IsVarArgBit = 1 << IsVarArgOffset;

private const int ReturnsVoidBit = 1 << ReturnsVoidOffset;
private const int ReturnsVoidIsSetBit = 1 << ReturnsVoidOffset + 1;

private const int NullableContextMask = (1 << NullableContextSize) - 1;

private const int IsNullableAnalysisEnabledBit = 1 << IsNullableAnalysisEnabledOffset;

public bool TryGetReturnsVoid(out bool value)
Expand All @@ -93,9 +111,25 @@ public MethodKind MethodKind
get { return (MethodKind)((_flags >> MethodKindOffset) & MethodKindMask); }
}

public RefKind RefKind
{
get { return (RefKind)((_flags >> RefKindOffset) & RefKindMask); }
}

public bool HasAnyBody
{
get { return (_flags & HasAnyBodyBit) != 0; }
}

public bool IsExpressionBodied
{
get { return (_flags & IsExpressionBodiedBit) != 0; }
}

public bool IsExtensionMethod
{
get { return (_flags & IsExtensionMethodBit) != 0; }
set { ThreadSafeFlagOperations.Set(ref _flags, value ? IsExtensionMethodBit : 0); }
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}

public bool IsNullableAnalysisEnabled
Expand All @@ -108,11 +142,17 @@ public bool IsMetadataVirtualLocked
get { return (_flags & IsMetadataVirtualLockedBit) != 0; }
}

public bool IsVarArg
{
get { return (_flags & IsVarArgBit) != 0; }
}

#if DEBUG
static Flags()
{
// Verify masks are sufficient for values.
Debug.Assert(EnumUtilities.ContainsAllValues<MethodKind>(MethodKindMask));
Debug.Assert(EnumUtilities.ContainsAllValues<RefKind>(RefKindMask));
Debug.Assert(EnumUtilities.ContainsAllValues<NullableContextKind>(NullableContextMask));
}
#endif
Expand All @@ -126,19 +166,22 @@ public Flags(
MethodKind methodKind,
DeclarationModifiers declarationModifiers,
bool returnsVoid,
bool isExpressionBodied,
bool isExtensionMethod,
bool isNullableAnalysisEnabled,
bool isMetadataVirtualIgnoringModifiers = false)
{
bool isMetadataVirtual = isMetadataVirtualIgnoringModifiers || ModifiersRequireMetadataVirtual(declarationModifiers);

int methodKindInt = ((int)methodKind & MethodKindMask) << MethodKindOffset;
int isExpressionBodyInt = isExpressionBodied ? IsExpressionBodiedBit : 0;
int isExtensionMethodInt = isExtensionMethod ? IsExtensionMethodBit : 0;
int isNullableAnalysisEnabledInt = isNullableAnalysisEnabled ? IsNullableAnalysisEnabledBit : 0;
int isMetadataVirtualIgnoringInterfaceImplementationChangesInt = isMetadataVirtual ? IsMetadataVirtualIgnoringInterfaceChangesBit : 0;
int isMetadataVirtualInt = isMetadataVirtual ? IsMetadataVirtualBit : 0;

_flags = methodKindInt
| isExpressionBodyInt
| isExtensionMethodInt
| isNullableAnalysisEnabledInt
| isMetadataVirtualIgnoringInterfaceImplementationChangesInt
Expand All @@ -147,6 +190,17 @@ public Flags(
| ReturnsVoidIsSetBit;
}

/// <summary>
/// Only allowed to be called in constructors of SourceMemberMethodSymbol (and derived constructors), so
/// does not need ThreadSafe operations.
/// </summary>
public void SetOrdinaryMethodFlags(RefKind refKind, bool hasAnyBody, bool isVarArg)
{
_flags |= ((int)refKind & RefKindMask) << RefKindOffset;
_flags |= hasAnyBody ? HasAnyBodyBit : 0;
_flags |= isVarArg ? IsVarArgBit : 0;
}

public bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
{
// This flag is immutable, so there's no reason to set a lock bit, as we do below.
Expand Down Expand Up @@ -292,12 +346,13 @@ protected void MakeFlags(
MethodKind methodKind,
DeclarationModifiers declarationModifiers,
bool returnsVoid,
bool isExpressionBodied,
bool isExtensionMethod,
bool isNullableAnalysisEnabled,
bool isMetadataVirtualIgnoringModifiers = false)
{
DeclarationModifiers = declarationModifiers;
this.flags = new Flags(methodKind, declarationModifiers, returnsVoid, isExtensionMethod, isNullableAnalysisEnabled, isMetadataVirtualIgnoringModifiers);
this.flags = new Flags(methodKind, declarationModifiers, returnsVoid, isExpressionBodied, isExtensionMethod, isNullableAnalysisEnabled, isMetadataVirtualIgnoringModifiers);
}

protected void SetReturnsVoid(bool returnsVoid)
Expand Down Expand Up @@ -1003,7 +1058,7 @@ protected void CheckFeatureAvailabilityAndRuntimeSupport(SyntaxNode declarationS
/// If the method has both block body and an expression body
/// present, this is not treated as expression-bodied.
/// </remarks>
internal abstract bool IsExpressionBodied { get; }
internal bool IsExpressionBodied => flags.IsExpressionBodied;

internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree)
{
Expand Down
Loading