roslyn icon indicating copy to clipboard operation
roslyn copied to clipboard

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

Open SteveDunn opened this issue 1 year ago • 5 comments

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.@[email protected]"

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.

SteveDunn avatar Jun 24 '24 06:06 SteveDunn

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.

CyrusNajmabadi avatar Jun 24 '24 06:06 CyrusNajmabadi

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?

SteveDunn avatar Jun 24 '24 07:06 SteveDunn

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 avatar Jun 24 '24 17:06 333fred

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

333fred avatar Jun 26 '24 17:06 333fred

Fine with me always escape it.

CyrusNajmabadi avatar Jun 26 '24 23:06 CyrusNajmabadi

Still need to handle escaping record itself.

333fred avatar Jul 01 '24 20:07 333fred

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 avatar Jul 01 '24 20:07 SteveDunn

Guessing b) seeing as this was just reopened

SteveDunn avatar Jul 01 '24 20:07 SteveDunn

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

333fred avatar Jul 01 '24 20:07 333fred

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

Leave it to me boss!

SteveDunn avatar Jul 01 '24 20:07 SteveDunn

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

SteveDunn avatar Jul 01 '24 21:07 SteveDunn

This was done in #74227.

333fred avatar Dec 10 '24 22:12 333fred