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

CS0108 Incorrectly shown with fix that breaks code #18391

Closed
TonyValenti opened this Issue Apr 3, 2017 · 28 comments

Comments

Projects
None yet
5 participants
@TonyValenti

TonyValenti commented Apr 3, 2017

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

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

📝 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 sharwell added this to the 15.3 milestone Apr 3, 2017

@sharwell

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

📝 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.

@sharwell sharwell added the help wanted label Apr 3, 2017

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

@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

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

@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

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

Thanks!

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

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

Have you tried light weight solution load on Roslyn?

@sharwell

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

Have you tried light weight solution load on Roslyn?

I haven't.

@Pilchie

This comment has been minimized.

Member

Pilchie commented Apr 3, 2017

Assigning to Sam to act as shepherd for this.

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

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

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

Might be worth documenting...

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

... [|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

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

you should have ; instead of ]

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

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

This comment has been minimized.

Member

sharwell commented Apr 3, 2017

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

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

I found something interesting. InsertTokensBefore.

I think it's part of the solution.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Apr 3, 2017

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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Apr 3, 2017

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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Apr 3, 2017

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

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

@CyrusNajmabadi

Will definitely check it out!

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

@CyrusNajmabadi

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

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Apr 3, 2017

great! :)

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

This comment has been minimized.

Contributor

CyrusNajmabadi commented Apr 3, 2017

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

MaximRouiller added a commit to MaximRouiller/roslyn that referenced this issue Apr 3, 2017

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

@MaximRouiller

This comment has been minimized.

Contributor

MaximRouiller commented Apr 3, 2017

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

PR: #18399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment