From 5677d8a60e3661f4ca610508fe5ecde981303557 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 20 Jan 2025 13:38:42 -0800 Subject: [PATCH 1/3] Special case inlining a collection expr into a spreaded element --- ...deRefactoringProvider.ReferenceRewriter.cs | 248 ++++++++++-------- .../InlineTemporaryCodeRefactoringProvider.cs | 73 +++++- .../InlineTemporary/InlineTemporaryTests.cs | 38 +++ 3 files changed, 241 insertions(+), 118 deletions(-) diff --git a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs index db4d31e5d894b..05d1b8a6822d1 100644 --- a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs +++ b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs @@ -1,116 +1,132 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; -using System.Linq; -using System.Threading; -using Microsoft.CodeAnalysis.CSharp.Syntax; - -namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary; - -internal sealed partial class CSharpInlineTemporaryCodeRefactoringProvider -{ - private sealed class ReferenceRewriter : CSharpSyntaxRewriter - { - private readonly ISet _conflictReferences; - private readonly ISet _nonConflictReferences; - private readonly ExpressionSyntax _expressionToInline; - private readonly CancellationToken _cancellationToken; - - private ReferenceRewriter( - ISet conflictReferences, - ISet nonConflictReferences, - ExpressionSyntax expressionToInline, - CancellationToken cancellationToken) - { - _conflictReferences = conflictReferences; - _nonConflictReferences = nonConflictReferences; - _expressionToInline = expressionToInline; - _cancellationToken = cancellationToken; - } - - private ExpressionSyntax UpdateIdentifier(IdentifierNameSyntax node) - { - _cancellationToken.ThrowIfCancellationRequested(); - - if (_conflictReferences.Contains(node)) - return node.Update(node.Identifier.WithAdditionalAnnotations(CreateConflictAnnotation())); - - if (_nonConflictReferences.Contains(node)) - return _expressionToInline; - - return node; - } - - public override SyntaxNode? VisitIdentifierName(IdentifierNameSyntax node) - { - var result = UpdateIdentifier(node); - return result == _expressionToInline - ? result.WithTriviaFrom(node) - : result; - } - - public override SyntaxNode? VisitAnonymousObjectMemberDeclarator(AnonymousObjectMemberDeclaratorSyntax node) - { - if (node.NameEquals == null && - node.Expression is IdentifierNameSyntax identifier && - _nonConflictReferences.Contains(identifier)) - { - - // Special case inlining into anonymous types to ensure that we keep property names: - // - // E.g. - // int x = 42; - // var a = new { x; }; - // - // Should become: - // var a = new { x = 42; }; - return node.Update( - SyntaxFactory.NameEquals(identifier), UpdateIdentifier(identifier)).WithTriviaFrom(node); - } - - return base.VisitAnonymousObjectMemberDeclarator(node); - } - - public override SyntaxNode? VisitArgument(ArgumentSyntax node) - { - if (node.Parent is TupleExpressionSyntax tupleExpression && - ShouldAddTupleMemberName(node, out var identifier) && - tupleExpression.Arguments.Count(a => ShouldAddTupleMemberName(a, out _)) == 1) - { - return node.Update( - SyntaxFactory.NameColon(identifier), node.RefKindKeyword, UpdateIdentifier(identifier)).WithTriviaFrom(node); - } - - return base.VisitArgument(node); - } - - private bool ShouldAddTupleMemberName(ArgumentSyntax node, [NotNullWhen(true)] out IdentifierNameSyntax? identifier) - { - if (node.NameColon == null && - node.Expression is IdentifierNameSyntax id && - _nonConflictReferences.Contains(id) && - !SyntaxFacts.IsReservedTupleElementName(id.Identifier.ValueText)) - { - identifier = id; - return true; - } - - identifier = null; - return false; - } - - public static SyntaxNode Visit( - SyntaxNode scope, - ISet conflictReferences, - ISet nonConflictReferences, - ExpressionSyntax expressionToInline, - CancellationToken cancellationToken) - { - var rewriter = new ReferenceRewriter(conflictReferences, nonConflictReferences, expressionToInline, cancellationToken); - return rewriter.Visit(scope); - } - } -} +//// Licensed to the .NET Foundation under one or more agreements. +//// The .NET Foundation licenses this file to you under the MIT license. +//// See the LICENSE file in the project root for more information. + +//using System.Collections.Generic; +//using System.Diagnostics.CodeAnalysis; +//using System.Linq; +//using System.Threading; +//using Microsoft.CodeAnalysis.CSharp.Syntax; +//using Microsoft.CodeAnalysis.Shared.Extensions; +//using Microsoft.CodeAnalysis.Shared; +//using Microsoft.CodeAnalysis.CSharp.Extensions; + +//namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary; + +//internal sealed partial class CSharpInlineTemporaryCodeRefactoringProvider +//{ +// private sealed class ReferenceRewriter( +// ISet conflictReferences, +// ISet nonConflictReferences, +// ExpressionSyntax expressionToInline, +// ExpressionSyntax originalDeclaratorExpression, +// CancellationToken cancellationToken) : CSharpSyntaxRewriter +// { +// private readonly ISet _conflictReferences = conflictReferences; +// private readonly ISet _nonConflictReferences = nonConflictReferences; +// private readonly ExpressionSyntax _expressionToInline = expressionToInline; +// private readonly ExpressionSyntax _originalDeclaratorExpression = originalDeclaratorExpression; +// private readonly CancellationToken _cancellationToken = cancellationToken; + +// private ExpressionSyntax UpdateIdentifier(IdentifierNameSyntax node) +// { +// _cancellationToken.ThrowIfCancellationRequested(); + +// if (_conflictReferences.Contains(node)) +// return node.Update(node.Identifier.WithAdditionalAnnotations(CreateConflictAnnotation())); + +// if (_nonConflictReferences.Contains(node)) +// return _expressionToInline; + +// return node; +// } + +// private bool ShouldSpreadCollectionIntoCollection( +// ExpressionSyntax expressions, +// [NotNullWhen(true)] out CollectionExpressionSyntax? collectionToInline) +// { +// if (expressions is not IdentifierNameSyntax identifier || +// !_nonConflictReferences.Contains(identifier) || +// expressions.Parent is not SpreadElementSyntax) +// { +// collectionToInline = null; +// return false; +// } + +// collectionToInline = _originalDeclaratorExpression as CollectionExpressionSyntax; +// return collectionToInline != null; +// } + +// public override SyntaxNode? VisitIdentifierName(IdentifierNameSyntax node) +// { +// // Special case inlining a collection into a spread element. We can just move the original elements into +// // the spreaded location. +// if (ShouldSpreadCollectionIntoCollection(node, out _)) +// return node; + +// var result = UpdateIdentifier(node); +// return result == _expressionToInline +// ? result.WithTriviaFrom(node) +// : result; +// } + +// public override SyntaxNode? VisitAnonymousObjectMemberDeclarator(AnonymousObjectMemberDeclaratorSyntax node) +// { +// if (node.NameEquals == null && +// node.Expression is IdentifierNameSyntax identifier && +// _nonConflictReferences.Contains(identifier)) +// { + +// } + +// return base.VisitAnonymousObjectMemberDeclarator(node); +// } + +// public override SyntaxNode? VisitArgument(ArgumentSyntax node) +// { +// if (node.Parent is TupleExpressionSyntax tupleExpression && +// ShouldAddTupleMemberName(node, out var identifier) && +// tupleExpression.Arguments.Count(a => ShouldAddTupleMemberName(a, out _)) == 1) +// { +// return node.Update( +// SyntaxFactory.NameColon(identifier), node.RefKindKeyword, UpdateIdentifier(identifier)).WithTriviaFrom(node); +// } + +// return base.VisitArgument(node); +// } + +// private bool ShouldAddTupleMemberName(ArgumentSyntax node, [NotNullWhen(true)] out IdentifierNameSyntax? identifier) +// { +// if (node.NameColon == null && +// node.Expression is IdentifierNameSyntax id && +// _nonConflictReferences.Contains(id) && +// !SyntaxFacts.IsReservedTupleElementName(id.Identifier.ValueText)) +// { +// identifier = id; +// return true; +// } + +// identifier = null; +// return false; +// } + +// public override SyntaxNode? VisitCollectionExpression(CollectionExpressionSyntax node) +// { + + +// node.Update(VisitToken(node.OpenBracketToken), VisitList(node.Elements), VisitToken(node.CloseBracketToken)) +// } + +// public override SyntaxNode? VisitSpreadElement(SpreadElementSyntax node) +// { +// if (!ShouldSpreadCollectionIntoCollection(node.Expression, out var collectionToInline)) +// return node; + +// // inlining an existing `[...]` collection into a `..` spread element. We can just move the original +// // elements into the final location. +// var leadingTrivia = node.GetLeadingTrivia() is [.., (kind: SyntaxKind.WhitespaceTrivia) lastWhitespace] +// ? lastWhitespace +// : default; +// } +// } +//} diff --git a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs index df419eeb3864d..1b6c68ac68f74 100644 --- a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs +++ b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs @@ -16,6 +16,7 @@ using Microsoft.CodeAnalysis.CodeRefactorings; using Microsoft.CodeAnalysis.CSharp.Extensions; using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.InlineTemporary; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -24,6 +25,8 @@ namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary; +using static SyntaxFactory; + [ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = PredefinedCodeRefactoringProviderNames.InlineTemporary), Shared] [method: ImportingConstructor] [method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] @@ -159,11 +162,11 @@ private static async Task InlineTemporaryAsync(Document document, Vari // Checks to see if inlining the temporary variable may change the code's meaning. This can only apply if the variable has two or more // references. We later use this heuristic to determine whether or not to display a warning message to the user. - var mayContainSideEffects = allReferences.Count() > 1 && + var mayContainSideEffects = allReferences.Length > 1 && MayContainSideEffects(declarator.Initializer.Value); var scope = GetScope(declarator); - var newScope = ReferenceRewriter.Visit(scope, conflictReferences, nonConflictReferences, expressionToInline, cancellationToken); + var newScope = RewriteScope(scope); document = await document.ReplaceNodeAsync(scope, newScope, cancellationToken).ConfigureAwait(false); @@ -209,6 +212,72 @@ private static async Task InlineTemporaryAsync(Document document, Vari }, cancellationToken).ConfigureAwait(false); return document; + + SyntaxNode RewriteScope(SyntaxNode scope) + { + var editor = new SyntaxEditor(scope, document.Project.Solution.Services); + + foreach (var identifier in conflictReferences) + editor.ReplaceNode(identifier, identifier.WithAdditionalAnnotations(CreateConflictAnnotation())); + + foreach (var identifier in nonConflictReferences) + { + if (identifier.Parent is AnonymousObjectMemberDeclaratorSyntax { NameEquals: null } anonymousMember) + { + // Special case inlining into anonymous types to ensure that we keep property names: + // + // E.g. + // int x = 42; + // var a = new { x; }; + // + // Should become: + // var a = new { x = 42; }; + editor.ReplaceNode( + anonymousMember, + anonymousMember.Update(NameEquals(identifier), expressionToInline).WithTriviaFrom(anonymousMember)); + } + else if (identifier.Parent is ArgumentSyntax + { + Parent: TupleExpressionSyntax tupleExpression, + NameColon: null, + } argument && + !SyntaxFacts.IsReservedTupleElementName(identifier.Identifier.ValueText) && + tupleExpression.Arguments.Count(a => nonConflictReferences.Contains(a.Expression)) == 1) + { + // Special case inlining into tuples to ensure that we keep property names: + // + // E.g. + // int x = 42; + // var a = (x, y); + // + // Should become: + // var a = (x: 42, y); + editor.ReplaceNode( + argument, + argument.Update(NameColon(identifier), argument.RefKindKeyword, expressionToInline).WithTriviaFrom(argument)); + } + else if (identifier.Parent is SpreadElementSyntax spreadElement && + declarator.Initializer.Value is CollectionExpressionSyntax collectionToInline) + { + // Special case inlining a collection into a spread element. We can just move the original elements + // into the final collection. + var leadingTrivia = spreadElement.GetLeadingTrivia() is [.., (kind: SyntaxKind.WhitespaceTrivia) space] + ? TriviaList(ElasticMarker, space) + : TriviaList(ElasticMarker); + + editor.ReplaceNode( + spreadElement, + (_, _) => collectionToInline.Elements.Select( + e => e.WithLeadingTrivia(leadingTrivia).WithoutTrailingTrivia())); + } + else + { + editor.ReplaceNode(identifier, expressionToInline.WithTriviaFrom(identifier)); + } + } + + return editor.GetChangedRoot(); + } } private static bool MayContainSideEffects(SyntaxNode expression) diff --git a/src/Features/CSharpTest/InlineTemporary/InlineTemporaryTests.cs b/src/Features/CSharpTest/InlineTemporary/InlineTemporaryTests.cs index 33b14d1cc7e88..cce147aef8d77 100644 --- a/src/Features/CSharpTest/InlineTemporary/InlineTemporaryTests.cs +++ b/src/Features/CSharpTest/InlineTemporary/InlineTemporaryTests.cs @@ -5911,4 +5911,42 @@ void M(string[] args) } """); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/73148")] + public async Task InlineCollectionIntoSpread() + { + await TestInRegularAndScriptAsync( + """ + using System; + class A + { + void M(string[] args) + { + string[] [||]eStaticSymbols = [ + "System.Int32 E.StaticProperty { get; }", + "event System.Action E.StaticEvent", + .. args]; + + string[] allStaticSymbols = [ + .. eStaticSymbols, + "System.Int32 E2.StaticProperty { get; }"]; + } + } + """, + """ + using System; + class A + { + void M(string[] args) + { + + string[] allStaticSymbols = [ + "System.Int32 E.StaticProperty { get; }", + "event System.Action E.StaticEvent", + .. args, + "System.Int32 E2.StaticProperty { get; }"]; + } + } + """); + } } From d5ef52a5cbbdeba1b76a037485c01d7a91fd1753 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 20 Jan 2025 13:39:16 -0800 Subject: [PATCH 2/3] Delete --- ...deRefactoringProvider.ReferenceRewriter.cs | 132 ------------------ 1 file changed, 132 deletions(-) delete mode 100644 src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs diff --git a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs deleted file mode 100644 index 05d1b8a6822d1..0000000000000 --- a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.ReferenceRewriter.cs +++ /dev/null @@ -1,132 +0,0 @@ -//// Licensed to the .NET Foundation under one or more agreements. -//// The .NET Foundation licenses this file to you under the MIT license. -//// See the LICENSE file in the project root for more information. - -//using System.Collections.Generic; -//using System.Diagnostics.CodeAnalysis; -//using System.Linq; -//using System.Threading; -//using Microsoft.CodeAnalysis.CSharp.Syntax; -//using Microsoft.CodeAnalysis.Shared.Extensions; -//using Microsoft.CodeAnalysis.Shared; -//using Microsoft.CodeAnalysis.CSharp.Extensions; - -//namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary; - -//internal sealed partial class CSharpInlineTemporaryCodeRefactoringProvider -//{ -// private sealed class ReferenceRewriter( -// ISet conflictReferences, -// ISet nonConflictReferences, -// ExpressionSyntax expressionToInline, -// ExpressionSyntax originalDeclaratorExpression, -// CancellationToken cancellationToken) : CSharpSyntaxRewriter -// { -// private readonly ISet _conflictReferences = conflictReferences; -// private readonly ISet _nonConflictReferences = nonConflictReferences; -// private readonly ExpressionSyntax _expressionToInline = expressionToInline; -// private readonly ExpressionSyntax _originalDeclaratorExpression = originalDeclaratorExpression; -// private readonly CancellationToken _cancellationToken = cancellationToken; - -// private ExpressionSyntax UpdateIdentifier(IdentifierNameSyntax node) -// { -// _cancellationToken.ThrowIfCancellationRequested(); - -// if (_conflictReferences.Contains(node)) -// return node.Update(node.Identifier.WithAdditionalAnnotations(CreateConflictAnnotation())); - -// if (_nonConflictReferences.Contains(node)) -// return _expressionToInline; - -// return node; -// } - -// private bool ShouldSpreadCollectionIntoCollection( -// ExpressionSyntax expressions, -// [NotNullWhen(true)] out CollectionExpressionSyntax? collectionToInline) -// { -// if (expressions is not IdentifierNameSyntax identifier || -// !_nonConflictReferences.Contains(identifier) || -// expressions.Parent is not SpreadElementSyntax) -// { -// collectionToInline = null; -// return false; -// } - -// collectionToInline = _originalDeclaratorExpression as CollectionExpressionSyntax; -// return collectionToInline != null; -// } - -// public override SyntaxNode? VisitIdentifierName(IdentifierNameSyntax node) -// { -// // Special case inlining a collection into a spread element. We can just move the original elements into -// // the spreaded location. -// if (ShouldSpreadCollectionIntoCollection(node, out _)) -// return node; - -// var result = UpdateIdentifier(node); -// return result == _expressionToInline -// ? result.WithTriviaFrom(node) -// : result; -// } - -// public override SyntaxNode? VisitAnonymousObjectMemberDeclarator(AnonymousObjectMemberDeclaratorSyntax node) -// { -// if (node.NameEquals == null && -// node.Expression is IdentifierNameSyntax identifier && -// _nonConflictReferences.Contains(identifier)) -// { - -// } - -// return base.VisitAnonymousObjectMemberDeclarator(node); -// } - -// public override SyntaxNode? VisitArgument(ArgumentSyntax node) -// { -// if (node.Parent is TupleExpressionSyntax tupleExpression && -// ShouldAddTupleMemberName(node, out var identifier) && -// tupleExpression.Arguments.Count(a => ShouldAddTupleMemberName(a, out _)) == 1) -// { -// return node.Update( -// SyntaxFactory.NameColon(identifier), node.RefKindKeyword, UpdateIdentifier(identifier)).WithTriviaFrom(node); -// } - -// return base.VisitArgument(node); -// } - -// private bool ShouldAddTupleMemberName(ArgumentSyntax node, [NotNullWhen(true)] out IdentifierNameSyntax? identifier) -// { -// if (node.NameColon == null && -// node.Expression is IdentifierNameSyntax id && -// _nonConflictReferences.Contains(id) && -// !SyntaxFacts.IsReservedTupleElementName(id.Identifier.ValueText)) -// { -// identifier = id; -// return true; -// } - -// identifier = null; -// return false; -// } - -// public override SyntaxNode? VisitCollectionExpression(CollectionExpressionSyntax node) -// { - - -// node.Update(VisitToken(node.OpenBracketToken), VisitList(node.Elements), VisitToken(node.CloseBracketToken)) -// } - -// public override SyntaxNode? VisitSpreadElement(SpreadElementSyntax node) -// { -// if (!ShouldSpreadCollectionIntoCollection(node.Expression, out var collectionToInline)) -// return node; - -// // inlining an existing `[...]` collection into a `..` spread element. We can just move the original -// // elements into the final location. -// var leadingTrivia = node.GetLeadingTrivia() is [.., (kind: SyntaxKind.WhitespaceTrivia) lastWhitespace] -// ? lastWhitespace -// : default; -// } -// } -//} From 8b1707eb41ace30b532aa2790c47a35eef0543b7 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 20 Jan 2025 21:55:32 -0800 Subject: [PATCH 3/3] Update src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs --- .../InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs index 1b6c68ac68f74..eff81955f23e9 100644 --- a/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs +++ b/src/Features/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs @@ -268,7 +268,7 @@ SyntaxNode RewriteScope(SyntaxNode scope) editor.ReplaceNode( spreadElement, (_, _) => collectionToInline.Elements.Select( - e => e.WithLeadingTrivia(leadingTrivia).WithoutTrailingTrivia())); + e => e.WithLeadingTrivia(leadingTrivia))); } else {