Roslyn: CS0108 Incorrectly shown with fix that breaks code

[TonyValenti]

Version Used:
Visual Studio 2017

Steps to Reproduce:
Put this code into C#:

````
public class BaseClass {
public const int PRIORITY = 100;
}

public class DerivedClass : BaseClass {
    public const int PRIORITY = BaseClass.PRIORITY + 100;
}

````

You get the following warning:
Warning CS0108 'DerivedClass.PRIORITY' hides inherited member 'BaseClass.PRIORITY'. Use the new keyword if hiding was intended.

If you right-click on the squiggilies and select Quick Actions and Refactorings > Hide base member, it changes the code to look like this:

public class DerivedClass : BaseClass { public const new int PRIORITY = BaseClass.PRIORITY + 100; }

The problem is, that code doesn't compile and now generates 4 errors instead of one warning.

[sharwell]

:memo: The problem with the code fix is it fails to preserve the order of modifiers as required by the language specification:

https://github.com/dotnet/csharplang/blob/master/spec/classes.md#constants

constant_declaration
    : attributes? constant_modifier* 'const' type constant_declarators ';'
    ;

[sharwell]

:memo: I'm reserving this for a first-time contributor (or any infrequent non-Microsoft contributor) for the next few days. If you're a first time contributor and want to try your hand at working on Roslyn, this is a pretty basic code fix which you'll find implemented in HideBaseCodeFixProvider.AddNewKeywordAction.cs. The tests for this feature are in HideBaseTests.cs.

[MaximRouiller]

@sharwell Alright. Let's say I'm interested.

I'm downloading the code on my machine now. I'll be checking out from the master branch.

I'm installing the requirements right now. Anything I should know about Restore/Build/Test that might go wrong?

[sharwell]

@MaximRouiller The instructions were recently updated (courtesy of @jaredpar) and seem to accurately reflect what I experienced getting started just a couple weeks ago. Personally I've been using the xunit.runner.wpf approach for running tests which is described in that document. The specific assembly you'll need to open up in the runner will be found at the following location after you build the test project:

Binaries\Debug\UnitTests\CSharpEditorServicesTest\Roslyn.Services.Editor.CSharp.UnitTests.dll

[MaximRouiller]

Thanks!

Testing this project is a good way to run your CPU to 100% for a few minutes btw. 😉

[MaximRouiller]

Have you tried light weight solution load on Roslyn?

[sharwell]

Have you tried light weight solution load on Roslyn?

I haven't.

[Pilchie]

Assigning to Sam to act as shepherd for this.

[MaximRouiller]

So if I get this right...

Current

public class DerivedClass : BaseClass {
        public const new int PRIORITY = BaseClass.PRIORITY + 100;
    }

Expected

public class DerivedClass : BaseClass {
        public new const int PRIORITY = BaseClass.PRIORITY + 100;
    }

[MaximRouiller]

I'm at the right place in the code. I see where the annotation is right now.

I have another Visual Studio Solution opened with the actual problem replicated.

How do I go and debug this?

[sharwell]

So if I get this right...

👍

How do I go and debug this?

The way I go and debug this may or may not be the best way, but it seems to be working so far.

1. Write a unit test that I expect to work properly
2. Go through the solution and right click → Unload Project on all the test projects except for CSharpEditorServicesTests (which is the test project containing the test you just wrote). This step makes Test Explorer fast enough to use for a solution this size.
3. Set the Test Explorer search filter to Class:HideBaseTests
4. Build CSharpEditorServicesTests and wait for Test Explorer to populate the tests
5. Right click on the test you added, and click Debug Selected Tests

[MaximRouiller]

Haven't seen the [|public int Test;|] syntax before.

Might be worth documenting...

[MaximRouiller]

Since I'm baby stepping this... I'll post what I have so far and the test seems good.

Unit Test

        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddNew)]
        public async Task TestAddNewToConstant()
        {
            await TestInRegularAndScriptAsync(
@"class Application
{
    public const int Test = 1;
}

class App : Application
{
    [|public const int Test = Application.Test + 1;|]
}",
@"class Application
{
    public const int Test = 1;
}

class App : Application
{
    public new const int Test = Application.Test + 1]
}");
        }

Output

Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics.HideBase.HideBaseTests.TestAddNewToConstant FAILED:
Exception type: 'Xunit.Sdk.FalseException', number: '0', parent: '-1'
Exception message:
Unexpected token. Actual 'Application { public ^const^ new int Test = Application ' Expected 'Application { public ^new^ const int Test = Application '
Expected: False
Actual: True
Exception stacktrace
...

Is it a good starting point?

[sharwell]

... [|stuff|] syntax ... Might be worth documenting...

I agree. It's used by the test framework for various reasons, but I'm not 100% on the specifics. Want to create a new issue for this, since documentation would help new contributors?

Is it a good starting point?

Looks good to me. However, in the expected output you should have ; instead of ] at the end of the modified line.

[MaximRouiller]

you should have ; instead of ]

Agreed. Now to find the root cause and fix it. 😄

[MaximRouiller]

Am I wrong in believing that AddModifiers only append at the end.

While const is also a modifier, it would mean that when we are adding new to the mix, it will automatically do const new.

What I'm trying to do... is "when const, insert at index" to see if my theory is correct.

Is it the way refactorings works? Adding things in the proper order without having them re-ordered when needed?

[sharwell]

Is it the way refactorings works? Adding things in the proper order without having them re-ordered when needed?

Yes, manipulation of tokens is a pretty low-level operation. This ensures that code fixes retain maximum control over the output, allowing for things like implementing custom formatting operations as a code fix.

[MaximRouiller]

I found something interesting. InsertTokensBefore.

I think it's part of the solution.

[CyrusNajmabadi]

Note: my preferred way to do this would be as follows:

Use SyntaxGenerator (which you can get from document.GetLanguageService<SyntaxGenerator>(). Then call generator.WithModifiers(d, generator.GetModifiers(d).WithIsConst(true));

SyntaxGenerator is our 'smart' syntax modifier. It is supposed to know how to generate syntactically correct code. If it does not place 'const' last, it should be fixed to place const last. But once that is workign properly, then anyone can use SyntaxGenerator properly for this sort of thing.

[CyrusNajmabadi]

Looks like SyntaxGenerator will do the right thing most of the time:

https://github.com/dotnet/roslyn/blob/master/src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs#L1749

Technically adding const should be after adding unsafe/readonly as those are true modifiers.

But SyntaxGenerator is def the way to go here.

[CyrusNajmabadi]

As an added bonus, you don't need to manually handle any specific C# node types. And if we go this approach, we'd get VB support for free as well.

[MaximRouiller]

@CyrusNajmabadi

Will definitely check it out!

[MaximRouiller]

@CyrusNajmabadi

Wow. The syntax is actually way simpler with that. I might be able to simplify the code by a lot.

[CyrusNajmabadi]

great! :)

[MaximRouiller]

We may have to rewrite the other implementations to reuse the SyntaxGenerator.

I'm not going to include that in that pull request... but it's definitely a "TODO" ticket. Maybe another up for grabs? 😛

[CyrusNajmabadi]

sure. i can take care of it once your work goes in.

[MaximRouiller]

All tests passing. Creating pull request now so we can discuss.

[MaximRouiller]

Holy cow... it also works in VS! 💃 😄

PR: https://github.com/dotnet/roslyn/pull/18399