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

Support consumption of instance increment operators #77098

Open
wants to merge 2 commits into
base: features/UserDefinedCompoundAssignment
Choose a base branch
from

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested review from 333fred and cston February 7, 2025 01:08
@AlekseyTs AlekseyTs requested a review from a team as a code owner February 7, 2025 01:08
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 7, 2025
@AlekseyTs
Copy link
Contributor Author

@333fred, @cston, @dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

@cston, @dotnet/roslyn-compiler For the second review

@@ -1399,6 +1399,10 @@ internal SingleLookupResult CheckViability(Symbol symbol, int arity, LookupOptio
{
return LookupResult.Empty();
}
else if ((options & LookupOptions.MustBeOperator) != 0 && unwrappedSymbol is not MethodSymbol { MethodKind: MethodKind.UserDefinedOperator })
{
return LookupResult.Empty();
Copy link
Member

Choose a reason for hiding this comment

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

return LookupResult.Empty();

Are we testing this code path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we testing this code path?

Not yet

bool resultIsUsed = ResultIsUsed(node);

if ((kind is (UnaryOperatorKind.PostfixIncrement or UnaryOperatorKind.PostfixDecrement) && resultIsUsed) ||
!CheckValueKind(node, operand, BindValueKind.RefersToLocation | BindValueKind.Assignable, checkingReceiver: false, BindingDiagnosticBag.Discarded))
Copy link
Member

Choose a reason for hiding this comment

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

| BindValueKind.Assignable

Are we testing BindValueKind.Assignable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we testing BindValueKind.Assignable?

Not specifically. A an increment/decrement target must be assignable and this is checked on line 2272. Therefore, I do not find it critical to cover that specific dimension in error scenarios.

OverloadResolution.Options.None);

typeArguments.Free();
diagnostics.Add(node, useSiteInfo);
Copy link
Member

Choose a reason for hiding this comment

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

diagnostics.Add(node, useSiteInfo);

Are we testing use-site diagnostics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we testing use-site diagnostics?

No

public void Increment_077_Consumption_Postfix_Used([CombinatorialValues("++", "--")] string op, bool fromMetadata)
{
var source1 = @"
public class C1
Copy link
Member

@cston cston Feb 10, 2025

Choose a reason for hiding this comment

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

class

Are we testing with struct, or with type parameter with/without class or struct constraints? #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.

Are we testing with struct, or with type parameter with/without class or struct constraints?

I do not find these scenarios interesting. The test is not about code that we generate, it is about an error that doesn't depend on the mentioned aspects.

public void Increment_078_Consumption_Postfix_Used([CombinatorialValues("++", "--")] string op, bool fromMetadata)
{
var source1 = @"
public class C1
Copy link
Member

@cston cston Feb 10, 2025

Choose a reason for hiding this comment

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

class

Are we testing with struct, or with type parameter with/without class or struct constraints? #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.

Are we testing with struct, or with type parameter with/without class or struct constraints?

I do not find these scenarios interesting. The test is verifying fall back to the legacy operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature-UserDefinedCompoundAssignmentOperators 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