Symbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) does not escape everything it should

[SteveDunn]

I think I've found a bug in Symbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat).

Previously, in my source generator, I used the following to get the fully qualified name: value.FullName() ?? value.Name ?? throw new InvalidOperationException()

But it was recommended I use .ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) instead.

But it doesn't escape everything it should.

Version Used: Happens in all versions tested.

Steps to Reproduce:

In Vogen, my source generator, I look for ValueObject attributes and get a symbol from the underlying type argument. If I give the following horrendous (but legal) C#@

using Vogen;
namespace @class
{
    namespace record.@struct.@float
    {
        public readonly record struct @decimal();
        public readonly record struct @event2();
        public readonly record struct @event();
    }

  [ValueObject(underlyingType: typeof(record.@struct.@float.@decimal))]
  public partial class MyVo 
  { 
  }

... then I get different results. Here's the difference between the two:

value.FullName() "@class.@record.@struct.@float.@decimal"

value.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) "global::@class.record.@struct.@float.decimal"

Expected Behavior: value.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat) should escape decimal. It might also want to escape record

Actual Behavior: It doesn't escape decimal and leads to illegal C# code being emitted from my source generator.

[CyrusNajmabadi]

Sorry. I missed this was for creating a type syntax. The decimal part would be a bug. The record part looks correct to me. It's not a keyword.

[SteveDunn]

I'm a bit confused now. In my source generator, I want to emit the fully qualified name of a symbol. Should I use FullName() or ToDisplayString(), or perhaps something else entirely?

[333fred]

The record part looks correct to me. It's not a keyword.

I could go either way here. It will generate a warning if it's a class name, so it feels like the safer thing to do is just escape it always.

[333fred]

@CyrusNajmabadi, did you have a response to my comment?

[CyrusNajmabadi]

Fine with me always escape it.

[333fred]

Still need to handle escaping record itself.

[SteveDunn]

Sorry, lost track of this one @333fred . Should I: a) create a new issue and a new PR b) create a new PR and reference this issue c) [something else]

[SteveDunn]

Guessing b) seeing as this was just reopened

[333fred]

B; if you want to take care of it, we'd absolutely welcome the PR :slightly_smiling_face:.

[SteveDunn]

B; if you want to take care of, we'd absolutely welcome the PR 🙂.

Leave it to me boss!

[SteveDunn]

https://github.com/dotnet/roslyn/pull/74227