Remove cecil FullName property from diagnostic messages #2449
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cecil `FullName` properties use '/' as delimiters for nested classes, but the standard is to use '.'. GetDisplayName extension is used instead to accomplish this.
vitek-karas
reviewed
Dec 15, 2021
| if (Origin?.Provider is MethodDefinition method) | ||
| sb.Append (method.GetDisplayName ()); | ||
| else if (Origin?.Provider is TypeDefinition type) | ||
| else if (Origin?.Provider is MemberReference type) |
Member
There was a problem hiding this comment.
While I think this is in general correct, it feels dangerous. If there can be an instance of IMemberDefinition which is not MemberReference we would now throw NotSupportedException. Which would be a regression. Quickly looking through Cecil, this should not be the case, but I don't know for sure.
Would be safer to keep the IMemberDefinition case as a fallback.
tlakollo
reviewed
Dec 15, 2021
| if (Origin?.Provider is MethodDefinition method) | ||
| sb.Append (method.GetDisplayName ()); | ||
| else if (Origin?.Provider is TypeDefinition type) | ||
| else if (Origin?.Provider is MemberReference type) |
Contributor
There was a problem hiding this comment.
nit: probably change the name from 'type', a memberReference could be several things
jtschuster
force-pushed
the
RemoveCecilFullNameDiags
branch
2 times, most recently
from
fcc8c0c to
86cf8f2
Compare
…are no regressions
jtschuster
force-pushed
the
RemoveCecilFullNameDiags
branch
from
86cf8f2 to
93fb2a7
Compare
vitek-karas
approved these changes
Dec 15, 2021
tlakollo
approved these changes
Dec 15, 2021
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
…NameDiags Remove cecil FullName property from diagnostic messages Commit migrated from dotnet/linker@0ee8afd
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Diagnostic messages would occasionally use the Cecil
FullNameproperty which uses '/' as a delimiter for nested classes. The standard is to use '.' instead. This fix usesGetDisplayName()instead to accomplish this.Fixes #2441
Customer Impact
Improves warning messages by better formatting nested type names.
Testing
Manually verified improved warning messages.
Risk
Very low - only affects warning messages.