Implement IdentifiersShouldNotContainUnderscoresFixer #3706

Youssef1313 · 2020-06-02T18:15:29Z

Fixes #444

Youssef1313 · 2020-06-02T18:20:24Z

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

+        private static async Task<Document> RenameWithoutUnderscores(Document document, ISymbol symbol, CancellationToken cancellationToken)
+        {
+            Solution newSolution = await Renamer.RenameSymbolAsync(document.Project.Solution, symbol, symbol.Name.Replace("_", ""), null, cancellationToken).ConfigureAwait(false);
+            return newSolution.GetDocument(document.Id);


This is written just like this.

But I'm getting a null-reference warning:

Why a warning is emitted here but not there? I can't get the difference.

GetDocument can return a null value if the document ID does not belong to the solution. You should add a ! suppression here. We should consider adding GetRequiredXXX extension methods to our repo similar to the ones in Roslyn: https://github.com/dotnet/roslyn/blob/bc2d6a2597d07a574be4e292f9dbd9c8f975fd7f/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/ISolutionExtensions.cs#L36-L68

@mavasani Thanks. I didn't see the ! there 😆

That makes sense now.

Should I add the extension methods in this PR? or leave it as another task?

A small question: why the ISolutionExtensions is made internal in roslyn? I can't see an obvious problem with exposing these extensions to be public.

I think there is possible a pending work item to make these public with API review approval. @CyrusNajmabadi do you know?

Should I add the extension methods in this PR? or leave it as another task?

A separate PR might get in quicker.

I think there is possible a pending work item to make these public with API review approval. @CyrusNajmabadi do you know?

Don't know what we're planning there. It's a pretty awful situation all around. i'd prefer we just break these to throw.

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

Youssef1313 · 2020-06-02T21:00:38Z

@mavasani Any thing left except unit test?

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

codecov · 2020-06-03T11:06:28Z

Codecov Report

Merging #3706 (e7c3d0e) into main (9e5f533) will increase coverage by 0.00%.
The diff coverage is 98.23%.

@@            Coverage Diff            @@
##             main    #3706     +/-   ##
=========================================
  Coverage   95.54%   95.55%             
=========================================
  Files        1185     1205     +20     
  Lines      272465   279213   +6748     
  Branches    16428    17835   +1407     
=========================================
+ Hits       260338   266794   +6456     
- Misses      10001    10256    +255     
- Partials     2126     2163     +37

Youssef1313 · 2020-06-03T11:36:15Z

I think there is something wrong in the way I wrote the test causing it to fail.

@mavasani, Can you check please?

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

mavasani · 2020-06-03T12:34:38Z

I think there is something wrong in the way I wrote the test causing it to fail.

CA1707 analyzer uses multiple descriptors with same rule ID (see here), but different messages. So the test framework cannot implicitly deduce which descriptor/message is expected from markup. You need to explicitly provide which descriptor to use, see an example test below:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Usage/ProvideCorrectArgumentToEnumHasFlagTests.cs

Lines 17 to 36 in 03d8e7d

    
                   [Fact] 
        
                   public async Task CA2248_EnumTypesAreDifferent_Diagnostic() 
        
                   { 
        
                       await VerifyCS.VerifyAnalyzerAsync(@" 
        
           using System; 
        
           public class C 
        
           { 
        
               [Flags] 
        
               public enum MyEnum { A, B, } 
        
               [Flags] 
        
               public enum OtherEnum { A, } 
        
               public void Method(MyEnum m) 
        
               { 
        
                   {|#0:m.HasFlag(OtherEnum.A)|}; 
        
               } 
        
           }", 
        
                           VerifyCS.Diagnostic(ProvideCorrectArgumentToEnumHasFlag.DifferentTypeRule).WithLocation(0).WithArguments("OtherEnum", "MyEnum"));

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx

Youssef1313 · 2020-06-03T13:59:03Z

@Evangelink Thanks for your review. 💯

mavasani · 2020-06-06T03:32:09Z

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

+
+        private static async Task<Solution> RenameWithoutUnderscoresAsync(Document document, ISymbol symbol, string newName, CancellationToken cancellationToken)
+        {
+            return await Renamer.RenameSymbolAsync(document.Project.Solution, symbol, newName, null, cancellationToken).ConfigureAwait(false);


Can we just inline this call and avoid an await?

mavasani

Overall looks good. Please add some more tests to cover all scenarios. Thanks!

Youssef1313 · 2020-06-06T11:15:09Z

@mavasani, I've added an equivalent VB test to the test I previously added.
But have no idea about what kind of more cases you'd like to see?

edit:

A two possible cases that came to my mind and will likely fail with the current approach are:

If the name contains only underscores, the new name will become just an empty string.
If the name starts with an underscore then followed by a digit. By removing the underscore the name will start with a digit which shouldn't be a valid name.

Are these the kind of cases you'd like to see?

Youssef1313 · 2020-06-08T10:33:51Z

The current test failure is due to the identifier being a reserved word after renaming. My_class is renaming to MyClass, which is invalid due to MyClass being a VB keyword.

/cc @Evangelink Mentioning as your PR (#3352) was based on renaming too. Did you handle such case?

Evangelink · 2020-06-08T20:03:14Z

@Youssef1313 I am not handling this case so I have the same issue as you do. I would have expected the symbol renamer to handle this case. I can see a couple of implementations to workaround that but let's wait for @mavasani's feedback.

Youssef1313 · 2020-06-30T08:29:41Z

Sorry for the delay on this one. I've updated it locally to put @newName (C#) or [newName] (VB) when the new name becomes invalid (because it's a keyword). But the test fails (it doesn't enter the fixer at all) with the following error:

The markup location '#0' was not found in the input.

However, I see that the input has {|#0:My_class|}, not sure why it's not found.

...eQuality.Analyzers/ApiDesignGuidelines/CSharpIdentifiersShouldNotContainUnderscores.Fixer.cs

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

Evangelink · 2020-08-09T17:57:52Z

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

+public enum DoesNotMatterEnum
+{
+    {|#2:_EnumWithUnderscore|},
+    {|#3:_|}


I would probably not report on this case.

I thought about that at first, but preferred to keep the warning in this case without a code fix.

But after a second thought, it would probably be okay if we don't report on this case. It's really a very edge case. Almost no one will write something like that 😄

@mavasani, would you be okay if we don't report on this case?

Yes, I don't really care if we report a diagnostic on _ or not.

On the second thought I also think reporting or not are both valid options.

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

...eQuality.Analyzers/ApiDesignGuidelines/CSharpIdentifiersShouldNotContainUnderscores.Fixer.cs

Evangelink · 2020-08-09T18:20:45Z

@Youssef1313 Let me know if I haven't provided enough info for you to move forward on this PR.

Evangelink · 2020-08-09T19:45:08Z

@Youssef1313 Do you mind if I push a commit on your PR?

Youssef1313 · 2020-08-09T19:49:45Z

@Evangelink You can push anytime :)

Does that require giving you write access on my fork?

Evangelink · 2020-08-09T20:01:17Z

@Youssef1313 Don't bother! I thought I found something to unlock but it's not the case...

To sum-up my local changes:

Update the few broken tests because of the rename not renaming the override/implementations. It'd be nice to create a ticket (I couldn't find any I was supposed to have created).
Create a new method to retrieve the declaration node (for example protected abstract SyntaxNode GetDeclarationNode(SyntaxNode node);), the implementation is rather simple.

C#:

protected override SyntaxNode GetDeclarationNode(SyntaxNode node)
    => node.IsKind(SyntaxKind.IdentifierName)
        ? node.Parent
        : node;

VB.NET:

Protected Overrides Function GetDeclarationNode(node As SyntaxNode) As SyntaxNode
    If node.IsKind(SyntaxKind.IdentifierName) Then
        Return node.Parent
    Else
        Return node
    End If
End Function

Update the field test (CA1707_ForFields_CSharp) as explained here Implement IdentifiersShouldNotContainUnderscoresFixer #3706 (comment)

Youssef1313 · 2020-08-10T07:23:15Z

@Evangelink Thanks for the great help you provided here. I opened an issue in dotnet/roslyn for the problem with RenameSymbolAsync. I'll do your suggestions very soon and push.

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

Youssef1313 · 2020-08-10T15:52:57Z

What should the expected behavior be for a case like the following (which is far away from a real scenario)?

public class My_Class { }
public class My__Class { }

@Evangelink @mavasani

Another case that comes to mind also:

public class Base
{
    public void My_Method() { }
}
public class Derived : Base
{
    new public void My_Method() { }
}

For the above case, if the user chose to fix all, then it's simple.

But if the user chose to fix in only one of them, the "new" keyword should be removed. (I'm not aware of how this would be implemented, nor how it would be tested)

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

Youssef1313 · 2020-08-11T17:37:12Z

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

 Public Class ImplementI1
    Implements I1
    ' No diagnostic
    Public Event E_ As System.EventHandler Implements I1.E_
-    Public Event E2_ As System.EventHandler
+    Public Event {|#3:E2_|} As System.EventHandler
 End Class

 Public Class Derives
    Inherits ImplementI1
+
+    Public Shadows Event {|#4:E2_|} As System.EventHandler


When renaming E2_ in Derives class, the new name "E2" is detected as a clashing, most probably because E2 already exists inImplementI1. However, we indeed want to rename it. It's marked with Shadows and is meant to shadow the member in ImplementI1.

My opinion is that the analyzer don't report for the "Shadows" case, and RenameSymbolAsync gets it updated when dotnet/roslyn#46663 is fixed.

@mavasani @Evangelink What are your opinions?

It seems that there is a compiler warning when there is a member shadowing a non-existing member so I think that's fine if the code-fix misses renaming this case.

Tagging @mavasani for confirmation.

Yes, I am fine to keep it as-is for now to reduce the scope of this PR. If required, this can be tackled in a follow-up issue/PR.

mavasani · 2020-10-09T20:22:30Z

@Youssef1313 are you able to fix the tests here? Anything else remaining for this PR?

Evangelink · 2020-10-12T10:27:45Z

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs

@@ -19,7 +19,7 @@ public class IdentifiersShouldNotContainUnderscoresTests
    {
        #region CSharp Tests
        [Fact]
-        public async Task CA1707_ForAssembly_CSharp()
+        public async Task CA1707_ForAssembly_CSharp() // TODO: How to test the code fixer for this?


Shall we skip the codefix for the assembly level then?

...eQuality.Analyzers/ApiDesignGuidelines/CSharpIdentifiersShouldNotContainUnderscores.Fixer.cs

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs

src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md

Co-authored-by: Amaury Levé <[email protected]>

Evangelink · 2021-03-03T18:18:42Z

@Youssef1313 Could you fix this little conflict and the few open comments?

mavasani · 2021-05-20T17:07:27Z

/azp run

azure-pipelines · 2021-05-20T17:07:38Z

Azure Pipelines successfully started running 1 pipeline(s).

mavasani · 2021-05-21T04:45:55Z

Thanks @Youssef1313 and sorry for the delay in getting this merged.

Youssef1313 · 2021-05-21T04:48:07Z

Thanks @Youssef1313 and sorry for the delay in getting this merged.

No problem.

Youssef1313 requested a review from a team as a code owner June 2, 2020 18:15

Youssef1313 commented Jun 2, 2020

View reviewed changes

mavasani reviewed Jun 2, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jun 2, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Show resolved Hide resolved

mavasani reviewed Jun 2, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jun 2, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jun 2, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Show resolved Hide resolved

mavasani reviewed Jun 3, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jun 3, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jun 3, 2020

View reviewed changes

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs Outdated Show resolved Hide resolved

Evangelink reviewed Jun 3, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Show resolved Hide resolved

Evangelink reviewed Jun 3, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

Evangelink reviewed Jun 3, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

Evangelink reviewed Jun 3, 2020

View reviewed changes

...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved

Youssef1313 mentioned this pull request Jun 4, 2020

RS0046: Refactor call to 'LookupSymbols' to avoid name clash in arguments #3707

Open

mavasani reviewed Jun 6, 2020

View reviewed changes

mavasani reviewed Jul 6, 2020

View reviewed changes

...eQuality.Analyzers/ApiDesignGuidelines/CSharpIdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jul 6, 2020

View reviewed changes

...eQuality.Analyzers/ApiDesignGuidelines/CSharpIdentifiersShouldNotContainUnderscores.Fixer.cs Show resolved Hide resolved

mavasani reviewed Jul 6, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

mavasani reviewed Jul 6, 2020

View reviewed changes

...ft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscores.Fixer.cs Outdated Show resolved Hide resolved

Evangelink reviewed Aug 9, 2020

View reviewed changes

Youssef1313 mentioned this pull request Aug 10, 2020

RenameSymbolAsync doesn't rename the symbol in the implementing class dotnet/roslyn#46663

Open

mavasani reviewed Aug 10, 2020

View reviewed changes

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs Show resolved Hide resolved

Youssef1313 mentioned this pull request Aug 10, 2020

Extend CA1707 rule #3984

Closed

Youssef1313 commented Aug 10, 2020

View reviewed changes

...oft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldNotContainUnderscoresTests.cs Outdated Show resolved Hide resolved

Youssef1313 commented Aug 11, 2020

View reviewed changes

Youssef1313 added 2 commits December 20, 2020 19:28

Implement IdentifiersShouldNotContainUnderscoresFixer

27962a6

Update auto-generated files

3d4966e

Youssef1313 force-pushed the ca1707 branch from 191b447 to 3d4966e Compare December 20, 2020 17:44

Evangelink reviewed Dec 21, 2020

View reviewed changes

Apply suggestions from code review

7b559cd

Co-authored-by: Amaury Levé <[email protected]>

Base automatically changed from master to main March 5, 2021 02:20

Merge branch 'main' into ca1707

e7c3d0e

Evangelink requested a review from mavasani March 7, 2021 15:33

mavasani self-assigned this May 20, 2021

mavasani approved these changes May 21, 2021

View reviewed changes

mavasani merged commit e8cd97a into dotnet:main May 21, 2021

Youssef1313 deleted the ca1707 branch May 21, 2021 04:47

Implement IdentifiersShouldNotContainUnderscoresFixer #3706

Implement IdentifiersShouldNotContainUnderscoresFixer #3706

Conversation

Youssef1313 commented Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Jun 2, 2020

codecov bot commented Jun 3, 2020 • edited Loading

Codecov Report

Youssef1313 commented Jun 3, 2020

mavasani commented Jun 3, 2020

Youssef1313 commented Jun 3, 2020

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

Youssef1313 commented Jun 6, 2020 • edited Loading

Youssef1313 commented Jun 8, 2020

Evangelink commented Jun 8, 2020

Youssef1313 commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evangelink commented Aug 9, 2020

Evangelink commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020 • edited Loading

Evangelink commented Aug 9, 2020

Youssef1313 commented Aug 10, 2020

Youssef1313 commented Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Oct 9, 2020

Choose a reason for hiding this comment

Evangelink commented Mar 3, 2021

mavasani commented May 20, 2021

azure-pipelines bot commented May 20, 2021

mavasani commented May 21, 2021

Youssef1313 commented May 21, 2021

Youssef1313 commented Jun 2, 2020 •

edited

Loading

codecov bot commented Jun 3, 2020 •

edited

Loading

Youssef1313 commented Jun 6, 2020 •

edited

Loading

Youssef1313 commented Aug 9, 2020 •

edited

Loading

Youssef1313 commented Aug 10, 2020 •

edited

Loading