Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RS1024 has no code fix #4539

Closed
weltkante opened this issue Dec 8, 2020 · 9 comments · Fixed by #7296
Closed

RS1024 has no code fix #4539

weltkante opened this issue Dec 8, 2020 · 9 comments · Fixed by #7296
Assignees
@ryzngard
Labels
Area-Microsoft.CodeAnalysis.Analyzers Bug The product is not behaving according to its current intended design
Milestone
vNext

Comments

@weltkante
Copy link

weltkante commented Dec 8, 2020 •
edited

Analyzer

Diagnostic ID: RS1024: Compare symbols correctly

Analyzer source

<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.8.0" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.2" PrivateAssets="all" />

Using current public VS 2019 preview if it matters. The project is a .NET Standard 2.0 project, as this was suggested for code generators I assumed it would also be a good choice for porting analyzers.

Describe the bug

Was porting code from an older analyzer and got strange warnings I did not understand. Came across #3427 where I had to read the whole issue to understand what the warning even means. Part of the discussion was that it was supposed to have code fixes available, which it doesn't, and I was asked to open a separate issue for this.

Steps To Reproduce

Write an anlyzer which compares symbols, either via == or Equals. (Note the repro code is not nullability enabled. Just compare two symbols like any old style analyzer would do.)

public void Test(ISymbol symbol1, ITypeSymbol symbol2)
{
    if (symbol1 == symbol2) // warning RS1024 without offering a code fix
    {
        // do something
    }
}

Expected behavior

According to #3427 a code fix should be offered (making the warning easier to understand is #3427 itself)

Actual behavior

No code fix is offered

/cc @sharwell - opened a separate issue as requested
@Evangelink
Copy link
Member

I can reproduce the issue with this version of the analyzer and a project targeting netstandard2.0. If I start using Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="5.0.1" then I do have the code-fix so I am wondering if it is just something that was introduced in between.

@Evangelink
Copy link
Member

Yep it works when using the pre-release 3.3.1-beta1.20505.3.

@weltkante
Copy link
Author

@Evangelink erm, why close if this regressed? I don't want to downgrade from stable to an unlisted prerelease.

it works when using the pre-release 3.3.1-beta1.20505.3

Sounds like it worked in an earlier version and broke for 3.3.2 ?

If I start using Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="5.0.1" then I do have the code-fix

Is this a replacement package or what is its role? Everyone seems to be still referring to Microsoft.CodeAnalysis.Analyzers including some sample linked by a relatively recent blog post.

@Evangelink
Copy link
Member

@weltkante I am not saying there was no regression, only that between the last released version and the code on master (included in this pre-release) the bug was fixed. The only remaining work is to publish a new version of the package.

Regarding the package, it just depends on what rules you are looking for (see the readme). The package you are referencing is still valid (as far as I know).

@mavasani Do you want to keep this ticket open? Is there any ETA for a new release of Microsoft.CodeAnalysis.Analyzers?

@mavasani
Copy link
Member

Couple of points to note:

  1. Microsoft.CodeAnalysis.NetAnalyzers is unrelated to this issue. It is primarily for CAxxxx rules, and has no effect on any RSxxxx rules or their code fixes
  2. For Microsoft.CodeAnalysis.Analyzers, we recently released version 3.3.2, which should have fixes from 3.3.1. @Evangelink your comment RS1024 has no code fix #4539 (comment) suggests this was fixed in 3.3.1 preview drops. If so, did this regress later and was again fixed post 3.3.2? Regardless, if the issue is fixed in latest 3.3.3 preview bits, we will likely wait for some more time before releasing a stable 3.3.3 to allow for any more important bug fixes to also get clubbed.

@weltkante
Copy link
Author

If this is fixed in master I'm fine leaving this closed, and its not urgent to push the fix to nuget either, just isn't clear from the comments since they cited a preview version which is earlier than the one I'm using.

@Evangelink
Copy link
Member

Evangelink commented Dec 11, 2020 •
edited by mavasani

@weltkante Hey sorry, I just got confused with versions in my head! I think I should just give up coding for today...

So I confirm that the code-fix was offered in 3.3.1 but is no longer offered in 3.3.2. The pre-release link in the readme doesn't provide any 3.3.3 preview, did it changed location?

@mavasani I was mentioning <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="5.0.1"> because it was present in my test project and I do confirm that having only this analyzer I do have access to the RS1024 rules.

I am going to reopen this ticket, we need a little more investigation.

@Evangelink Evangelink reopened this Dec 11, 2020
@mavasani
Copy link
Member

@Evangelink I believe the RS rules are coming to your project indirectly from Microsoft.CodeAnalysis, which in turn has a build-time dependency on Microsoft.CodeAnalysis.Analyzers. Microsoft.CodeAnalysis.NetAnalyzers itself has no relation to RS analyzers or code fixes.

@weltkante weltkante mentioned this issue Jan 5, 2021
Open
@mavasani mavasani added Area-Microsoft.CodeAnalysis.Analyzers Bug The product is not behaving according to its current intended design labels Jan 8, 2021
@mavasani mavasani added this to the vNext milestone Jan 8, 2021
@RussKie
Copy link
Member

RussKie commented Jul 19, 2021

So I confirm that the code-fix was offered in 3.3.1 but is no longer offered in 3.3.2

Ran into this today.

@mavasani mavasani removed the Verified label Nov 9, 2023
@DoctorKrolic DoctorKrolic mentioned this issue Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ryzngard ryzngard

Labels
Area-Microsoft.CodeAnalysis.Analyzers Bug The product is not behaving according to its current intended design
Projects
None yet
Milestone
vNext
Development

Successfully merging a pull request may close this issue.

Avoid direct descriptor comparison, so the fix can be shown DoctorKrolic/roslyn-analyzers
5 participants
@ryzngard @RussKie @weltkante @mavasani @Evangelink