Ref-safety for with(...) elements.
#81542
Conversation
…-arguments' into creationArguments
…y.cs Co-authored-by: Rikki Gibson <[email protected]>
Co-authored-by: Rikki Gibson <[email protected]>
…/roslyn into creationArguments
Co-authored-by: Fred Silberberg <[email protected]>
…y.cs Co-authored-by: Fred Silberberg <[email protected]>
dotnet-policy-service
bot
added
the
Needs API Review
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
…ArgumentsRefSafety
| // as it could be capturing local data, we conservatively restrict it to the local scope. | ||
| var safeContext = node.Elements.Length == 0 || LocalRewriter.ShouldUseRuntimeHelpersCreateSpan(node, elementType.Type) | ||
| ? SafeContext.CallingMethod | ||
| : _localScopeDepth; |
There was a problem hiding this comment.
intentionally changed this. SafeContext.ReturnOnly feels wrong to me based on my undersrtanding of ref-safety. If we have no-elements, or we know we are creating a span out of a program data segment, then it should be safe to pass this value to the calling-method, since there's no way it could actually be capturing a local ref to anything.
There was a problem hiding this comment.
there are several tests taht validate this.
CyrusNajmabadi
changed the title
with(...) ref-safety.with(...) elements.
|
@RikkiGibson @333fred @jcouv this is ready for review. |
|
I'll try to take a look after I'm done with unions reviews |
| Diagnostic(ErrorCode.ERR_CollectionExpressionEscape, "[local]").WithArguments("R").WithLocation(10, 16)); | ||
| } | ||
|
|
||
| [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/75802")] |
There was a problem hiding this comment.
Should this and similar tests be linking to #81520 instead?
There was a problem hiding this comment.
enh :) I'm fine wither way.
|
I believe that I was meaning to take the writeup I send you directly and open it as a spec update to csharplang. I'll try to do that tomorrow. |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
Implementation changes LGTM, not going to be able to review tests until tomorrow.
jcouv
added
Feature - Collection Arguments
3 participants
Relates to test plan #80613