Ensure that refkinds are rewritten for complex methods#79916
Ensure that refkinds are rewritten for complex methods#79916333fred merged 5 commits intodotnet:mainfrom
Conversation
29ec6ce to
80ea18a
Compare
|
@dotnet/roslyn-compiler for review |
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
Outdated
Show resolved
Hide resolved
| // keyword | ||
| else if (paramRefKind is RefKind.Ref && argRefKind == RefKind.None && parameters[p].Type is NamedTypeSymbol { IsInterpolatedStringHandlerType: true, IsValueType: true }) | ||
| { | ||
| argRefKind = RefKind.Ref; |
There was a problem hiding this comment.
I'll add an assert to this effect. My expectation is that the only things that can get to this part of binding with "mismatched" ref kinds are:
invsref readonly, handled above.- COM scenarios
- String interpolation into an interpolated string handler.
There was a problem hiding this comment.
Actually, asserting this is going to be either complicated or fragile: at this point we have the rewritten BoundSequence from the conversion, not the original interpolation. I could assert part or all of the shape of the sequence, but that feels somewhat fragile to refactoring; a more robust check of "what we had before rewriting" would need us to pass the original arguments through a couple of layers with 5-10 callsites to update. That doesn't strike me as a worthwhile tradeoff. What do you think @AlekseyTs?
There was a problem hiding this comment.
What do you think?
After taking a closer look, it appears to me that the issue is caused by a duplicated logic that got out of sync. We already have an argument ref kind patching similar to the one added here in GetEffectiveArgumentRefKind. Therefore, I am fine with proceeding without an assert or a stricter condition. However, I think we should extract a shared helper, "GetEffectiveArgumentRefKind", to calculate effective argument ref kind and reuse it in both places.
There was a problem hiding this comment.
However, I think we should extract a shared helper, "GetEffectiveArgumentRefKind", to calculate effective argument ref kind and reuse it in both places.
I actually started with this approach; the main issue is that there is a slight difference in expected behavior between these spots. In this location, we only want to rewrite None to Ref for interpolated strings; COM can cause similar mismatches, but that isn't handled here. In the other location we have this check, we unconditionally do the rewrite, as we only go through that path for non-complex locations (which excludes COM). We don't actually know in this method whether or not COM is involved, and in attempting to refactor that information to pass it through to include in the check, I also found that needed a number of callsites to be updated. I see two options for consistency:
- Add a comment to both locations to tell anyone who updates one to go update the other.
- Parameterize the helper, and assume there will be no assert about the COM mismatch in this case.
There was a problem hiding this comment.
@AlekseyTs did you have have a thought on this?
There was a problem hiding this comment.
did you have have a thought on this?
At the moment, I do not have a preference. I suggest implementing an option that you prefer and go through the regular review process.
src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (commit 3) |
|
@dotnet/roslyn-compiler for reviews. |
|
@dotnet/roslyn-compiler for a second review |
| } | ||
| } | ||
|
|
||
| internal static RefKind GetEffectiveRefKind(RefKind paramRefKind, RefKind currentArgRefKind, TypeSymbol paramType, bool comRefKindMismatchPossible) |
There was a problem hiding this comment.
I'm not sure what "current" signifies in currentArgRefKind. I would name it just argRefKind.
There was a problem hiding this comment.
Will rename to initialArgRefKind; I want to keep the connotation that this is what the starting point was, and the method gets the real ref kind post-updates. Hopefully initial makes that clearer.
| } | ||
|
|
||
| internal static RefKind GetEffectiveRefKind(RefKind paramRefKind, RefKind currentArgRefKind, TypeSymbol paramType, bool comRefKindMismatchPossible) | ||
| { |
There was a problem hiding this comment.
In BuildStoresToTemps there was an assert Debug.Assert(argRefKind is RefKind.None or RefKind.In or RefKind.Ref); - consider preserving that.
* upstream/main: (233 commits) Extensions: add SyntaxGenerator support and AssociatedExtensionImplementation API (dotnet#80170) Fix error when hoisting a non-ref call (dotnet#80138) Ensure that refkinds are rewritten for complex methods (dotnet#79916) Revert Do not go through the workspace to access services DefiniteAssignmentPass.MarkFieldsUsed - avoid infinite recursion due to generic substitution (dotnet#80135) Reduce allocations in AnalyzerDriver.TryExecuteSymbolEndActions (dotnet#79855) RefSafetyAnalysis: Fix handling of nested deconstruction utilizing modern extensions (dotnet#80231) Extensions: adjust rewriting of anonymous type property symbols (dotnet#80211) Extensions: Move public APIs into INamedTypeSymbol (dotnet#80230) Extensions: improve error recovery in older language versions (dotnet#80206) Fall back to `dotnet exec` if apphost does not exist (dotnet#80153) Update dependencies from https://github.com/dotnet/dotnet build 282708 (dotnet#80228) Add a workaround for microsoft/vs-mef#620 Revert "FailFast if the MEF composition is clearly broken" switch from windows combobox to visualstudio combobox (dotnet#80219) Update System.Text.Json in packages which use 4.12 Roslyn (dotnet#80197) add flags to unblock CI (dotnet#80222) Move static members to another type - qualifies static member references in the moved members (dotnet#80178) Fix broken link for C# 14 lambda parameter modifiers ...
Fixes #79888