Fix code generation to add default constraint when needed#53318
Fix code generation to add default constraint when needed#53318CyrusNajmabadi merged 12 commits intodotnet:mainfrom
Conversation
| return !method.ExplicitInterfaceImplementations.Any() && !method.IsOverride | ||
| ? method.TypeParameters.GenerateConstraintClauses() | ||
| : default; | ||
| : GenerateDefaultConstraints(method); |
There was a problem hiding this comment.
Are there any cases where this will be generating extra default constraints?
There was a problem hiding this comment.
i think this is fine.
| var seenTypeParameters = new HashSet<string>(); | ||
| var listOfClauses = new List<TypeParameterConstraintClauseSyntax>(method.TypeParameters.Length); |
There was a problem hiding this comment.
pooled collections please :)
| foreach (var parameter in method.Parameters) | ||
| { | ||
| if (parameter.Type is not ITypeParameterSymbol { NullableAnnotation: NullableAnnotation.Annotated } typeParameter || | ||
| !seenTypeParameters.Add(parameter.Type.Name)) |
There was a problem hiding this comment.
oh please don't mix a check and a mutation in the same expression. separate out to two if blocks.
There was a problem hiding this comment.
@CyrusNajmabadi Will do. But curious if there is any particular reason?
There was a problem hiding this comment.
99.999% of all checks are non-mutating (true number :)). As such, it can be easy to miss and get confused by the odd case which looks like a normal if check but does mutate. I like uncommon cases to stick out so it is very easy to see and understand what is going on.
| continue; | ||
| } | ||
|
|
||
| TypeParameterConstraintSyntax constraint; |
There was a problem hiding this comment.
consider a switch or ternary. up to you though.
| class D : I | ||
| { | ||
| void I.M<T1, T2, T3>(T1? a, T2 b, T1? c, T3? d) | ||
| where T1 : class |
There was a problem hiding this comment.
that's interesting. are we supposed to keep that in?
There was a problem hiding this comment.
@CyrusNajmabadi Yes. There are compile errors otherwise.
|
@CyrusNajmabadi Thanks for the fixes! There is now:
I think it was fixed in #58754, can you re-run the failing checks? Thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
Thanks @Youssef1313 |
Fixes #53012
Fixes #53003 (please confirm on this since the description of that issue isn't 100% clear to me).