-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add analyzer/fixer to update expressions like Array.Empty<T>()
over to using the []
collection expression.
#69279
Add analyzer/fixer to update expressions like Array.Empty<T>()
over to using the []
collection expression.
#69279
Conversation
@@ -12,33 +12,30 @@ | |||
using Microsoft.CodeAnalysis.LanguageService; | |||
using Microsoft.CodeAnalysis.UseCollectionInitializer; | |||
|
|||
namespace Microsoft.CodeAnalysis.CSharp.UseCollectionInitializer | |||
namespace Microsoft.CodeAnalysis.CSharp.UseCollectionInitializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review with whitespasce off.
@@ -53,7 +54,7 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context) | |||
|
|||
foreach (var diagnostic in diagnostics.OrderByDescending(d => d.Location.SourceSpan.Start)) | |||
{ | |||
var expression = diagnostic.Location.FindNode(getInnermostNodeForTie: true, cancellationToken); | |||
var expression = diagnostic.AdditionalLocations[0].FindNode(getInnermostNodeForTie: true, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a bug before, but was not something you could generally notice as the location was the first token of these nodes and asking for the node would give you teh same thing. but htis is more correct/safer, so i updated.
IsOnSingleLine(sourceText, initializer))); | ||
} | ||
|
||
bool ShouldReplaceExistingExpressionEntirely(ExpressionSyntax explicitOrImplicitArray, InitializerExpressionSyntax initializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is from #69278
@@ -243,7 +243,8 @@ class C | |||
FixedCode = """ | |||
class C | |||
{ | |||
object[] i = [ | |||
object[] i = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is from #69278
using Microsoft.CodeAnalysis.Testing; | ||
using Xunit; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp.Analyzers.UnitTests.UseCollectionExpression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are the new tests.
01a4e2b
to
1534a1a
Compare
@akhera99 ptal :) |
@@ -88,14 +88,14 @@ private static void AnalyzeArrayInitializer(SyntaxNodeAnalysisContext context) | |||
: initializer; | |||
|
|||
if (!UseCollectionExpressionHelpers.CanReplaceWithCollectionExpression( | |||
semanticModel, arrayCreationExpression, cancellationToken)) | |||
semanticModel, arrayCreationExpression, skipVerificationForReplacedNode: false, cancellationToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed as the speculative system currently isn't ok with a node's kind changing this dramatically. specifically from a method invocation to another expression form. I wasn't comfortable mucking with the deep speculatrion logic, so easier to just not compare the bottommost nodes directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄I don't have any context about this, but choose to trust you here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the new analyzer/test part.
Draft while waiting for #69219 to go in.