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

Consider RefSafetyRules with UnscopedRef #75930

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jjonescz
Copy link
Member

Fixes #75828

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 15, 2024
@jjonescz jjonescz marked this pull request as ready for review November 15, 2024 11:23
@jjonescz jjonescz requested a review from a team as a code owner November 15, 2024 11:23
@AlekseyTs
Copy link
Contributor

The issue that this PR is claiming to fix sounds as though the problem is related to using a specific language version, but the code changes and the tests look like the language version makes no difference on its own. If that is the case, consider clarifying the issue, clearly describing all the factors involved. And, perhaps, adjusting its title.

@jjonescz
Copy link
Member Author

If that is the case, consider clarifying the issue, clearly describing all the factors involved. And, perhaps, adjusting its title.

I've updated the issue's title and description.

""";
CreateCompilation([source, UnscopedRefAttributeDefinition],
parseOptions: TestOptions.Regular10).VerifyDiagnostics(
// (6,47): error CS8170: Struct members cannot return 'this' or other instance members by reference
Copy link
Member

@cston cston Nov 21, 2024

Choose a reason for hiding this comment

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

error CS8170: Struct members cannot return 'this' or other instance members by reference

This looks like a breaking change, although it also looks like users of this library would ignore the [UnscopedRef] attribute on the method since the assembly is not marked with [RefSafetyRules], so perhaps the breaking change when compiling this library is appropriate.

Consider adding a note to the Breaking Change document.

Is this the only breaking change in the PR? (From the tests below, it looks like in other cases we're reporting other errors where [UnscopedRef] is used (and ignored) with earlier ref safety rules.)

Copy link
Member

Choose a reason for hiding this comment

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

Should the compiler report a diagnostic when [UnscopedRef] is applied to a source method that is compiled with earlier ref safety rules? Otherwise, the library author may not realize the annotation will be ignored by callers. For instance, would the following still compile with earlier ref safety rules without warnings or errors?

struct S
{
    int F;
    [UnscopedRef] public ref int Ref() => ref Unsafe.AsRef(in F);
}
static class C
{
    static ref int M()
    {
        S s = default;
        return ref s.Ref();
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like an oversight that we did not report an error when UnscopedRef was used in presence of the old ref safety rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

For instance, would the following still compile with earlier ref safety rules without warnings or errors?

No, it wouldn't compile. There will be errors because you are still using this by reference inside S.Ref which is disallowed there.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I've changed the implementation to report an error for [UnscopedRef] on a member in the old rules similarly as we report it for [UnscopedRef] on a parameter.

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

UnscopedRef should not affect code under the old ref safety rules
4 participants