Fix ref safety of implicit calls that might capture refs in the receiver #76657
Conversation
ghost
added
the
untriaged
|
@jjonescz Could you provide more details about the change? How exactly it fixes the issues, etc. |
It's now superseded by the full call analysis.
|
@dotnet/roslyn-compiler for reviews, thanks |
|
I will try to get to this next week |
jjonescz
requested review from
a team and
RikkiGibson
and removed request for
cston
|
@RikkiGibson @jaredpar @dotnet/roslyn-compiler for reviews, thanks |
| else | ||
| { | ||
| // We have an extension method receiver. | ||
| Debug.Assert(methodInfo.Method?.IsExtensionMethod != false); |
There was a problem hiding this comment.
We'll want to either test new extensions here or add a tracking issue link for adding such tests.
There was a problem hiding this comment.
I've realized the "correct" way is to call ReplaceWithExtensionImplementationIfNeeded like the ref safety analysis does in other places. I will update the code to do that.
| } | ||
|
|
||
| private void GetInterpolatedStringHandlerArgumentsForEscape(BoundExpression expression, ArrayBuilder<BoundExpression> arguments) | ||
| private bool CheckValEscapeOfInterpolatedStringHandlerCalls(BoundExpression expression, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
Does this need to take in a BoundExpression? Or would it work to take in a BoundInterpolatedString and make the expression used for traversal a local?
There was a problem hiding this comment.
The expression can start as BoundBinaryOperator or BoundInterpolatedString. So yes, this needs to take BoundExpression.
|
@dotnet/roslyn-compiler for a second review, thanks |
1 similar comment
|
@dotnet/roslyn-compiler for a second review, thanks |
|
@dotnet/roslyn-compiler for a second review, thanks |
| return result; | ||
| } | ||
|
|
||
| private SafeContext GetValEscapeOfInterpolatedStringHandlerCalls(BoundExpression expression, SafeContext localScopeDepth) |
There was a problem hiding this comment.
Why do we need both of these functions? Surely Check can be implemented as a call to Get and a comparison to the allowed escape scope?
There was a problem hiding this comment.
Same reason we have Get*/Check* pairs elsewhere in ref safety analysis - better error reporting.
|
@333fred for another look, thanks |
* upstream/main: (87 commits) Fix ref safety of implicit calls that might capture refs in the receiver (dotnet#76657) Update dependencies from https://github.com/dotnet/dotnet build 278961 Updated Dependencies: System.CommandLine (Version 2.0.0-rc.1.25410.101 -> 2.0.0-rc.1.25411.109) Update checklist for adding new language version (dotnet#79881) Skip ValidateAllOptions flaky test Don't try to load file based projects unless we get a .cs file (dotnet#79844) Update package restore error message. [main] Source code updates from dotnet/dotnet (dotnet#79862) Fix failure to report integration test results when retrying Also fix newly failing tests feedback More semantic update changes (dotnet#79828) Fix flow analysis of extern local functions (dotnet#79741) Allow using `/main` with top-level statements (dotnet#79577) Delete Async JTF.run Delete comment Delete code Cancel in flight work Simplify Fix race condition ...
4 participants
Alternative to #76237 and #76263.
Fixes #75802.
Fixes #63306.
There can be implicit calls generated by the compiler like
collection.AddorinterpolatedStringHandler.AppendFormattedthat previously weren't fully analyzed by the ref safety analysis.For collection initializers, the ref safety analysis went over all the initializers (
Addcalls) and intersected their val escape scopes, but that essentially did nothing since thoseAddmethods usually returnvoidso GetValEscape doesn't look at them.For interpolated strings, the ref safety analysis went over all the arguments and intersected their val escape scopes.
But neither of those approaches detected when a ref passed as an
inargument could be captured into the receiver (collection / interpolated string handler). So this PR analyzes the implicit calls properly to detect that.All the existing get / check val escape utilities work on expressions and so they analyze calls only if they can actually return a ref. (Arg mixing checks if a ref can escape into the receiver but since here we are essentially trying to determine what the escape scope of the receiver should be, we cannot easily use that - I've tried that in the alternative PRs linked above but it wasn't very nice.) So I added other utilities to determine/check the escape scope of the receiver of a call.