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

VS 17.3 Preview 3 Regression from Preview 2 with ref-returning an out parameter #62900

Closed
fitdev opened this issue Jul 25, 2022 · 12 comments
Closed
Assignees
Labels
Area-Compilers Bug Feature - Ref Fields Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@fitdev
Copy link

fitdev commented Jul 25, 2022

Version Used:
VS 2022 17.3 Preview 3

VS 2022 17.3 Preview 3 Regression Error:

public static ref TTo AsRef<TFrom, TTo>(this ref TFrom it, out TTo v) where TFrom : struct { v = Unsafe.As<TFrom, TTo>(ref it); return ref v; } // <-- CS8166: Cannot return a parameter by reference 'v' because it is not a ref parameter

VS 2022 17.3 Preview 2 or earlier:

public static ref TTo AsRef<TFrom, TTo>(this ref TFrom it, out TTo v) where TFrom : struct { v = Unsafe.As<TFrom, TTo>(ref it); return ref v; } // <-- No error, everything compiles

Expected Behavior: Should work the same as in VS 17.3 Preview 2 or earlier: an out parameter should be ref-returnable.

Actual Behavior: Since VS 17.3 Preview 3: cannot return a parameter by reference 'v' because it is not a ref parameter

@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 25, 2022
@Sergio0694
Copy link
Contributor

This should also be by design as per the latest draft of the lowlevel struct improvements proposal (see https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md). out parameters are implicitly scoped and cannot be returned by ref. You should be able to work around this by annotating it with [UnscopedRef] though

@fitdev
Copy link
Author

fitdev commented Jul 25, 2022

@Sergio0694
Do you mean to add [UnscopedRef] to the parameter, or the return value? Also, is there another workaround until [UnscopedRef] becomes available later this year? I mean it did work in previous VS Preview without any compile error, and worked well at runtime as well.

@cston
Copy link
Member

cston commented Jul 25, 2022

Also, is there another workaround until [UnscopedRef] becomes available

It should be possible to use Unsafe.AsRef() in the meantime: return ref Unsafe.AsRef(in v);.

@fitdev
Copy link
Author

fitdev commented Jul 25, 2022

@cston Thank you for your suggestion, but now I get a different compile error: CS8347: Cannot use a result of 'Unsafe.AsRef<TTo>(in TTo)' in this context because it may expose variables referenced by parameter 'source' outside of their declaration scope

@Sergio0694
Copy link
Contributor

I think for the same reason MemoryMarshal.CreateSpan wasn't annotated, AsRef may also not have scoped for you.
As a temporary workaround to get you unclocked, can you not do:

public static ref TTo AsRef<TFrom, TTo>(this ref TFrom it, out TTo v)
    where TFrom : struct
{
    ref TTo to = ref Unsafe.As<TFrom, TTo>(ref it);

    v = to;

    return ref to;
}

@fitdev
Copy link
Author

fitdev commented Jul 26, 2022

@Sergio0694 Thank you for your suggested workaround. Yes, I thought about it, but was concerned about the performance hit of assigning to an intermediate variable. I guess it is still acceptable as a workaround. Though I still don't get how come it worked with the previous VS preview and no longer works with latest VS preview, thus seemingly breaking backwards-compat, as I did not change the DotNet SDK version at all - only updated VS.

@cston
Copy link
Member

cston commented Jul 26, 2022

Though I still don't get how come it worked with the previous VS preview and no longer works with latest VS preview

17.3 preview 3 added support for declaring ref fields in ref struct types, when compiling with -langversion:preview. Supporting ref fields required a couple of breaking changes to method invocation escape analysis: see ref arguments and ref struct return and returning out parameters.

The previous behavior can be enabled by compiling with -langversion:10 or earlier when targeting .NET 6 or earlier (for .NET 7, the updated escape analysis will be required). [updated]

@fitdev
Copy link
Author

fitdev commented Jul 26, 2022

@cston Just to clarify then: once [UnscopedRef] is added to BCL in the next DotNet preview, will attributing an out parameter with this attribute resolve the issue and allow it to be ref-returned? Since the "workaround" mentioned in the docs:

A possible workaround is to change the method signature to pass the parameter by ref instead.

... is not really a good option since signature change is required and all the call sites need to be changed in an unwanted way, i.e. inability to declare and out var at the call site.

@Sergio0694
Copy link
Contributor

"Yes, I thought about it, but was concerned about the performance hit of assigning to an intermediate variable"

There is no performance hit. There's no variable either. All of that will be enregistered anyway (see here).

@cston
Copy link
Member

cston commented Jul 26, 2022

Just to clarify then: once [UnscopedRef] is added to BCL in the next DotNet preview, will attributing an out parameter with this attribute resolve the issue and allow it to be ref-returned?

Yes, adding [UnscopedRef] to the out parameter will allow the parameter to be returned by reference.

I have updated the breaking change documentation, and added that as a better work around, as part of the PR to support [UnscopedRef], thanks.

@jcouv jcouv added Bug Resolution-By Design The behavior reported in the issue matches the current design and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 28, 2022
@jcouv jcouv added this to the 17.4 milestone Jul 28, 2022
@jcouv
Copy link
Member

jcouv commented Jul 28, 2022

@cston Do you want to keep this issue open to capture this as a test case for [UnscopedRef], or should we close it now?

@cston
Copy link
Member

cston commented Jul 28, 2022

#62783 includes tests for [UnscopedRef] out parameters, and an update to the breaking changes documentation to mention [UnscopedRef].

@fitdev, thanks for reporting this issue.

@cston cston closed this as completed Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Feature - Ref Fields Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

4 participants