Improve DisplayNameHelpers for NativeAOT #70084
Conversation
These helpers are used to report names of things in warnings. The functional changes are: * For method parameters, use the parameter name if available (and only if not fallback to the #1 notation) * For property accessor methods, use the C# naming scheme, so for example Type.Property.get instead of Type.get_Property. Both of these changes are in preparation to bring NativeAOT closer in behavior to ILLink and the trim analyzers. For this I moved some of the helpers to the common shared code. Some unrelated code cleanup as well.
| { | ||
| sb.Append(property.Name); | ||
| sb.Append('.'); | ||
| sb.Append(property.GetMethod.Name == method.Name ? "get" : "set"); |
There was a problem hiding this comment.
If we want to match C# syntax, should this also handle init?
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/init
(Not necessary to fix in this PR.)
There was a problem hiding this comment.
I created dotnet/linker#2816 to add this to linker/analyzer/aot (They are going to share tests soon, so we will need to do it in all 3).
There was a problem hiding this comment.
I wonder if it would be better to just standardize on however reflection displays these. This is the same issue as with interpreting ref/in/out in method signatures. C# tends to come up with new things every release. Do we want to be in a position where we need to play catch up, or in the position of reflection that just doesn't care?
dotnet/linker#2158 (comment) (ref/in/out comment here)
There was a problem hiding this comment.
I must admit I thought about it - but I was lazy to fix probably 100s of places in the linker tests to change this.
Another way would be to implement some smarts to the test validation system to let it allow a difference here - probably doable - didn't try.
|
|
||
| var type = (EcmaType)ecmaAccessor.OwningType; | ||
| var reader = type.MetadataReader; | ||
| var module = type.EcmaModule; |
There was a problem hiding this comment.
The module seems to be unused in this method
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DiagnosticUtilities.cs
Outdated
Show resolved
Hide resolved
| using System.Text; | ||
|
|
||
| using Internal.TypeSystem; | ||
|
|
||
| using Internal.TypeSystem.Ecma; |
There was a problem hiding this comment.
Nit: we tend to split using Foo = ... from the regular usings with a newline.
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
ghost
locked as resolved and limited conversation to collaborators
These helpers are used to report names of things in warnings. The functional changes are:
Both of these changes are in preparation to bring NativeAOT closer in behavior to ILLink and the trim analyzers.
For this I moved some of the helpers to the common shared code.
Some unrelated code cleanup as well.