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

Partial properties: merging declarations and emit of simple cases #72999

Merged
merged 6 commits into from
May 1, 2024
Merged

Partial properties: merging declarations and emit of simple cases #72999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f23ba9a
Partial properties: merging declarations and emit of simple cases
RikkiGibson Apr 18, 2024
36cbfb0
Address feedback
RikkiGibson Apr 24, 2024
038e6d3
Address feedback
RikkiGibson Apr 26, 2024
bc5f89f
adjust InitializePartialMethod/PropertyParts asserts to deal with a p…
RikkiGibson Apr 29, 2024
1f1a7a0
Address impl feedback
RikkiGibson Apr 30, 2024
95cf0a3
Address more feedback
RikkiGibson Apr 30, 2024
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
21 changes: 15 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,8 @@ public override Binder VisitAccessorDeclaration(AccessorDeclarationSyntax parent
if ((object)propertySymbol != null)
{
accessor = (parent.Kind() == SyntaxKind.GetAccessorDeclaration) ? propertySymbol.GetMethod : propertySymbol.SetMethod;
// PROTOTYPE(partial-properties): check if a property with no accessors could fail this assertion, in which case we should either adjust the assertion or remove it.
Debug.Assert(accessor is not null || parent.HasErrors);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}
break;
}
Expand Down Expand Up @@ -585,18 +587,25 @@ bool checkSymbol(Symbol sym, TextSpan memberSpan, SymbolKind kind, out Symbol re
return false;
}

if (sym.Kind == SymbolKind.Method)
if (kind is SymbolKind.Method or SymbolKind.Property)
{
if (InSpan(sym.GetFirstLocation(), this.syntaxTree, memberSpan))
{
return true;
}

// If this is a partial method, the method represents the defining part,
// not the implementation (method.Locations includes both parts). If the
// span is in fact in the implementation, return that method instead.
var implementation = ((MethodSymbol)sym).PartialImplementationPart;
if ((object)implementation != null)
// If this is a partial member, the member represents the defining part,
// not the implementation (member.Locations includes both parts). If the
// span is in fact in the implementation, return that member instead.
if (sym switch
#pragma warning disable format
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we link to a tracking issue? Is the problem what's described in #46464?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, take the switch out of the if?

    Symbol implementation = sym switch
    {
        MethodSymbol method => method.PartialImplementationPart,
        SourcePropertySymbol property => property.PartialImplementationPart,
        _ => throw ExceptionUtilities.UnexpectedValue(sym)
    };

    if (implementation is { } && InSpan(implementation.GetFirstLocation(), this.syntaxTree, memberSpan))
    {
        result = implementation;
        return true;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, I feel like it is more likely to have the same cause as #73251

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be willing to do either including a link or extracting the switch in a follow-up PR. I almost feel a little defiant toward keeping the current factoring since I want the formatter to be fixed :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd like it fixed too :-) But all the related issues are currently marked as Backlog :-/
It would help to add links and screenshots to get a fix prioritized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
MethodSymbol method => (Symbol)method.PartialImplementationPart,
jcouv marked this conversation as resolved.
Show resolved Hide resolved
SourcePropertySymbol property => property.PartialImplementationPart,
_ => throw ExceptionUtilities.UnexpectedValue(sym)
}
#pragma warning restore format
is { } implementation)
{
if (InSpan(implementation.GetFirstLocation(), this.syntaxTree, memberSpan))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
Expand Down
20 changes: 19 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,7 +1171,7 @@
<value>Cannot implicitly convert type '{0}' to '{1}'. An explicit conversion exists (are you missing a cast?)</value>
</data>
<data name="ERR_PartialMisplaced" xml:space="preserve">
<value>The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method return type.</value>
<value>The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method or property return type.</value>
</data>
<data name="ERR_ImportedCircularBase" xml:space="preserve">
<value>Imported type '{0}' is invalid. It contains a circular base type dependency.</value>
Expand Down Expand Up @@ -7929,4 +7929,22 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_InterceptsLocationDataInvalidPosition" xml:space="preserve">
<value>The data argument to InterceptsLocationAttribute refers to an invalid position in file '{0}'.</value>
</data>
<data name="ERR_PartialPropertyMissingImplementation" xml:space="preserve">
<value>Partial property '{0}' must have an implementation part.</value>
</data>
<data name="ERR_PartialPropertyMissingDefinition" xml:space="preserve">
<value>Partial property '{0}' must have an definition part.</value>
</data>
<data name="ERR_PartialPropertyDuplicateDefinition" xml:space="preserve">
<value>A partial property may not have multiple defining declarations, and cannot be an auto-property.</value>
</data>
<data name="ERR_PartialPropertyDuplicateImplementation" xml:space="preserve">
<value>A partial property may not have multiple implementing declarations</value>
333fred marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="ERR_PartialPropertyMissingAccessor" xml:space="preserve">
<value>Property accessor '{0}' must be implemented because it is declared on the definition part</value>
</data>
<data name="ERR_PartialPropertyUnexpectedAccessor" xml:space="preserve">
<value>Property accessor '{0}' does not implement any accessor declared on the definition part</value>
</data>
</root>
8 changes: 8 additions & 0 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 @@ -2310,6 +2310,14 @@ internal enum ErrorCode
ERR_InterceptsLocationDataInvalidPosition = 9235,
INF_TooManyBoundLambdas = 9236,

// PROTOTYPE(partial-properties): pack
ERR_PartialPropertyMissingImplementation = 9300,
ERR_PartialPropertyMissingDefinition = 9301,
ERR_PartialPropertyDuplicateDefinition = 9302,
ERR_PartialPropertyDuplicateImplementation = 9303,
ERR_PartialPropertyMissingAccessor = 9304,
ERR_PartialPropertyUnexpectedAccessor = 9305,

#endregion

// Note: you will need to do the following after adding errors:
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2438,6 +2438,13 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.ERR_InterceptsLocationFileNotFound:
case ErrorCode.ERR_InterceptsLocationDataInvalidPosition:
case ErrorCode.INF_TooManyBoundLambdas:

case ErrorCode.ERR_PartialPropertyMissingImplementation:
case ErrorCode.ERR_PartialPropertyMissingDefinition:
case ErrorCode.ERR_PartialPropertyDuplicateDefinition:
case ErrorCode.ERR_PartialPropertyDuplicateImplementation:
case ErrorCode.ERR_PartialPropertyMissingAccessor:
case ErrorCode.ERR_PartialPropertyUnexpectedAccessor:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
22 changes: 14 additions & 8 deletions src/Compilers/CSharp/Portable/Symbols/MemberSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ internal static bool ShouldEmit(this MethodSymbol method)
}

// Don't emit partial methods without an implementation part.
if (method.IsPartialMethod() && method.PartialImplementationPart is null)
if (method.IsPartialMember() && method.PartialImplementationPart is null)
{
return false;
}
Expand Down Expand Up @@ -545,22 +545,28 @@ internal static bool IsExplicitInterfaceImplementation(this Symbol member)
}
}

internal static bool IsPartialMethod(this Symbol member)
internal static bool IsPartialMember(this Symbol member)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider introducing some new members on a common base type, or introducing an IPartialMemberSymbol interface or something similar. But ultimately I think extension methods with type tests is an appropriate level of abstraction for this feature.

I am meaning to add a debug assertion that these methods are only used with original definition symbols. I'll try and add a check for that in future PR.

RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
var sms = member as SourceMemberMethodSymbol;
return sms?.IsPartial == true;
return member
is SourceOrdinaryMethodSymbol { IsPartial: true }
or SourcePropertySymbol { IsPartial: true }
or SourcePropertyAccessorSymbol { IsPartial: true };
}

internal static bool IsPartialImplementation(this Symbol member)
{
var sms = member as SourceOrdinaryMethodSymbol;
return sms?.IsPartialImplementation == true;
return member
is SourceOrdinaryMethodSymbol { IsPartialImplementation: true }
or SourcePropertySymbol { IsPartialImplementation: true }
or SourcePropertyAccessorSymbol { IsPartialImplementation: true };
}

internal static bool IsPartialDefinition(this Symbol member)
{
var sms = member as SourceOrdinaryMethodSymbol;
return sms?.IsPartialDefinition == true;
return member
is SourceOrdinaryMethodSymbol { IsPartialDefinition: true }
or SourcePropertySymbol { IsPartialDefinition: true }
or SourcePropertyAccessorSymbol { IsPartialDefinition: true };
}

internal static bool ContainsTupleNames(this Symbol member)
Expand Down
Loading