-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Escape record
keywords
#74227
Escape record
keywords
#74227
Conversation
src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.cs
Outdated
Show resolved
Hide resolved
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.
@dotnet/roslyn-compiler for a second review.
return new SymbolDisplayPart(kind, symbol, text); | ||
} | ||
|
||
bool isNamedTypeOrAliasName = symbol?.Kind is SymbolKind.NamedType or SymbolKind.Alias; |
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.
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.
Thanks, I have added a test for an alias. If Goo
the new Foo
? :)
|
||
bool isEscapable = IsEscapable(kind); | ||
|
||
text = isEscapable ? EscapeIdentifier(text, isNamedTypeOrAliasName) : text; |
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.
Consider rearranging to avoid calculating isNamedTypeOrAliasName
or reassigning text
unnecessarily. Perhaps:
if (_escapeKeywordIdentifiers && IsEscapable(kind))
{
text = EscapeIdentifer(text, symbol?.Kind is SymbolKind.NamedType or SymbolKind.Alias);
}
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.
Actually, looking at the previous line, we return if !escapeKeywordIdentifers
- so we can omit that check here.
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.
Yes, we're checking !escapeKeywordIdentifiers
earlier but the two if
statements could be combined to avoid two return new SymbolDisplayPart(kind, symbol, text);
.
…dd a unit test for escaping an alias.
See issue #74117 where it was decided that records should be escaped, even though they're not keywords, as it is the safer thing to do.
See other PR #74125