Roslyn: IDE0009 wrongly generated when using the base keyword

[ghost]

Version Used: VS2017 15.0.0-RTW+26228.4

Steps to Reproduce: Consider this code

class A
{
    public virtual int Value => 5;
}

class B : A
{
    public override int Value => base.Value + 6; // warning here
}

Expected Behavior: No warning generated for IDE0009 on the marked line

Actual Behavior: IDE0009 is generated here

The analysis is completely wrong. Applying 'this' would create an infinite loop. The property does not need to be an expression body for IDE0009 to be generated. Value { get { base.Value + 6; } } will cause the warning as well.

[jasonmalinowski]

@CyrusNajmabadi I think you already fixed this one?

[CyrusNajmabadi]

I don't think i've done any work in this space. I'm also not familiar with this area (Simplification i believe). Maybe this should go to @DustinCampbell ?

[jasonmalinowski]

@CyrusNajmabadi: we should probably expand knowledge to include people not responsible for unrelated deliverables...

[jasonmalinowski]

@CyrusNajmabadi nominated @sharwell, so sure. :-)

[sharwell]

I'm reserving this for a new first time contributor for the next three days.

Interested in trying your hand at this? Here's what you need to know:

1. If you are serious about working on this, add a comment here. While multiple people are allowed to participate, I'm willing to go out of my way to make sure the first contributor working on this gets their code merged into Roslyn.
2. I know more about this issue than I've provided here. If you get stuck, ask any questions you want and I'll fill out details. I don't want this to be a boring task, but I also don't want you to get stuck.

The equivalent code in Visual Basic produces the same problem:

Public Class A
    Public Overridable ReadOnly Property Value As Integer
End Class

Public Class B
    Inherits A

    Public Overrides ReadOnly Property Value As Integer
        Get
            Return MyBase.Value + 1
        End Get
    End Property
End Class

Additional information for getting this figured out:

1. There is a missing step in the Steps to Reproduce. You need to make sure you have the IDE configured to prefer to qualify property accesses.

   

   

2. The source code for this feature is in AbstractQualifyMemberAccessDiagnosticAnalyzer.cs and its derived types

3. The tests for this feature are in QualifyMemberAccessTests.cs and QualifyMemberAccessTests.vb

[RobSiklos]

@sharwell I'd like to give it a try. Should be able to get to in the next week or so.

[sharwell]

@RobSiklos Suitable issues like this come up pretty often. I'm hoping to get this one closed out by the end of the week; do you mind if I leave it up for grabs, or do you think you'll be able to start working on it sooner? You always tweet at me when you're ready to tackle something and I can find you an appropriate one. 👍

[RobSiklos]

@sharwell Sure - if someone else wants it, they can take it.

[RobSiklos]

@sharwell I took a look, and I think I have a working fix, however, the VB project is giving me trouble - looks like it doesn't have the references working properly. Here's a screenshot of the errors I'm getting: http://i.imgur.com/wzjxYNk.png. Maybe there's a more suitable place to get help for this kind of non-issue-related error?

[sharwell]

@RobSiklos when working through an issue like this with a first-time contributor, it's fine to ask your questions here. Among other things, we use it as a gauge of how difficult it is for someone to start contributing.

Can you check which specific version of Visual Studio 2017 you are using? Also, do you know which commit of Roslyn you are using as the base for your work? The following Git command will (probably) tell you:

git merge-base origin/master head

[Paxxi]

I'm interested in this one. Holiday in Sweden tomorrow so good timing 😄

[sharwell]

@RobSiklos I was able to reproduce a similar error by using Visual Studio 2017 version 15.2. Starting with pull request #19576 (merged yesterday), you'll need Visual Studio 2017 15.3 Preview 1 or newer to develop Roslyn. While it still compiles using an older version, it seems the error message you get when you open a VB file is pretty terrible.

If you don't want to update, you can create a branch for your work off of commit 7eba8523896899e9db5b5faa4c2de3abba44e288. At this point there are not likely to be merge conflicts when you submit your PR.

[sharwell]

@Paxxi It looks like @RobSiklos got to this quicker than expected. Would you like for me to find you a similar issue to work on before tomorrow?

[Paxxi]

If there is one then I'm interested in looking at it but don't feel any pressure to find a good issue.

[sharwell]

@Paxxi you kidding? I would file a new issue if that's what it took to make a good one available! 😁 But I found a great one for you.

[Paxxi]

hehe awesome 😄

[sharwell]

@RobSiklos During my explanation of another issue for @Paxxi I found another case you'll want to address as part of this bug. The MyClass keyword in Visual Basic is similar to the MyBase keyword, so whatever you do to fix the code for one you'll want to do for the other as well. Here is a test case you can leverage:

Public Class B
    Public Overridable ReadOnly Property Value As Integer

    Public Overrides ReadOnly Property OtherValue As Integer
        Get
            Return MyClass.Value + 1
        End Get
    End Property
End Class

[RobSiklos]

@sharwell Thanks! I never even heard of MyClass; what a weird keyword. Ah well, such is VB.

Question about tests. Should I write a separate set of tests for properties, methods, and events; or just pick one?

Also, I asked this in gitter, but may as well ask here as well. What's the policy on method documentation. I am looking in the relevant source file you mentioned, and none of the members have any <summary> style documentation. If I'm adding a new virtual method, should I follow suit, or is it OK if I document it?

[sharwell]

Thanks! I never even heard of MyClass; what a weird keyword. Ah well, such is VB.

(I hadn't either!) One of the cool things about participating in this project is you get exposure to a variety of languages in a variety of ways. I may have a preference when it comes to writing code, but for the sake of learning and understanding alternatives it's great to get the exposure. 👍

Question about tests. Should I write a separate set of tests for properties, methods, and events; or just pick one?

My approach is typically to hit them all. You wouldn't think it makes too much of a difference but #19373 and #19750 shows how things can and sometimes do slip through the cracks.

I asked this in gitter,

To reach me specifically, here is typically better (with an @sharwell mention). Sometimes I venture out but then there are days like today where one issue took my whole day away. :smile:

If I'm adding a new virtual method, should I follow suit, or is it OK if I document it?

Typically documentation would be considered required in the following places:

1. Public API (you aren't working on this)
2. At the request of someone reviewing it, in an effort to make a specific item smoother for future readers/maintainers

You are free to include additional documentation to the degree you believe it helps you.

[RobSiklos]

@sharwell Ok, I think I'm nearly there. The problem I'm running into now is that I can't seem to make a unit test which fails against the existing code. This is the test I'm running:

[WorkItem(17711, "https://github.com/dotnet/roslyn/issues/17711")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsQualifyMemberAccess)]
public async Task RobTest()
{
    await TestMissingAsyncWithOption(
@"class Base
{
    public virtual int Property { get; }
}
class Derived : Base
{
    public override int Property { get { return base.[|Property|]; } }
}",
CodeStyleOptions.QualifyPropertyAccess);
}

However, even without my fix, this test passes. I think the problem is because although the analyzer returns a problem, the fixer says there are no fixes, so the test framework treats this as success.

How should I proceed?

[sharwell]

I believe you want [|base.|]Property, where the special indicators show where the warning would be.

[RobSiklos]

@sharwell No - the editor highlights base.Property with the warning (at least in VS 2017 15.2). I tried [|base.|]Property, [|base.Property|], and [|base|].Property, and in all cases, the test passed.

From what I can tell, the analyzer detects the problem as expected, but the fixer reports no available fixes (due to AbstractQualifyMemberAccessCodeFixprovider.cs line 38). TestMissingAsync() only returns failure if there are 1 or more fixes (AbstractCodeActionOrUserDiagnosticTest.cs line 89), so the test passes.

[sharwell]

@RobSiklos I'm checking with @CyrusNajmabadi now. It seems like that method should be verifying that no diagnostics or fixes are reported. Seems strange to only cover the latter.

[RobSiklos]

@sharwell I can't figure out why the issue happens for properties, but not for methods. Any hints?

[sharwell]

@RobSiklos My guess is you're hitting #19373. If that's the case, I would suggest one of the following:

1. Add the test, but use [Fact(Skip = "https://github.com/dotnet/roslyn/issues/19373")]
2. Fix the other case as well

I am only expecting the first, but if you happen to stumble across the answer for the issue (it could be trivial to very difficult) I wouldn't stop you from fixing it!

[RobSiklos]

@sharwell Ok - pull request submitted. I can't figure out the cause of #19373, but since it isn't causing my tests to fail, I didn't mark them to be skipped.

Also, entered issue #19807 discovered during testing.