Initial support for non-boxing union TryGetValue access pattern#82325
Conversation
|
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. |
RikkiGibson
left a comment
There was a problem hiding this comment.
Got thru implementation and some tests. Taking a break and will try to finish later today.
| static bool isTryGetValueSignature(MethodSymbol method) | ||
| { | ||
| // PROTOTYPE: Cover individual conditions with tests | ||
| return method is |
There was a problem hiding this comment.
Is the TryGetValue method allowed to have type parameters? #Resolved
There was a problem hiding this comment.
Is the TryGetValue method allowed to have type parameters?
Good question. Not according to my reading of the spec. I'll adjust the condition.
| if (input.UnionValue is { } unionValue) | ||
| { | ||
| BoundDagTemp temp = MakeUnionValue(input.DagTemp, unionValue, out BoundDagEvaluation valueEvaluation); | ||
| unionValueEvaluation = new Tests.One(valueEvaluation); |
There was a problem hiding this comment.
nit: consider both declaring + assigning unionValueEvaluation at this point. #Resolved
|
@333fred, @dotnet/roslyn-compiler For a second review |
1 similar comment
|
@333fred, @dotnet/roslyn-compiler For a second review |
333fred
left a comment
There was a problem hiding this comment.
Got through the impl side. Will get to the tests this afternoon.
| if (candidate.Parameters[0].Type.Equals(type, TypeCompareKind.AllIgnoreOptions) && // PROTOTYPE: Allow conversions per spec | ||
| caseTypes.Contains(method.Parameters[0].Type, Symbols.SymbolEqualityComparer.AllIgnoreOptions)) // PROTOTYPE: Optimize this check? | ||
| { | ||
| if (method.GetUseSiteInfo().DiagnosticInfo?.DefaultSeverity == DiagnosticSeverity.Error) |
There was a problem hiding this comment.
I'm a bit conflicted here: I think there's a valid argument that we because TryGetValue is an optimization, we should fall back to just using Value. But I also somewhat feel like we should just proceed and report the error, rather than bailing for some potentially non-obvious reason and silently getting boxing codegen.
There was a problem hiding this comment.
I'm a bit conflicted here: I think there's a valid argument that we because
TryGetValueis an optimization, we should fall back to just usingValue. But I also somewhat feel like we should just proceed and report the error, rather than bailing for some potentially non-obvious reason and silently getting boxing codegen.
I will capture this in a speclet as an open question
No description provided.