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 UnscopedRef attribute on interface methods and properties #72171

Merged

Conversation

@AlekseyTs AlekseyTs requested a review from a team as a code owner February 19, 2024 20:16
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 19, 2024
}

// Receiver of an interface method could possibly be a structure.
// Let's treat it as scoped ref by ref parameter for the purpose of ref safety analysis.
Copy link
Member

@jjonescz jjonescz Feb 20, 2024

Choose a reason for hiding this comment

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

Suggested change
// Let's treat it as scoped ref by ref parameter for the purpose of ref safety analysis.
// Let's treat it as scoped ref parameter for the purpose of ref safety analysis.
``` #WontFix

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 isn't actually a typo. the value that we return is ScopedRef and the parameter is ref parameter

@@ -2729,6 +2735,84 @@ private static ErrorCode GetStandardCallEscapeError(bool checkingReceiver)
{
return checkingReceiver ? ErrorCode.ERR_EscapeCall2 : ErrorCode.ERR_EscapeCall;
}

#nullable enable
Copy link
Member

@jjonescz jjonescz Feb 20, 2024

Choose a reason for hiding this comment

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

Consider nullable-enabling the trivial function above as well to make the #nullable enable range more contiguous. #Resolved

{
get { return -1; }
}

public override ImmutableArray<CustomModifier> RefCustomModifiers
public sealed override ImmutableArray<CustomModifier> RefCustomModifiers
{
get { return ImmutableArray<CustomModifier>.Empty; }
}

// TODO: structs
Copy link
Member

@jjonescz jjonescz Feb 20, 2024

Choose a reason for hiding this comment

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

I know this comment has been copied, but I don't understand what it means. Maybe it can be removed? #Resolved

if (attributeData is SourceAttributeData { ApplicationSyntaxReference: { } applicationSyntaxReference } &&
attributeData.IsTargetAttribute(AttributeDescription.UnscopedRefAttribute))
{
MessageID.IDS_RefStructInterfaces.CheckFeatureAvailability(diagnostics, implementingSymbol.DeclaringCompilation, applicationSyntaxReference.GetLocation());
Copy link
Member

@jjonescz jjonescz Feb 20, 2024

Choose a reason for hiding this comment

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

Consider asserting we always get here (i.e., find the UnscopedRefAttribute). #Resolved

}

[Fact]
public void UnscopedRefInInterface_Property_08()
Copy link
Member

@jjonescz jjonescz Feb 20, 2024

Choose a reason for hiding this comment

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

Consider testing also indexers. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Feb 20, 2024

Choose a reason for hiding this comment

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

Consider testing also indexers.

While I can certainly clone property tests with indexers, I do not think the tests will have any immediate value, because, from the symbol model point of view indexers are properties, they do not have separate representation or handling.

interface C : I
{
[UnscopedRef]
ref int I.M()
Copy link
Member

@jjonescz jjonescz Feb 20, 2024

Choose a reason for hiding this comment

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

Do we have a similar test (interface derived from another interface) but where the method is not explictly implemented? #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.

Do we have a similar test (interface derived from another interface) but where the method is not explictly implemented?

I am not sure what scenario you have in mind. Could you please elaborate? Implicit implementation is not a thing in interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind then, I haven't realized that.

@AlekseyTs AlekseyTs requested review from cston and a team February 21, 2024 17:39
@AlekseyTs
Copy link
Contributor Author

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

@cston
Copy link
Member

cston commented Feb 22, 2024

    public void DefensiveCopy_02()

Are there any changes here related to interfaces? #Resolved


Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/RefStructInterfacesTests.cs:3382 in 004452e. [](commit_id = 004452e, deletion_comment = False)

@cston
Copy link
Member

cston commented Feb 22, 2024

    public void UnscopedRef_ArgumentsMustMatch_0_ConstrainedTypeParameter(bool addStructConstraint)

Should this be ArgumentsMustMatch_01 here and in several other cases below? #Resolved


Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/RefStructInterfacesTests.cs:2765 in 004452e. [](commit_id = 004452e, deletion_comment = False)

@cston
Copy link
Member

cston commented Feb 22, 2024

    public void DefensiveCopy_05_ClassdConstrainedTypeParameter()

Class? #Resolved


Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/RefStructInterfacesTests.cs:3538 in 004452e. [](commit_id = 004452e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor Author

    public void DefensiveCopy_02()

Are there any changes here related to interfaces?

It looks like I missed adjusting this test. Thanks.


In reply to: 1959806502


Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/RefStructInterfacesTests.cs:3382 in 004452e. [](commit_id = 004452e, deletion_comment = False)

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