Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException) #1880

NikolayIT · 2015-12-01T17:39:21Z

codecov-io · 2015-12-01T17:45:08Z

Current coverage is `79.44%`

Merging #1880 into stabilization will increase coverage by +0.03% as of 127039e

@@            stabilization   #1880   diff @@
=============================================
  Files                 558     558       
  Stmts               34807   34824    +17
  Branches             9637    9644     +7
  Methods                 0               
=============================================
+ Hit                 27642   27665    +23
- Partial              5624    5631     +7
+ Missed               1541    1528    -13

Review entire Coverage Diff as of 127039e

Powered by Codecov. Updated on successful CI builds.

sharwell · 2015-12-01T18:00:08Z

StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/ReadabilityRules/SA1133CodeFixProvider.cs

@@ -51,8 +51,18 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context)
        private static async Task<Document> GetTransformedDocumentAsync(Document document, Diagnostic diagnostic, CancellationToken cancellationToken)
        {
            var syntaxRoot = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
-            var violatingAttribute = (AttributeSyntax)syntaxRoot.FindNode(diagnostic.Location.SourceSpan).Parent;
-            var attributeList = (AttributeListSyntax)violatingAttribute.Parent;
+            var nodeInSourceSpan = syntaxRoot.FindNode(diagnostic.Location.SourceSpan);


We've run into problems in the past where error nodes in the tree caused FindNode to return unexpected nodes. Normally this is solved by setting getInnermostNodeForTie: true, but I'm not sure that would behave properly here. We probably want to have a third case handled where nodeInSourceSpan.Parent is neither an AttributeListSyntax nor an AttributeSyntax, and just return.

❗ A better solution would be to use getInnermostNodeForTie: true and then call FirstAncestorOrSelf<AttributeListSyntax>() on the found node. That call should never fail, as the analyzer works on AttributeSyntaxList.

NikolayIT · 2015-12-01T18:15:04Z

@sharwell added default case which just returns the document object

Noryoko · 2015-12-01T19:17:38Z

Could you add these tests?

[Foo]
[Bar, Car]

[Foo, Bar]
[Car]

[Foo]
[Bar, Car]
[Ear]

~~[Foo, Bar, Car]~~ Already covered

NikolayIT · 2015-12-01T20:13:38Z

@Noryoko unit tests added.

NikolayIT · 2015-12-01T20:28:45Z

@Noryoko, @vweijsters, do you think the following tests should also pass?

[InlineData("[Foo][Bar, Car]", "[Foo]\r\n[Bar]\r\n[Car]", 2, 12)]
[InlineData("[Foo, Bar][Car]", "[Foo]\r\n[Bar]\r\n[Car]", 2, 7)]
[InlineData("[Foo][Bar, Car][Ear]", "[Foo]\r\n[Bar]\r\n[Car]\r\n[Ear]", 2, 12)]

With the current code they are failing with:

Xunit.Sdk.EqualException

Assert.Equal() Failure
                                     ↓ (pos 27)
Expected: ···ystem;[Foo][Bar][Car]public class TestClass{}···
Actual:   ···ystem;[Foo][Bar][Car]public class TestClass{}pu···
                                     ↑ (pos 27)

Here you can find the test code: https://github.com/NikolayIT/StyleCopAnalyzers/commit/74bef9273a0a148b7a2e1a8fb32db9e5529b2ed9#diff-9e4ac25db4b34d81902b3ff7fedf0e6cR176

Noryoko · 2015-12-01T20:56:03Z

I think so. The fix is already splitting the attributes onto their own lines. Would make the end result look consistent.

vweijsters · 2015-12-01T20:59:58Z

I don't think they should pass, as they are partly in the domain of SA1134. The SA1133 code fix adds the new lines to make sure that it doesn't immediately introduces a SA1134 after code fixing, but it will not / should not attempt to fix any preexisting SA1134 cases.

NikolayIT · 2015-12-01T21:02:06Z

OK, then this pull request is ready for approval.

Noryoko · 2015-12-01T21:03:42Z

I'll change my vote to agree with @vweijsters.

vweijsters · 2015-12-01T21:05:39Z

@NikolayIT I would like to see my remark on SA1133CodeFixProvider.cs to be addressed before I'm ok with this pull request.

NikolayIT · 2015-12-01T21:13:25Z

@vweijsters is that what you meant?

vweijsters · 2015-12-01T21:14:58Z

No, I meant:

❗ A better solution would be to use getInnermostNodeForTie: true and then call FirstAncestorOrSelf() on the found node. That call should never fail, as the analyzer works on AttributeSyntaxList.

…storOrSelf() on the found node

NikolayIT · 2015-12-01T21:22:22Z

Sorry, didn't see it. Is now OK? @vweijsters

vweijsters · 2015-12-01T21:25:59Z

Totally 😉

👍 Looks good to me

Noryoko · 2015-12-01T21:27:45Z

👍 Looks good to me

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException)

vweijsters · 2015-12-02T07:52:52Z

@sharwell Could you merge this into master as well?

sharwell · 2015-12-02T15:13:20Z

@vweijsters yes I'll merge it tonight

NikolayIT added 2 commits December 1, 2015 19:18

Added repro test for #1878

f25bde0

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException)

3332660

sharwell added the code review label Dec 1, 2015

sharwell added this to the 1.0.0 RC 2 milestone Dec 1, 2015

sharwell reviewed Dec 1, 2015
View reviewed changes

Implement CR feedback

fcf292e

CR: Added additional unit tests for SA1133

74bef92

CR: Added remark for the new lines in SA1133CodeFixProvider

67983c5

CR: In SA1133 use getInnermostNodeForTie:true and then call FirstAnce…

972c393

…storOrSelf() on the found node

sharwell removed the code review label Dec 2, 2015

sharwell assigned vweijsters Dec 2, 2015

sharwell added a commit that referenced this pull request Dec 2, 2015

Merge pull request #1880 from NikolayIT/Fix1878

73afee8

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException)

sharwell merged commit 73afee8 into DotNetAnalyzers:stabilization Dec 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException) #1880

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException) #1880

NikolayIT commented Dec 1, 2015

codecov-io commented Dec 1, 2015

sharwell Dec 1, 2015

vweijsters Dec 1, 2015

NikolayIT commented Dec 1, 2015

Noryoko commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

Noryoko commented Dec 1, 2015

vweijsters commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

Noryoko commented Dec 1, 2015

vweijsters commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

vweijsters commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

vweijsters commented Dec 1, 2015

Noryoko commented Dec 1, 2015

vweijsters commented Dec 2, 2015

sharwell commented Dec 2, 2015

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException) #1880

Fix #1878 (SA1133CodeFixProvider crash with System.InvalidCastException) #1880

Conversation

NikolayIT commented Dec 1, 2015

codecov-io commented Dec 1, 2015

Current coverage is 79.44%

sharwell Dec 1, 2015

Choose a reason for hiding this comment

vweijsters Dec 1, 2015

Choose a reason for hiding this comment

NikolayIT commented Dec 1, 2015

Noryoko commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

Noryoko commented Dec 1, 2015

vweijsters commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

Noryoko commented Dec 1, 2015

vweijsters commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

vweijsters commented Dec 1, 2015

NikolayIT commented Dec 1, 2015

vweijsters commented Dec 1, 2015

Noryoko commented Dec 1, 2015

vweijsters commented Dec 2, 2015

sharwell commented Dec 2, 2015

Current coverage is `79.44%`