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

Avoid assert in emit layer when intercepted call implicitly captures receiver to temp #74509

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

RikkiGibson
Copy link
Contributor

Closes #71657

Extracted from #74363. This part of the change I feel confident is one we should be making. The other part I want to revisit and PR separately.

This PR is preserving an already-shipped behavior without changing it. It is just changing the exact way we get that behavior by adding the temp in lowering, when we know we are in the specific situation we want to permit, rather than in the more general code path in emit, which was tripping a debug assertion.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 23, 2024
@RikkiGibson RikkiGibson marked this pull request as ready for review July 23, 2024 22:05
@RikkiGibson RikkiGibson requested a review from a team as a code owner July 23, 2024 22:05
public static class C
{
{{locationSpecifier.GetInterceptsLocationAttributeSyntax()}}
public static void M1(this ref S s) => Console.WriteLine(1);
Copy link
Member

Choose a reason for hiding this comment

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

Consider using s in a way that mutates, to show that we are actually capturing by ref and that the results are observable. The same for the rest of the test methods.

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 wondering how exactly should I demonstrate this? Do you want me to try and read the managed reference at the call site (e.g. by returning 'ref S s' by-reference)?

Copy link
Member

@333fred 333fred Jul 24, 2024

Choose a reason for hiding this comment

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

Why not just assign a field in the method, then read the field from the local in the calling function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

read the field from the local

The lowering temp which is being passed by-ref to the call can't be directly used in user code. We would have to return a ref to that temp if we wanted the call site to be able to read it.

Copy link
Member

Choose a reason for hiding this comment

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

Am I misunderstanding the original bug? I expect something like this would be accessible, and would have triggered the emit assert:

var s = new S();
s.M(); // Intercepted, and `s` was mutated

Copy link
Contributor Author

@RikkiGibson RikkiGibson Jul 24, 2024

Choose a reason for hiding this comment

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

That case would not trigger the assert. The assert occurs when the compiler needs to create a temp for the receiver to use as an argument for a by-ref this parameter, such as new S().M(). It won't occur when the receiver argument is already something we can pass by-reference for the this parameter.

The emit layer is fine with creating a temp for an instance call receiver in the new S().M() case. However, when we intercept the call with a method like static Extension(this ref S s), the emit layer basically sees the call as Extension(new S()), doesn't expect to create a temp for new S(), and will assert.

Copy link
Member

Choose a reason for hiding this comment

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

Could we observe this with a ref-returning method? IE, something like this?

var s = new S();
Identity(ref s).M();
s.ObserveMutated();

ref S Identity(ref S s) => ref s;

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 think in this case the compiler also doesn't need to capture the receiver 'Identity(ref s)' to temp. But we can go ahead and add the test to show that we use the reference properly here.

Copy link
Member

@jaredpar jaredpar Jul 25, 2024

Choose a reason for hiding this comment

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

Think we could observe it this way

using System;
using System.Diagnostics.CodeAnalysis;

Span<int> Test() {
  // should error in both intercepted and non intercepted case
  return new S().M();
}

ref struct S {
    [UnscopedRef]
    public Span<int> M() => default;
}

static class D
{
    [InterceptsLocation(1, "...")]
    public static Span<int> M(this ref S s)
}

@RikkiGibson RikkiGibson merged commit 90da6fa into dotnet:main Aug 5, 2024
24 checks passed
@RikkiGibson RikkiGibson deleted the interceptors-capture-receiver branch August 5, 2024 15:41
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 5, 2024
@dibarbet dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024
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
4 participants