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.

Sign up for GitHub

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

Adding IsConst modreq on ref readonly signatures #19658

Merged
merged 6 commits into from
Jun 23, 2017
Merged

Adding IsConst modreq on ref readonly signatures #19658

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e6b31c1
Adding IsConst modreq on ref readonly signatures
May 19, 2017
56764f1
First set of PR Comments
May 23, 2017
42a8f4c
Added tests
May 25, 2017
b32dc89
Responded to PR feedback
Jun 6, 2017
15c5a6c
More PR Comments
Jun 13, 2017
743a3bc
Resolve merge conflicts
Jun 22, 2017
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
27 changes: 14 additions & 13 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,33 +1002,34 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations

internal override DiagnosticInfo GetUseSiteDiagnostic()
{
DiagnosticInfo result = null;

if (!_packedFlags.IsUseSiteDiagnosticPopulated)
{
DiagnosticInfo result = null;
CalculateUseSiteDiagnostic(ref result);
EnsureTypeParametersAreLoaded(ref result);
return InitializeUseSiteDiagnostic(result);
}

var uncommonFields = _uncommonFields;
if (uncommonFields == null)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

if (uncommonFields == null) [](start = 12, length = 27)

It feels like we might be losing an optimization for this case. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took a look. We won't be loosing an optimization. CSDiagnosticInfo.IsEmpty will return false for null results. Therefore, it won't be initialized. I believe the changed code is simpler to read now.

In reply to: 119753756 [](ancestors = 119753756)

{
return null;
result = InitializeUseSiteDiagnostic(result);
}
else
{
var result = uncommonFields._lazyUseSiteDiagnostic;
return CSDiagnosticInfo.IsEmpty(result)
? InterlockedOperations.Initialize(ref uncommonFields._lazyUseSiteDiagnostic, null, CSDiagnosticInfo.EmptyErrorInfo)
: result;
result = _uncommonFields?._lazyUseSiteDiagnostic;
}

return CSDiagnosticInfo.IsEmpty(result)
? InterlockedOperations.Initialize(ref AccessUncommonFields()._lazyUseSiteDiagnostic, null, CSDiagnosticInfo.EmptyErrorInfo)
: result;
}

private DiagnosticInfo InitializeUseSiteDiagnostic(DiagnosticInfo diagnostic)
{
Debug.Assert(!CSDiagnosticInfo.IsEmpty(diagnostic));
if (_packedFlags.IsUseSiteDiagnosticPopulated)
{
return _uncommonFields?._lazyUseSiteDiagnostic;
}

if (diagnostic != null)
{
Debug.Assert(!CSDiagnosticInfo.IsEmpty(diagnostic));
diagnostic = InterlockedOperations.Initialize(ref AccessUncommonFields()._lazyUseSiteDiagnostic, diagnostic, CSDiagnosticInfo.EmptyErrorInfo);
}

Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Reflection.Metadata;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE
Expand Down Expand Up @@ -301,6 +303,12 @@ public PEParameterSymbolWithCustomModifiers(
_customModifiers = CSharpCustomModifier.Convert(customModifiers);
_refCustomModifiers = CSharpCustomModifier.Convert(refCustomModifiers);

if (this.RefKind != RefKind.RefReadOnly && _refCustomModifiers.Any(modifier => !modifier.IsOptional && modifier.Modifier.IsWellKnownTypeIsConst()))
{
// IsConst modreq is only accepted on RefReadOnly symbols
isBad = true;
}

Debug.Assert(_refCustomModifiers.IsEmpty || isByRef);
}

Expand Down
15 changes: 10 additions & 5 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Reflection.Metadata;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.DocumentationComments;
using Roslyn.Utilities;
using Microsoft.CodeAnalysis.CSharp.Emit;

namespace Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE
Expand Down Expand Up @@ -68,6 +68,7 @@ internal static PEPropertySymbol Create(
var propertyParams = metadataDecoder.GetSignatureForProperty(handle, out callingConvention, out propEx);
Debug.Assert(propertyParams.Length > 0);

var isBad = false;
var returnInfo = propertyParams[0];
PEPropertySymbol result;

Expand All @@ -77,10 +78,10 @@ internal static PEPropertySymbol Create(
}
else
{
result = new PEPropertySymbolWithCustomModifiers(moduleSymbol, containingType, handle, getMethod, setMethod, propertyParams, metadataDecoder);
result = new PEPropertySymbolWithCustomModifiers(moduleSymbol, containingType, handle, getMethod, setMethod, propertyParams, metadataDecoder, out isBad);
}

if (propEx != null)
if (propEx != null || isBad)
{
result._lazyUseSiteDiagnostic = new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, result);
}
Expand Down Expand Up @@ -730,14 +731,18 @@ public PEPropertySymbolWithCustomModifiers(
PEMethodSymbol getMethod,
PEMethodSymbol setMethod,
ParamInfo<TypeSymbol>[] propertyParams,
MetadataDecoder metadataDecoder)
MetadataDecoder metadataDecoder,
out bool isBad)
: base (moduleSymbol, containingType, handle, getMethod, setMethod,
propertyParams[0].CustomModifiers.NullToEmpty().Length + propertyParams[0].RefCustomModifiers.NullToEmpty().Length,
propertyParams, metadataDecoder)
{
var returnInfo = propertyParams[0];
_typeCustomModifiers = CSharpCustomModifier.Convert(returnInfo.CustomModifiers);
_refCustomModifiers = CSharpCustomModifier.Convert(returnInfo.RefCustomModifiers);

// IsConst modreq is only accepted on RefReadOnly symbols
isBad = this.RefKind != RefKind.RefReadOnly && _refCustomModifiers.Any(modifier => !modifier.IsOptional && modifier.Modifier.IsWellKnownTypeIsConst());
}

public override ImmutableArray<CustomModifier> TypeCustomModifiers
Expand Down
7 changes: 6 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Metadata/PE/SymbolFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,16 @@ internal override Cci.PrimitiveTypeCode GetPrimitiveTypeCode(PEModuleSymbol modu
return type.PrimitiveTypeCode;
}

internal override bool IsVolatileModifierType(PEModuleSymbol moduleSymbol, TypeSymbol type)
internal override bool IsAcceptedVolatileModifierType(PEModuleSymbol moduleSymbol, TypeSymbol type)
{
return type.SpecialType == SpecialType.System_Runtime_CompilerServices_IsVolatile;
}

internal override bool IsAcceptedIsConstModifierType(TypeSymbol type)
{
return type.IsWellKnownTypeIsConst();
}
Copy link
Contributor Author

@OmarTawfik OmarTawfik May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Comparing symbols by full name is probably not be the best way to do it. Looking up a better alternative. #Closed

Copy link
Member

@jaredpar jaredpar May 22, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

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

Consider a similar scenario like IsVolatileModfifierType. Can we re-use that approach here? #Resolved

Copy link
Contributor Author

@OmarTawfik OmarTawfik May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, no. Unless there is something I'm missing, IsConst is not a special library type :(

In reply to: 117829472 [](ancestors = 117829472)

Copy link
Member

@cston cston May 22, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

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

Perhaps the same approach as SymbolFactory.GetSystemTypeSymbol. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

we need a bug or a PROTOTYPE marker to track this

In reply to: 117561528 [](ancestors = 117561528)

Copy link
Member

@jaredpar jaredpar May 23, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

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

This seems simple enough to fix now vs. needing a PROTOTYPE comment to fix later. #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Comparing symbols by full name is probably not be the best way to do it. Looking up a better alternative.

Given that this is not a SpecialType, I think name comparison is the only right way to do that. If performance is a concern we can compare type name and namespace names separately, then calling ToDisplayString won't be needed. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

If going with name comparing, should definitely compare namespace/type name parts separately. I think ToDisplay will concat them every time

In reply to: 118063143 [](ancestors = 118063143)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Planning on fixing now.

In reply to: 117888944 [](ancestors = 117888944,117561528)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I tracked down, it ends up doing a lookup in NamespaceOrTypeSymbol based on string comparison as well. Will separate the comparison to namespace/type pair to improve performance.

In reply to: 117866143 [](ancestors = 117866143)

Copy link
Member

@cston cston May 26, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

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

Could we simplify IsWellKnownTypeIsConst by caching the IsConst type on the PEModuleSymbol, similar to the handling of System.Type in SymbolFactory.GetSystemTypeSymbol. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please explain? caching what? this is not doing lookup, but rather checking the passed symbol for full name equality.

In reply to: 118763594 [](ancestors = 118763594)


internal override TypeSymbol GetSZArrayTypeSymbol(PEModuleSymbol moduleSymbol, TypeSymbol elementType, ImmutableArray<ModifierInfo<TypeSymbol>> customModifiers)
{
if (elementType is UnsupportedMetadataTypeSymbol)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/PropertySymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public static bool HasRefOrOutParameter(this PropertySymbol property)
{
foreach (ParameterSymbol param in property.Parameters)
{
if (param.RefKind != RefKind.None)
if (param.RefKind == RefKind.Ref || param.RefKind == RefKind.Out)
Copy link
Contributor

@AlekseyTs AlekseyTs May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

if (param.RefKind == RefKind.Ref || param.RefKind == RefKind.Out) [](start = 16, length = 65)

Did you add a test backing this change? Could you point to the test(s)? #Closed

Copy link
Contributor Author

@OmarTawfik OmarTawfik May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Adding IsConstModReqIsConsumedInRefCustomModifiersPosition_Indexers_ReturnTypes. This change basically allows to pass ref-readonly arguments to indexer methods. Passing ordinary arguments is the same to properties as ref-readonly arguments.

In reply to: 118061963 [](ancestors = 118061963)

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding IsConstModReqIsConsumedInRefCustomModifiersPosition_Indexers_ReturnTypes. This change basically allows to pass ref-readonly arguments to indexer methods. Passing ordinary arguments is the same to properties as ref-readonly arguments.

I couldn't find the test, what file it should be in?

In reply to: 118609191 [](ancestors = 118609191,118061963)

Copy link
Contributor Author

@OmarTawfik OmarTawfik Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

File IsConstModifierTests.cs in C# emit tests project. You can look forIsConstModReqIsConsumedInRefCustomModifiersPosition_Code_Indexers_ReturnTypes now.

In reply to: 119789809 [](ancestors = 119789809,118609191,118061963)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction: IsConstModReqIsConsumedInRefCustomModifiersPosition_Code_Indexers_Parameters*

In reply to: 120218968 [](ancestors = 120218968,119789809,118609191,118061963)

{
return true;
}
Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/CustomModifierUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ internal static void CopyMethodCustomModifiers(
// have already been compared.
MethodSymbol constructedSourceMethod = sourceMethod.ConstructIfGeneric(destinationMethod.TypeArguments);

customModifiers = CustomModifiersTuple.Create(constructedSourceMethod.ReturnTypeCustomModifiers,
destinationMethod.ReturnsByRef ? constructedSourceMethod.RefCustomModifiers : ImmutableArray<CustomModifier>.Empty);
customModifiers = CustomModifiersTuple.Create(
constructedSourceMethod.ReturnTypeCustomModifiers,
destinationMethod.ReturnsByRef || destinationMethod.ReturnsByRefReadonly ? constructedSourceMethod.RefCustomModifiers : ImmutableArray<CustomModifier>.Empty);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

destinationMethod.ReturnsByRef || destinationMethod.ReturnsByRefReadonly [](start = 16, length = 73)

Could we simply check if RefKind is None? #Closed


parameters = CopyParameterCustomModifiers(constructedSourceMethod.Parameters, destinationMethod.Parameters, alsoCopyParamsModifier);

Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ private void ComputeParameters()
arglistToken: out arglistToken,
allowRefOrOut: true,
allowThis: true,
addIsConstModifier: false,
diagnostics: diagnostics);

ParameterHelpers.EnsureIsReadOnlyAttributeExists(parameters, diagnostics, modifyCompilationForRefReadOnly: false);
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ public static ImmutableArray<ParameterSymbol> MakeParameters(
out SyntaxToken arglistToken,
DiagnosticBag diagnostics,
bool allowRefOrOut,
bool allowThis)
bool allowThis,
bool addIsConstModifier)
{
arglistToken = default(SyntaxToken);

Expand Down Expand Up @@ -90,6 +91,7 @@ public static ImmutableArray<ParameterSymbol> MakeParameters(
parameterIndex,
(paramsKeyword.Kind() != SyntaxKind.None),
parameterIndex == 0 && thisKeyword.Kind() != SyntaxKind.None,
addIsConstModifier,
diagnostics);

ReportParameterErrors(owner, parameterSyntax, parameter, firstDefault, diagnostics);
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ protected override void MethodChecks(DiagnosticBag diagnostics)
bodyBinder, this, parameterList, out arglistToken,
allowRefOrOut: true,
allowThis: false,
addIsConstModifier: false,
diagnostics: diagnostics);

_lazyIsVararg = (arglistToken.Kind() == SyntaxKind.ArgListKeyword);
Expand Down
22 changes: 18 additions & 4 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,8 @@ internal override LexicalSortKey GetLexicalSortKey()

private sealed class InvokeMethod : SourceDelegateMethodSymbol
{
private readonly RefKind refKind;
private readonly RefKind _refKind;
private readonly ImmutableArray<CustomModifier> _refCustomModifiers;

internal InvokeMethod(
SourceMemberContainerTypeSymbol delegateType,
Expand All @@ -242,13 +243,14 @@ internal InvokeMethod(
DiagnosticBag diagnostics)
: base(delegateType, returnType, syntax, MethodKind.DelegateInvoke, DeclarationModifiers.Virtual | DeclarationModifiers.Public)
{
this.refKind = refKind;
this._refKind = refKind;

SyntaxToken arglistToken;
var parameters = ParameterHelpers.MakeParameters(
binder, this, syntax.ParameterList, out arglistToken,
allowRefOrOut: true,
allowThis: false,
addIsConstModifier: true,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

addIsConstModifier: true, [](start = 20, length = 25)

Are we testing presence of modifiers on EndInvoke and BeginInvoke? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and modified tests
C# IsConstIsWrittenOnRefReadOnlyMembers_Delegates_*
VB UsingLambdasOfRefReadOnlyDelegatesIsNotSupported_*

In reply to: 119757863 [](ancestors = 119757863)

diagnostics: diagnostics);

if (arglistToken.Kind() == SyntaxKind.ArgListKeyword)
Expand All @@ -259,6 +261,16 @@ internal InvokeMethod(
diagnostics.Add(ErrorCode.ERR_IllegalVarArgs, new SourceLocation(arglistToken));
}

if (_refKind == RefKind.RefReadOnly)
{
var isConstType = binder.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, diagnostics, syntax.ReturnType);
_refCustomModifiers = ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType));
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

_refCustomModifiers = ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType)); [](start = 20, length = 94)

Are we testing presence of modifiers on EndInvoke? #Closed

}
else
{
_refCustomModifiers = ImmutableArray<CustomModifier>.Empty;
}

InitializeParameters(parameters);
}

Expand All @@ -269,7 +281,7 @@ public override string Name

internal override RefKind RefKind
{
get { return refKind; }
get { return _refKind; }
}

internal override LexicalSortKey GetLexicalSortKey()
Expand All @@ -287,14 +299,16 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
{
base.AfterAddingTypeMembersChecks(conversions, diagnostics);

if (refKind == RefKind.RefReadOnly)
if (_refKind == RefKind.RefReadOnly)
{
var syntax = (DelegateDeclarationSyntax)SyntaxRef.GetSyntax();
DeclaringCompilation.EnsureIsReadOnlyAttributeExists(diagnostics, syntax.ReturnType.GetLocation(), modifyCompilationForRefReadOnly: true);
}

ParameterHelpers.EnsureIsReadOnlyAttributeExists(Parameters, diagnostics, modifyCompilationForRefReadOnly: true);
}

public override ImmutableArray<CustomModifier> RefCustomModifiers => _refCustomModifiers;
}

private sealed class BeginInvokeMethod : SourceDelegateMethodSymbol
Expand Down
13 changes: 11 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ private void MethodChecks(MethodDeclarationSyntax syntax, Binder withTypeParamsB
signatureBinder, this, syntax.ParameterList, out arglistToken,
allowRefOrOut: true,
allowThis: true,
addIsConstModifier: IsVirtual || IsAbstract,
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

IsVirtual || IsAbst [](start = 36, length = 19)

This should probably include checks for override and explicit implementation, those are virtual (in metadata terms) as well and the difference will be observable for cases when the target method cannot be found, etc. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #20053 for that.

In reply to: 119902520 [](ancestors = 119902520)

diagnostics: diagnostics);

_lazyIsVararg = (arglistToken.Kind() == SyntaxKind.ArgListKeyword);
Expand Down Expand Up @@ -320,11 +321,19 @@ private void MethodChecks(MethodDeclarationSyntax syntax, Binder withTypeParamsB

if ((object)overriddenMethod != null)
{
CustomModifierUtils.CopyMethodCustomModifiers(overriddenMethod, this, out _lazyReturnType,
out _lazyCustomModifiers,
CustomModifierUtils.CopyMethodCustomModifiers(overriddenMethod, this, out _lazyReturnType,
Copy link
Contributor

@AlekseyTs AlekseyTs May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

CustomModifierUtils.CopyMethodCustomModifiers(overriddenMethod, this, out _lazyReturnType, [](start = 24, length = 90)

Is it Ok to end up without IsCons modifier for overriding case (overriding method is technically virtual)? Do we have a test for scenario like that? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that the overriding method should always copy the parent's modifiers (for CLR to consider it as an override). And if the overridden method is a C# 7.1 virtual method with a ref-readonly parameter or a return type, then it will have this modifier.
Am I missing something? when is it ever possible to deviate from the overridden method?

In reply to: 118078081 [](ancestors = 118078081)

Copy link
Contributor

@AlekseyTs AlekseyTs May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Am I missing something? when is it ever possible to deviate from the overridden method?

For proper overriding we should match modifiers exactly. I guess we have to decide whether it is fine to override without modifier, or should that be an error. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will keep the current behavior, and add a test to assert it.
Will also file a bug to make sure we decide whether we want to support overriding a ref-readonly method that has no modifier or not.

In reply to: 118420724 [](ancestors = 118420724)

out _lazyCustomModifiers,
out _lazyParameters, alsoCopyParamsModifier: true);
}
}
else if (_refKind == RefKind.RefReadOnly)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

else if (_refKind == RefKind.RefReadOnly) [](start = 16, length = 41)

I think this code should be moved before the if (syntax.ExplicitInterfaceSpecifier == null) above and combined with code _lazyCustomModifiers = CustomModifiersTuple.Empty; on line 298. Otherwise, we end up without the modifier for error cases when an overriden or implemented method is not found. Please add tests for scenarios like that. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #20053 to track that.

In reply to: 119768491 [](ancestors = 119768491)

{
var isConstType = withTypeParamsBinder.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, diagnostics, syntax.ReturnType);

_lazyCustomModifiers = CustomModifiersTuple.Create(
typeCustomModifiers: ImmutableArray<CustomModifier>.Empty,
refCustomModifiers: ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType)));
}
}
else if ((object)_explicitInterfaceType != null)
{
Expand Down
20 changes: 20 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public static SourceParameterSymbol Create(
int ordinal,
bool isParams,
bool isExtensionMethodThis,
bool addIsConstModifier,
DiagnosticBag declarationDiagnostics)
{
var name = identifier.ValueText;
Expand All @@ -48,6 +49,25 @@ public static SourceParameterSymbol Create(
identifier.Parent.GetLocation());
}

if (addIsConstModifier && refKind == RefKind.RefReadOnly)
{
var isConstType = context.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, declarationDiagnostics, syntax);

return new SourceComplexParameterSymbolWithCustomModifiers(
owner,
ordinal,
parameterType,
refKind,
ImmutableArray<CustomModifier>.Empty,
ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType)),
name,
locations,
syntax.GetReference(),
ConstantValue.Unset,
isParams,
isExtensionMethodThis);
}

if (!isParams &&
!isExtensionMethodThis &&
(syntax.Default == null) &&
Expand Down
13 changes: 11 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,14 @@ private SourcePropertySymbol(
_lazyParameters = CustomModifierUtils.CopyParameterCustomModifiers(overriddenOrImplementedProperty.Parameters, _lazyParameters, alsoCopyParamsModifier: isOverride);
}
}
else if (_refKind == RefKind.RefReadOnly)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

else if (_refKind == RefKind.RefReadOnly) [](start = 12, length = 41)

Same comment as for methods, should move it before if (isExplicitInterfaceImplementation || this.IsOverride). Please add tests. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #20053 for that.

In reply to: 119769069 [](ancestors = 119769069)

{
var isConstType = bodyBinder.GetWellKnownType(WellKnownType.System_Runtime_CompilerServices_IsConst, diagnostics, syntax.Type);

_customModifiers = CustomModifiersTuple.Create(
ImmutableArray<CustomModifier>.Empty,
ImmutableArray.Create(CSharpCustomModifier.CreateRequired(isConstType)));
}

if (!hasAccessorList)
{
Expand Down Expand Up @@ -775,7 +783,7 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, bool isExp
}

private static ImmutableArray<ParameterSymbol> MakeParameters(
Binder binder, SourcePropertySymbol owner, BaseParameterListSyntax parameterSyntaxOpt, DiagnosticBag diagnostics)
Binder binder, SourcePropertySymbol owner, BaseParameterListSyntax parameterSyntaxOpt, DiagnosticBag diagnostics, bool addIsConstModifier)
{
if (parameterSyntaxOpt == null)
{
Expand All @@ -792,6 +800,7 @@ private static ImmutableArray<ParameterSymbol> MakeParameters(
binder, owner, parameterSyntaxOpt, out arglistToken,
allowRefOrOut: false,
allowThis: false,
addIsConstModifier: addIsConstModifier,
diagnostics: diagnostics);

if (arglistToken.Kind() != SyntaxKind.None)
Expand Down Expand Up @@ -1388,7 +1397,7 @@ private TypeSymbol ComputeType(Binder binder, BasePropertyDeclarationSyntax synt
private ImmutableArray<ParameterSymbol> ComputeParameters(Binder binder, BasePropertyDeclarationSyntax syntax, DiagnosticBag diagnostics)
{
var parameterSyntaxOpt = GetParameterListSyntax(syntax);
var parameters = MakeParameters(binder, this, parameterSyntaxOpt, diagnostics);
var parameters = MakeParameters(binder, this, parameterSyntaxOpt, diagnostics, addIsConstModifier: IsVirtual || IsAbstract);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

IsVirtual || IsAbstract [](start = 111, length = 23)

Same comment as for methods. #Closed

HashSet<DiagnosticInfo> useSiteDiagnostics = null;

foreach (ParameterSymbol param in parameters)
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceUserDefinedOperatorSymbolBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ protected override void MethodChecks(DiagnosticBag diagnostics)
out arglistToken,
allowRefOrOut: true,
allowThis: false,
addIsConstModifier: false,
diagnostics: diagnostics);

if (arglistToken.Kind() == SyntaxKind.ArgListKeyword)
Expand Down
Loading