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

Jump to bottom

Extended partial methods #43582

Merged
merged 14 commits into from
May 5, 2020
Merged

Extended partial methods #43582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c95be2c
Extended partial methods
RikkiGibson Apr 23, 2020
747393f
Fix explicit accessibility checks and tests
RikkiGibson Apr 23, 2020
3d99838
Diagnostics for extended partial method modifiers
RikkiGibson Apr 24, 2020
88ba459
Fix diagnostic message
RikkiGibson Apr 28, 2020
94e8743
Check both parts in IsNew
RikkiGibson Apr 29, 2020
a25dfc8
Require equal combinations of 'new' modifiers
RikkiGibson Apr 29, 2020
fa9532e
Address some feedback
RikkiGibson Apr 29, 2020
cd76a21
Address some feedback
RikkiGibson Apr 30, 2020
96a7429
Add some tests
RikkiGibson Apr 30, 2020
3b64567
Address feedback
RikkiGibson Apr 30, 2020
b262784
Remove unused error codes. Restore some tests.
RikkiGibson May 1, 2020
3211eb1
Address feedback and add more tests
RikkiGibson May 2, 2020
ece12e6
CS9051->CS8794. Test inaccessible partial methods.
RikkiGibson May 4, 2020
16c7335
Fix up some diagnostic comments in tests
RikkiGibson May 4, 2020
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
29 changes: 22 additions & 7 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2112,14 +2112,11 @@ If such a class is used as a base class and if the deriving class defines a dest
<value>Inconsistent lambda parameter usage; parameter types must be all explicit or all implicit</value>
</data>
<data name="ERR_PartialMethodInvalidModifier" xml:space="preserve">
<value>A partial method cannot have access modifiers or the virtual, abstract, override, new, sealed, or extern modifiers</value>
<value>A partial method cannot have the 'abstract' modifier</value>
</data>
<data name="ERR_PartialMethodOnlyInPartialClass" xml:space="preserve">
<value>A partial method must be declared within a partial class, partial struct, or partial interface</value>
</data>
<data name="ERR_PartialMethodCannotHaveOutParameters" xml:space="preserve">
<value>A partial method cannot have out parameters</value>
</data>
<data name="ERR_PartialMethodNotExplicit" xml:space="preserve">
<value>A partial method may not explicitly implement an interface method</value>
</data>
Expand Down Expand Up @@ -2156,9 +2153,6 @@ If such a class is used as a base class and if the deriving class defines a dest
<data name="ERR_PartialMethodInExpressionTree" xml:space="preserve">
<value>Partial methods with only a defining declaration or removed conditional methods cannot be used in expression trees</value>
</data>
<data name="ERR_PartialMethodMustReturnVoid" xml:space="preserve">
<value>Partial methods must have a void return type</value>
</data>
<data name="WRN_ObsoleteOverridingNonObsolete" xml:space="preserve">
<value>Obsolete member '{0}' overrides non-obsolete member '{1}'</value>
</data>
Expand Down Expand Up @@ -6070,4 +6064,25 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_GeneratorFailedDuringInitialization_Title" xml:space="preserve">
<value>Generator failed to initialize.</value>
</data>
<data name="IDS_FeatureExtendedPartialMethods" xml:space="preserve">
<value>extended partial methods</value>
</data>
<data name="ERR_PartialMethodWithNonVoidReturnMustHaveAccessMods" xml:space="preserve">
<value>Partial method '{0}' must have accessibility modifiers because it has a non-void return type.</value>
</data>
<data name="ERR_PartialMethodWithOutParamMustHaveAccessMods" xml:space="preserve">
<value>Partial method '{0}' must have accessibility modifiers because it has 'out' parameters.</value>
</data>
<data name="ERR_PartialMethodWithAccessibilityModsMustHaveImplementation" xml:space="preserve">
<value>Partial method '{0}' must have an implementation part because it has accessibility modifiers.</value>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

Based on the LDM discussions we should not have separate errors for these violations. The requirement for an implementation is dictated by the presence of an explicit accessibility modifier. That is what mandates their is an implementation, not everything else.

At the same time all of these new scenarios must be predicated on an explicit accessibility modifier. Basically if you see out that is illegal unless there is an explicit accessibility modifier

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

<data name="ERR_PartialMethodWithExtendedModMustHaveAccessMods" xml:space="preserve">
<value>Partial method '{0}' must have accessibility modifiers because it has a 'virtual', 'override', 'sealed', 'new', or 'extern' modifier.</value>
</data>
<data name="ERR_PartialMethodAccessibilityDifference" xml:space="preserve">
<value>Both partial method declarations must have identical accessibility modifiers.</value>
</data>
<data name="ERR_PartialMethodExtendedModDifference" xml:space="preserve">
<value>Both partial method declarations must have identical combinations of 'virtual', 'override', 'sealed', and 'new' modifiers.</value>
</data>
</root>
15 changes: 13 additions & 2 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ internal enum ErrorCode
ERR_InconsistentLambdaParameterUsage = 748,
ERR_PartialMethodInvalidModifier = 750,
ERR_PartialMethodOnlyInPartialClass = 751,
ERR_PartialMethodCannotHaveOutParameters = 752,
// ERR_PartialMethodCannotHaveOutParameters = 752, Removed as part of 'extended partial methods' feature
// ERR_PartialMethodOnlyMethods = 753, Removed as it is subsumed by ERR_PartialMisplaced
ERR_PartialMethodNotExplicit = 754,
ERR_PartialMethodExtensionDifference = 755,
Expand All @@ -544,7 +544,7 @@ internal enum ErrorCode
ERR_PartialMethodStaticDifference = 763,
ERR_PartialMethodUnsafeDifference = 764,
ERR_PartialMethodInExpressionTree = 765,
ERR_PartialMethodMustReturnVoid = 766,
// ERR_PartialMethodMustReturnVoid = 766, Removed as part of 'extended partial methods' feature
ERR_ExplicitImplCollisionOnRefOut = 767,
ERR_IndirectRecursiveConstructorCall = 768,

Expand Down Expand Up @@ -1777,6 +1777,17 @@ internal enum ErrorCode
ERR_ExpressionTreeContainsFromEndIndexExpression = 8791,
ERR_ExpressionTreeContainsRangeExpression = 8792,

#region diagnostics introduced for C# 9

ERR_PartialMethodWithAccessibilityModsMustHaveImplementation = 8793,
ERR_PartialMethodWithNonVoidReturnMustHaveAccessMods = 8794,
ERR_PartialMethodWithOutParamMustHaveAccessMods = 8795,
ERR_PartialMethodWithExtendedModMustHaveAccessMods = 8796,
ERR_PartialMethodAccessibilityDifference = 8797,
ERR_PartialMethodExtendedModDifference = 8798,

#endregion

// Note: you will need to re-generate compiler code after adding warnings (eng\generate-compiler-code.cmd)
}
}
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ internal enum MessageID
IDS_FeatureMemberNotNull = MessageBase + 12768,
IDS_FeatureNativeInt = MessageBase + 12769,
IDS_FeatureTargetTypedObjectCreation = MessageBase + 12770,
IDS_FeatureExtendedPartialMethods = MessageBase + 12771
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -305,6 +306,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureTargetTypedObjectCreation: // syntax check
case MessageID.IDS_FeatureMemberNotNull:
case MessageID.IDS_FeatureNativeInt:
case MessageID.IDS_FeatureExtendedPartialMethods: // semantic check
return LanguageVersion.Preview;

// C# 8.0 features.
Expand Down
7 changes: 0 additions & 7 deletions src/Compilers/CSharp/Portable/Symbols/Source/ModifierUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,6 @@ internal static DeclarationModifiers CheckModifiers(
modifierErrors = true;
}

bool isMethod = (allowedModifiers & (DeclarationModifiers.Partial | DeclarationModifiers.Virtual)) == (DeclarationModifiers.Partial | DeclarationModifiers.Virtual);
if (isMethod && ((result & (DeclarationModifiers.Partial | DeclarationModifiers.Private)) == (DeclarationModifiers.Partial | DeclarationModifiers.Private)))
{
diagnostics.Add(ErrorCode.ERR_PartialMethodInvalidModifier, errorLocation);
modifierErrors = true;
}

if ((result & DeclarationModifiers.PrivateProtected) != 0)
{
modifierErrors |= !Binder.CheckFeatureAvailability(errorLocation.SourceTree, MessageID.IDS_FeaturePrivateProtected, diagnostics, errorLocation);
Expand Down
13 changes: 10 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1682,10 +1682,13 @@ private void CheckMemberNameConflicts(DiagnosticBag diagnostics)
// UNDONE: Consider adding a secondary location pointing to the second method.
private void ReportMethodSignatureCollision(DiagnosticBag diagnostics, SourceMemberMethodSymbol method1, SourceMemberMethodSymbol method2)
{
// Partial methods are allowed to collide by signature.
if (method1.IsPartial && method2.IsPartial)
switch (method1, method2)
Copy link
Member

@cston cston May 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

switch (method1, method2) [](start = 12, length = 25)

Does this switch statement represent a change in behavior from the original if?

Unless there is a change in behavior, we should stick with the if statement since it is significantly simpler. #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.

Yes, this fixes #43883. This solution ends up creating additional diagnostics in some error scenarios. I tried moving the checks for PartialMethodOnlyOneLatent/PartialMethodOnlyOneActual in here, but because partial method merging already happened in an earlier step, it causes us to miss diagnostics for multiple "actual" methods.

If we could do this at the same time or right before we merge partial methods, we might be able to get higher quality diagnostics in some scenarios when members have conflicting signatures:

  1. if the methods are not 'partial', we report ErrorCode.ERR_MemberAlreadyExists
  2. if we detect that both methods are partial implementations, we give ErrorCode.ERR_PartialMethodOnlyOneActual
  3. if we detect that both methods are partial definitions, we give ErrorCode.ERR_PartialMethodOnlyOneLatent

It might also be possible to fix this by modifying the partial method signature comparer, that groups declarations that might be part of the same partial method, to be aware of RefKind differences.

Copy link
Member

Choose a reason for hiding this comment

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

This solution ends up creating additional diagnostics in some error scenarios.

That sounds ok. We can revisit this later if necessary.

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

{
return;
case (SourceOrdinaryMethodSymbol { IsPartialDefinition: true }, SourceOrdinaryMethodSymbol { IsPartialImplementation: true }):
case (SourceOrdinaryMethodSymbol { IsPartialImplementation: true }, SourceOrdinaryMethodSymbol { IsPartialDefinition: true }):
// these could be 2 parts of the same partial method.
// Partial methods are allowed to collide by signature.
return;
}

// If method1 is a constructor only because its return type is missing, then
Expand Down Expand Up @@ -2566,6 +2569,10 @@ private static void MergePartialMembers(
{
diagnostics.Add(ErrorCode.ERR_PartialMethodInconsistentTupleNames, method.Locations[0], method, method.OtherPartOfPartial);
}
else if (method is { IsPartialDefinition: true, OtherPartOfPartial: null, HasExplicitAccessModifier: true })
{
diagnostics.Add(ErrorCode.ERR_PartialMethodWithAccessibilityModsMustHaveImplementation, method.Locations[0], method);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...pilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,7 @@ private static void CheckOverrideMember(Symbol overridingMember, OverriddenOrHid
diagnostics.Add(ErrorCode.ERR_CantOverrideSealed, overridingMemberLocation, overridingMember, overriddenMember);
suppressAccessors = true;
}
else if (!overridingMember.IsPartialMethod() && !OverrideHasCorrectAccessibility(overriddenMember, overridingMember))
else if (!OverrideHasCorrectAccessibility(overriddenMember, overridingMember))
{
var accessibility = SyntaxFacts.GetText(overriddenMember.DeclaredAccessibility);
diagnostics.Add(ErrorCode.ERR_CantChangeAccessOnOverride, overridingMemberLocation, overridingMember, accessibility, overriddenMember);
Expand Down
10 changes: 9 additions & 1 deletion 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 @@ -441,14 +441,22 @@ public override Accessibility DeclaredAccessibility
}
}

public sealed override bool IsExtern
internal bool HasExternModifier
{
get
{
return (this.DeclarationModifiers & DeclarationModifiers.Extern) != 0;
}
}

public override bool IsExtern
{
get
{
return HasExternModifier;
}
}

public sealed override bool IsSealed
{
get
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,8 @@ private void DecodeDllImportAttribute(ref DecodeWellKnownAttributeArguments<Attr
Debug.Assert(!attribute.HasErrors);
bool hasErrors = false;

if (!this.IsExtern || !this.IsStatic)
var implementationPart = this.PartialImplementationPart ?? this;
if (!implementationPart.IsExtern || !implementationPart.IsStatic)
{
arguments.Diagnostics.Add(ErrorCode.ERR_DllImportOnInvalidMethod, arguments.AttributeSyntaxOpt.Name.Location);
hasErrors = true;
Expand Down
97 changes: 70 additions & 27 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ private SourceOrdinaryMethodSymbol(
_hasAnyBody = hasBody;

bool modifierErrors;
var declarationModifiers = this.MakeModifiers(modifiers, methodKind, hasBody, location, diagnostics, out modifierErrors);
DeclarationModifiers declarationModifiers;
(declarationModifiers, HasExplicitAccessModifier, modifierErrors) = this.MakeModifiers(modifiers, methodKind, hasBody, location, diagnostics);

var isMetadataVirtualIgnoringModifiers = (object)explicitInterfaceType != null; //explicit impls must be marked metadata virtual

Expand Down Expand Up @@ -289,16 +290,6 @@ private void MethodChecks(MethodDeclarationSyntax syntax, Binder withTypeParamsB

if (IsPartial)
{
// check that there are no out parameters in a partial
foreach (var p in this.Parameters)
{
if (p.RefKind == RefKind.Out)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodCannotHaveOutParameters, location);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
break;
}
}

if (MethodKind == MethodKind.ExplicitInterfaceImplementation)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodNotExplicit, location);
Expand Down Expand Up @@ -611,7 +602,7 @@ internal bool IsPartialDefinition
{
get
{
return this.IsPartial && !_hasAnyBody;
return this.IsPartial && !_hasAnyBody && !HasExternModifier;
}
}

Expand All @@ -622,7 +613,7 @@ internal bool IsPartialImplementation
{
get
{
return this.IsPartial && _hasAnyBody;
return this.IsPartial && (_hasAnyBody || HasExternModifier);
}
}

Expand Down Expand Up @@ -677,6 +668,16 @@ public override MethodSymbol PartialImplementationPart
}
}

public sealed override bool IsExtern
{
get
{
return IsPartialDefinition
? _otherPartOfPartial?.IsExtern ?? false
: HasExternModifier;
}
}

public override string GetDocumentationCommentXml(CultureInfo preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default(CancellationToken))
{
ref var lazyDocComment = ref expandIncludes ? ref this.lazyExpandedDocComment : ref this.lazyDocComment;
Expand Down Expand Up @@ -756,7 +757,9 @@ internal override bool IsExpressionBodied
get { return _isExpressionBodied; }
}

private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, MethodKind methodKind, bool hasBody, Location location, DiagnosticBag diagnostics, out bool modifierErrors)
internal bool HasExplicitAccessModifier { get; }

private (DeclarationModifiers mods, bool hasExplicitAccessMod, bool modifierErrors) MakeModifiers(SyntaxTokenList modifiers, MethodKind methodKind, bool hasBody, Location location, DiagnosticBag diagnostics)
{
bool isInterface = this.ContainingType.IsInterface;
bool isExplicitInterfaceImplementation = methodKind == MethodKind.ExplicitInterfaceImplementation;
Expand Down Expand Up @@ -808,7 +811,19 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, MethodKind
allowedModifiers |= DeclarationModifiers.ReadOnly;
}

var mods = ModifierUtils.MakeAndCheckNontypeMemberModifiers(modifiers, defaultAccess, allowedModifiers, location, diagnostics, out modifierErrors);
// In order to detect whether explicit accessibility mods were provided, we pass the default value
// for 'defaultAccess' and manually add in the 'defaultAccess' flags after the call.
bool hasExplicitAccessMod;
var mods = ModifierUtils.MakeAndCheckNontypeMemberModifiers(modifiers, defaultAccess: DeclarationModifiers.None, allowedModifiers, location, diagnostics, out bool modifierErrors);
if ((mods & DeclarationModifiers.AccessibilityMask) == 0)
{
hasExplicitAccessMod = false;
mods |= defaultAccess;
}
else
{
hasExplicitAccessMod = true;
}

this.CheckUnsafeModifier(mods, diagnostics);

Expand All @@ -817,7 +832,7 @@ private DeclarationModifiers MakeModifiers(SyntaxTokenList modifiers, MethodKind
location, diagnostics);

mods = AddImpliedModifiers(mods, isInterface, methodKind, hasBody);
return mods;
return (mods, hasExplicitAccessMod, modifierErrors);
}

private static DeclarationModifiers AddImpliedModifiers(DeclarationModifiers mods, bool containingTypeIsInterface, MethodKind methodKind, bool hasBody)
Expand Down Expand Up @@ -905,26 +920,40 @@ private ImmutableArray<TypeParameterSymbol> MakeTypeParameters(MethodDeclaration
return result.ToImmutableAndFree();
}

private const DeclarationModifiers PartialMethodExtendedModifierMask =
DeclarationModifiers.Virtual |
DeclarationModifiers.Override |
DeclarationModifiers.New |
DeclarationModifiers.Sealed |
DeclarationModifiers.Extern;
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved

internal bool HasExtendedPartialModifier => (DeclarationModifiers & PartialMethodExtendedModifierMask) != 0;

private void CheckModifiers(bool isExplicitInterfaceImplementation, bool hasBody, Location location, DiagnosticBag diagnostics)
{
const DeclarationModifiers partialMethodInvalidModifierMask = (DeclarationModifiers.AccessibilityMask & ~DeclarationModifiers.Private) |
DeclarationModifiers.Virtual |
DeclarationModifiers.Abstract |
DeclarationModifiers.Override |
DeclarationModifiers.New |
DeclarationModifiers.Sealed |
DeclarationModifiers.Extern;

bool isExplicitInterfaceImplementationInInterface = isExplicitInterfaceImplementation && ContainingType.IsInterface;

if (IsPartial && !ReturnsVoid)
if (IsPartial && HasExplicitAccessModifier)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodMustReturnVoid, location);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
Binder.CheckFeatureAvailability(SyntaxNode, MessageID.IDS_FeatureExtendedPartialMethods, diagnostics, location);
}
else if (IsPartial && (DeclarationModifiers & partialMethodInvalidModifierMask) != 0)

if (IsPartial && IsAbstract)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodInvalidModifier, location);
}
else if (IsPartial && !HasExplicitAccessModifier && !ReturnsVoid)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodWithNonVoidReturnMustHaveAccessMods, location, this);
}
else if (IsPartial && !HasExplicitAccessModifier && HasExtendedPartialModifier)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodWithExtendedModMustHaveAccessMods, location, this);
}
else if (IsPartial && !HasExplicitAccessModifier && Parameters.Any(p => p.RefKind == RefKind.Out))
{
diagnostics.Add(ErrorCode.ERR_PartialMethodWithOutParamMustHaveAccessMods, location, this);
}
else if (this.DeclaredAccessibility == Accessibility.Private && (IsVirtual || (IsAbstract && !isExplicitInterfaceImplementationInInterface) || IsOverride))
{
diagnostics.Add(ErrorCode.ERR_VirtualPrivate, location, this);
Expand Down Expand Up @@ -1144,6 +1173,20 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
diagnostics.Add(ErrorCode.ERR_PartialMethodParamsDifference, implementation.Locations[0]);
}

if (definition.HasExplicitAccessModifier != implementation.HasExplicitAccessModifier
|| definition.DeclaredAccessibility != implementation.DeclaredAccessibility)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodAccessibilityDifference, implementation.Locations[0]);
}

if (definition.IsVirtual != implementation.IsVirtual
|| definition.IsOverride != implementation.IsOverride
|| definition.IsSealed != implementation.IsSealed
|| definition.IsNew != implementation.IsNew)
{
diagnostics.Add(ErrorCode.ERR_PartialMethodExtendedModDifference, implementation.Locations[0]);
}

PartialMethodConstraintsChecks(definition, implementation, diagnostics);

SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride(
Expand Down
Loading