-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Refactoring of EntityToString API #3635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the entity-to-string conversion API by consolidating multiple entity-specific methods (MethodToString, FieldToString, PropertyToString, EventToString) into a unified EntityToString method. The refactoring introduces a new ILAmbience class that handles IL-style formatting of types and entities using ConversionFlags instead of multiple boolean parameters.
Key changes:
- Introduced
ILAmbienceclass withConvertSymbolandConvertTypemethods to handle formatting - Replaced boolean parameters in API methods with
ConversionFlagsenum for more granular control - Deprecated entity-specific methods in favor of unified
EntityToStringAPI - Migrated
EscapeNameutility methods fromLanguageclass toILAmbience
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ICSharpCode.Decompiler/IL/ILAmbience.cs | New file implementing the core IL formatting logic with ConvertSymbol/ConvertType methods |
| ILSpy/Languages/Language.cs | Updated TypeToString signature, added EntityToString, deprecated old entity-specific methods, removed TypeToStringVisitor inner class |
| ILSpy/Languages/CSharpLanguage.cs | Removed overrides of deprecated methods, updated TypeToString and added EntityToString override |
| ICSharpCode.ILSpyX/Abstractions/ILanguage.cs | Updated interface to replace deprecated methods with EntityToString |
| ILSpy/ViewModels/CompareViewModel.cs | Updated entity formatting calls to use new EntityToString API with appropriate ConversionFlags |
| ILSpy/TreeNodes/*.cs | Updated all tree node classes to use EntityToString with ConversionFlags instead of deprecated methods |
| ILSpy/Analyzers/TreeNodes/*.cs | Updated analyzer tree nodes to use EntityToString with appropriate flags |
| ILSpy/Search/SearchResultFactory.cs | Updated search result formatting to use new API |
| ICSharpCode.Decompiler/Disassembler/ReflectionDisassembler.cs | Changed EnumNameCollection visibility to internal static, converted from class to struct, updated WriteEnum/WriteFlags signatures |
| ICSharpCode.Decompiler/CSharp/OutputVisitor/CSharpAmbience.cs | Minor visibility and nullable annotation updates |
| ICSharpCode.Decompiler/ICSharpCode.Decompiler.csproj | Added ILAmbience.cs to project |
Comments suppressed due to low confidence (1)
ICSharpCode.Decompiler/Disassembler/ReflectionDisassembler.cs:2031
- Changing EnumNameCollection from a class to a struct causes issues with the collection initializer syntax used for static readonly fields (lines 102, 118, 127, 136, 143, 1270, 1279, 1418, 1492). When using collection initializers like "new EnumNameCollection() { ... }", the Add method is called on the struct instance. However, since these are static readonly fields, the struct instance is readonly, and calling Add (which modifies the struct) will only modify a temporary copy, leaving the actual field empty. This will cause all enum/flag name lookups to fail silently.
internal struct EnumNameCollection<T> : IEnumerable<KeyValuePair<long, string>> where T : struct
{
List<KeyValuePair<long, string>> names = new List<KeyValuePair<long, string>>();
public EnumNameCollection()
{
}
public void Add(T flag, string name)
{
this.names.Add(new KeyValuePair<long, string>(Convert.ToInt64(flag), name));
}
public IEnumerator<KeyValuePair<long, string>> GetEnumerator()
{
return names.GetEnumerator();
}
System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
{
return names.GetEnumerator();
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
26eed2b to
80ba0c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
80ba0c3 to
77e0156
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sealed class EnumNameCollection<T> : IEnumerable<KeyValuePair<long, string>> where T : struct | ||
| internal struct EnumNameCollection<T> : IEnumerable<KeyValuePair<long, string>> where T : struct | ||
| { | ||
| List<KeyValuePair<long, string>> names = new List<KeyValuePair<long, string>>(); |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field names is not initialized in the parameterless constructor at line 2013. In C#, struct field initializers are not executed for default struct construction. When EnumNameCollection instances are created using field initializers (lines 102, 118, 127, etc.), the parameterless constructor won't be called, and names will be null, causing NullReferenceException when Add() is called. The field should be initialized as List<KeyValuePair<long, string>> names = new(); or the struct should remain a class.
| if (string.IsNullOrEmpty(typeDef.Namespace)) | ||
| { | ||
| writer.Write(typeDef.Namespace); | ||
| writer.Write('.'); | ||
| } |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition at line 139 checks if (string.IsNullOrEmpty(typeDef.Namespace)), but then writes the namespace. This logic is inverted - it should check if (!string.IsNullOrEmpty(typeDef.Namespace)) to write the namespace only when it's not empty.
| { | ||
| switch (owner) | ||
| { | ||
| case IType t: |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to t is useless, since its value is never read.
| case IType t: | |
| case IType: |
| case IType t: | ||
| writer.Write("`"); | ||
| break; | ||
| case IMethod m: |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to m is useless, since its value is never read.
| case IMethod m: | |
| case IMethod: |
| else if (ConversionFlags.HasFlag(ConversionFlags.UseFullyQualifiedTypeNames)) | ||
| { | ||
| if (string.IsNullOrEmpty(typeDef.Namespace)) | ||
| { | ||
| writer.Write(typeDef.Namespace); | ||
| writer.Write('.'); | ||
| } |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 'if' statements can be combined.
| else if (ConversionFlags.HasFlag(ConversionFlags.UseFullyQualifiedTypeNames)) | |
| { | |
| if (string.IsNullOrEmpty(typeDef.Namespace)) | |
| { | |
| writer.Write(typeDef.Namespace); | |
| writer.Write('.'); | |
| } | |
| else if (ConversionFlags.HasFlag(ConversionFlags.UseFullyQualifiedTypeNames) && string.IsNullOrEmpty(typeDef.Namespace)) | |
| { | |
| writer.Write(typeDef.Namespace); | |
| writer.Write('.'); |
No description provided.