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

Partial properties: basic indexer scenarios and signature mismatch diagnostics #73378

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented May 7, 2024

Test plan: #73090
Relevant spec change: dotnet/csharplang#8106

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels May 7, 2024
@RikkiGibson RikkiGibson force-pushed the partial-properties-4 branch 2 times, most recently from fcb68d3 to 21a42fa Compare May 8, 2024 22:42
@RikkiGibson RikkiGibson changed the title Partial properties: indexers Partial properties: basic indexer scenarios and signature mismatch diagnostics May 8, 2024
@RikkiGibson RikkiGibson force-pushed the partial-properties-4 branch 2 times, most recently from 86d245c to 2737de5 Compare May 8, 2024 22:47
@RikkiGibson RikkiGibson force-pushed the partial-properties-4 branch from 544d124 to bb86b40 Compare May 8, 2024 23:11
@RikkiGibson RikkiGibson marked this pull request as ready for review May 8, 2024 23:13
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 8, 2024 23:13
@RikkiGibson RikkiGibson requested review from 333fred and jcouv May 8, 2024 23:13
@@ -10,6 +10,11 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal static class PropertySymbolExtensions
{
public static bool IsParams(this PropertySymbol property)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is equivalent to the IsParams extension for methods

@@ -145,6 +146,12 @@ internal static SourcePropertySymbol Create(SourceMemberContainerTypeSymbol cont
MessageID.IDS_FeatureAutoPropertyInitializer.CheckFeatureAvailability(diagnostics, initializer.EqualsToken);
}

internal override void ForceComplete(SourceLocation? locationOpt, Predicate<Symbol>? filter, CancellationToken cancellationToken)
{
PartialImplementationPart?.ForceComplete(locationOpt, filter, cancellationToken);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is equivalent to the ForceComplete declared on ordinary method symbols

@RikkiGibson
Copy link
Contributor Author

@jcouv @333fred for review

}

[Fact]
public void DynamicDifference_IndexerParameter()
Copy link
Member

@jcouv jcouv May 9, 2024

Choose a reason for hiding this comment

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

Some of the added tests exercise the logic added a couple of PRs ago (in MergePartialMembers) but for indexer scenarios. It would be good to go back and cover that logic with indexer scenarios.
For example, scenarios producing diagnostics (ERR_PartialMethodMustHaveLatent, ERR_PartialMethodWithAccessibilityModsMustHaveImplementation, ERR_PartialPropertyMissingImplementation, ...).

There may be a couple more interesting scenarios excercising MemberSignatureComparer.PartialMethodsComparer. For example, we'll merge the parts if the return types differ. And we ignore tuple names.
Consider testing some nested differences too.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1) with some test suggestions to consider

@jcouv jcouv self-assigned this May 9, 2024
}

[Fact]
public void RefKindDifference_IndexerParameter_04()
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we also mixing in vs out/ref and such?

public partial int this[int x, int y = 1] { get => y; set { } }

public partial int this[int x, int y, int z = 1] { get; set; }
public partial int this[int x, int y, int z = 1] { get => z; set { } }
Copy link
Member

Choose a reason for hiding this comment

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

Let's show consumption for these scenarios.

// (10,44): warning CS1066: The default value specified for parameter 'y' will have no effect because it applies to a member that is used in contexts that do not allow optional arguments
// public partial int this[int x = 1, int y = 2] { get => y; set { } }
Diagnostic(ErrorCode.WRN_DefaultValueForUnconsumedLocation, "y").WithArguments("y").WithLocation(10, 44));
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's also test differing default values: one part has 1, the other has 2, for example. And verify behavior runtime for this scenario if it compiles.

@RikkiGibson RikkiGibson requested a review from 333fred May 9, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure Feature - Partial Properties untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants