Add generation for all IOperation nodes #38056
Conversation
|
@dotnet/roslyn-compiler please review. #Closed |
| @@ -3504,14 +3504,14 @@ private void AddExceptionStore(ITypeSymbol exceptionType, IOperation exceptionDe | |||
| if (exceptionTarget != null) | |||
| { | |||
| AddStatement(new SimpleAssignmentOperation( | |||
| exceptionTarget, | |||
| isRef: false, | |||
There was a problem hiding this comment.
formatting? #Resolved
| default: | ||
| return new SimpleAssignmentOperation(Visit(operation.LoopControlVariable), | ||
| isRef: false, // In C# this is an error case and VB doesn't support ref locals |
There was a problem hiding this comment.
seems like this comment was meant to remain attached toisRef: false? #Resolved
| @@ -77,7 +77,7 @@ | |||
| </Comments> | |||
| </Property> | |||
| </Node> | |||
| <Node Name="ISwitchOperation" Base="IOperation"> | |||
| <Node Name="ISwitchOperation" Base="IOperation" ChildrenOrder="Value,Cases"> | |||
There was a problem hiding this comment.
ChildrenOrder [](start = 50, length = 13)
It seems like sometimes it's validated that all properties are present in ChildrenOrder, but properties like Locals are not included. What am I missing about the way the validation works? #Closed
There was a problem hiding this comment.
Children are only IOperation or arrays of IOperation.
In reply to: 315387892 [](ancestors = 315387892)
Refers to: src/Compilers/Core/Portable/Generated/Operations.Generated.cs:2808 in ddee7cd. [](commit_id = ddee7cd, deletion_comment = False) |
| @@ -340,80 +348,202 @@ private void WriteClasses() | |||
| if (type.SkipClassGeneration) continue; | |||
|
|
|||
| var allProps = GetAllProperties(type); | |||
| bool hasSkippedProperties = !GetAllProperties(type, includeSkipGenerationProperties: true).SequenceEqual(allProps); | |||
| var ioperationTypes = allProps.Where(p => IsIOperationType(p.Type)).ToList(); | |||
There was a problem hiding this comment.
ioperationTypes [](start = 20, length = 15)
Consider naming ioperationProps. #Resolved
I wanted to break this up to avoid putting too much in one file, I'll add a copyright header. In reply to: 522740001 [](ancestors = 522740001) Refers to: src/Tools/Source/CompilerGeneratorTools/Source/IOperationGenerator/IOperationClassWriter.Verifier.cs:1 in ddee7cd. [](commit_id = ddee7cd, deletion_comment = False) |
| { | ||
| if (prop.Comments?.Elements?[0].Name != "summary" && !prop.IsInternal) | ||
| { | ||
| Console.WriteLine($"{abstractNode.Name}.{prop.Name} does not have correctly formatted comments, please ensure that there is a <summary> block for the type."); |
There was a problem hiding this comment.
type [](start = 178, length = 4)
"property"? #Resolved
|
|
||
| if (!abstractNode.Name.EndsWith("Operation")) | ||
| { | ||
| Console.WriteLine($"All IOperation node names must end with Operation. {abstractNode.Name} does not."); |
There was a problem hiding this comment.
Add error = true; #Resolved
|
|
||
| if (node.ChildrenOrder is null) | ||
| { | ||
| Console.WriteLine($"{node.Name} has more than 1 IOperation properties and must declare an explicit ordering with the ChildrenOrder attribute."); |
There was a problem hiding this comment.
properties [](start = 79, length = 10)
"property"? #Resolved
| continue; | ||
| } | ||
|
|
||
| var childrenOrdered = node.ChildrenOrder.Split(",", StringSplitOptions.RemoveEmptyEntries).Select(s => s.Trim()).ToList(); |
There was a problem hiding this comment.
.Split(",", StringSplitOptions.RemoveEmptyEntries).Select(s => s.Trim()) [](start = 56, length = 72)
Perhaps extract a helper method. #Resolved
…ange with default, minor formatting and naming suggestions.
|
@RikkiGibson @cston addressed feedback. #Closed |
| if (ioperationTypes.Count == 0) | ||
| bool hasSkippedProperties = !GetAllProperties(type, includeSkipGenerationProperties: true).SequenceEqual(allProps); | ||
| var ioperationProperties = allProps.Where(p => IsIOperationType(p.Type)).ToList(); | ||
| var publicIOperationTypes = ioperationProperties.Where(p => !p.IsInternal).ToList(); |
There was a problem hiding this comment.
publicIOperationTypes [](start = 20, length = 21)
publicIOperationProperties? #Resolved
| { | ||
| if (node.ChildrenOrder is string order) | ||
| { | ||
| var splitOrder = order.Split(",", StringSplitOptions.RemoveEmptyEntries).Select(s => s.Trim()).ToList(); |
There was a problem hiding this comment.
order.Split(",", StringSplitOptions.RemoveEmptyEntries).Select(s => s.Trim()).ToList(); [](start = 41, length = 87)
GetPropertyOrder(order) #Resolved
Almost all IOperation implementations in Microsoft.CodeAnalysis are now being generated. Exceptions are main NoneOperation, InvalidOperation, and the operations that involve dynamic, as we have a special base class they all inherit from internally that's used to implement some public API functionality. Unfortunately, as in the previous PR, I was unable to generate these new classes in-place. I've done some reordering of properties in the xml to make the constructors generate parameters in the same order as they did previously, but in a few cases that wasn't possible, and for those cases I've reordered parameters in other files. I've also standardized the names of the IConvertibleConversion properties, which caused a few renames in the code as well.
I've also added a verification step for the XML, to help prevent simple errors from being introduced. It will ensure that children have an explicit ordering if required, that operations follow naming conventions, that base types are correctly specified, and that summaries are appropriately included. @dotnet/roslyn-compiler please review.