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.

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

Extract To Component: Code handling capabilities #10760

Open
wants to merge 29 commits into
base: features/extract-to-component
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9e84b15
Arranging selection logic
marcarro Jul 8, 2024
aa15f7b
Completed and corrected selected range extraction functionality
marcarro Jul 15, 2024
55588d6
Include only dependencies included in the selected range
marcarro Jul 19, 2024
5753836
Base component dependency functionality and fixed range selection bug
marcarro Jul 18, 2024
459605e
Added method to scan identifiers
marcarro Aug 1, 2024
1e8168b
Corrected naming and nits
marcarro Aug 5, 2024
419055b
Set up basic end to end test for extract component
marcarro Aug 7, 2024
80d7b18
Refactoring: responsibilities to resolver and not provider
marcarro Aug 16, 2024
2ec6a8d
Handle code case: Method and field promotion
marcarro Aug 19, 2024
78a6d4c
Nits and reverted some changes that werent supposed to make it
marcarro Aug 19, 2024
5278bd8
Added support for MarkupTagHelperElements
marcarro Aug 19, 2024
8cf4f66
Adapted for roslyn endpoint rename, and added tests
marcarro Aug 21, 2024
7795d9f
Refactoring of TryProcessMultiPointSelection and FindContainingSiblin…
marcarro Aug 21, 2024
2b67a3b
Change to FindContainingSiblingPair
marcarro Aug 21, 2024
426ed28
Modified forwared constant fields
marcarro Aug 22, 2024
b9492f2
Preemptively added suggestions from previous PR feedback
marcarro Aug 25, 2024
462062a
Naming fixes and simplified promoted method string generation
marcarro Aug 25, 2024
96221e5
Adjust indentation for component extraction and some more tests
marcarro Aug 27, 2024
0f547f3
Removed unnecessary usings and separated end to end tests
marcarro Aug 27, 2024
b37e6d8
Updated functionality to match Roslyn endpoint response, finished att…
marcarro Sep 3, 2024
5b76f23
Rebase. Made some mistakes while rebasing, I'm sorry.
marcarro Sep 3, 2024
cec455e
Updated to reflect changes in Roslyn API
marcarro Sep 3, 2024
d76d535
PR Feedback
marcarro Sep 4, 2024
e08e1d3
Corrected logic for searching through ancestors in IsInsideMarkupTag
marcarro Sep 4, 2024
1dfb8fd
Cleaned up usings and integrated PR Feedback for IsInsideMarkupTag
marcarro Sep 5, 2024
268e303
Ignore previous commit message!
marcarro Sep 5, 2024
65daa58
Changed GetUsingDirectivesInRange to use a SyntaxNode instead of a Sy…
marcarro Sep 5, 2024
310da7d
Added more tests
marcarro Sep 7, 2024
1393e15
Final refactoring: moved selection validation methods to resolver to …
marcarro Sep 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refactoring: responsibilities to resolver and not provider
marcarro committed Sep 3, 2024
commit 80d7b188ea3ef43abdb3d1aa13b5c35a7428d9a8
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.CodeDom;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
@@ -16,6 +17,7 @@
using Microsoft.CodeAnalysis.Razor.Logging;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using SyntaxNode = Microsoft.AspNetCore.Razor.Language.Syntax.SyntaxNode;

namespace Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Razor;

@@ -25,39 +27,17 @@ internal sealed class ExtractToComponentCodeActionProvider(ILoggerFactory logger

public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken)
{
if (context is null)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

if (!context.SupportsFileCreation)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

if (!FileKinds.IsComponent(context.CodeDocument.GetFileKind()))
if (!IsValidContext(context))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much cleaner checking!

{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

var syntaxTree = context.CodeDocument.GetSyntaxTree();
if (syntaxTree?.Root is null)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

// Make sure the selection starts on an element tag
var (startElementNode, endElementNode) = GetStartAndEndElements(context, syntaxTree, _logger);
if (startElementNode is null)
if (!IsSelectionValid(context, syntaxTree))
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

if (endElementNode is null)
{
endElementNode = startElementNode;
}

if (!TryGetNamespace(context.CodeDocument, out var @namespace))
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
@@ -92,24 +72,19 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeAct
return Task.FromResult<ImmutableArray<RazorVSInternalCodeAction>>([codeAction]);
}

private static (MarkupElementSyntax? Start, MarkupElementSyntax? End) GetStartAndEndElements(RazorCodeActionContext context, RazorSyntaxTree syntaxTree, ILogger logger)
private static bool IsValidContext(RazorCodeActionContext context)
{
var owner = syntaxTree.Root.FindInnermostNode(context.Location.AbsoluteIndex, includeWhitespace: true);
if (owner is null)
{
logger.LogWarning($"Owner should never be null.");
return (null, null);
}

var startElementNode = owner.FirstAncestorOrSelf<MarkupElementSyntax>();
if (startElementNode is null || IsInsideProperHtmlContent(context, startElementNode))
{
return (null, null);
}

var endElementNode = GetEndElementNode(context, syntaxTree);
return context is not null &&
context.SupportsFileCreation &&
FileKinds.IsComponent(context.CodeDocument.GetFileKind()) &&
context.CodeDocument.GetSyntaxTree()?.Root is not null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: RazorSyntaxTree.Root can't be null, so just need to check if the syntax tree is null

}

return (startElementNode, endElementNode);
private static bool IsSelectionValid(RazorCodeActionContext context, RazorSyntaxTree syntaxTree)
{
var owner = syntaxTree.Root.FindInnermostNode(context.Location.AbsoluteIndex, includeWhitespace: true);
var startElementNode = owner?.FirstAncestorOrSelf<MarkupElementSyntax>();
return startElementNode is not null && !IsInsideProperHtmlContent(context, startElementNode) && !HasDiagnosticErrors(startElementNode);
}

private static bool IsInsideProperHtmlContent(RazorCodeActionContext context, MarkupElementSyntax startElementNode)
@@ -124,91 +99,10 @@ private static bool IsInsideProperHtmlContent(RazorCodeActionContext context, Ma
context.Location.AbsoluteIndex < startElementNode.EndTag.SpanStart;
}

private static MarkupElementSyntax? GetEndElementNode(RazorCodeActionContext context, RazorSyntaxTree syntaxTree)
private static bool HasDiagnosticErrors(MarkupElementSyntax markupElement)
{
var selectionStart = context.Request.Range.Start;
var selectionEnd = context.Request.Range.End;
if (selectionStart == selectionEnd)
{
return null;
}

var endAbsoluteIndex = context.SourceText.GetRequiredAbsoluteIndex(selectionEnd);
var endOwner = syntaxTree.Root.FindInnermostNode(endAbsoluteIndex, true);
if (endOwner is null)
{
return null;
}

// Correct selection to include the current node if the selection ends immediately after a closing tag.
if (endOwner is MarkupTextLiteralSyntax && endOwner.ContainsOnlyWhitespace() && endOwner.TryGetPreviousSibling(out var previousSibling))
{
endOwner = previousSibling;
}

return endOwner.FirstAncestorOrSelf<MarkupElementSyntax>();
}

private static ExtractToComponentCodeActionParams CreateInitialActionParams(RazorCodeActionContext context, MarkupElementSyntax startElementNode, string @namespace)
{
return new ExtractToComponentCodeActionParams
{
Uri = context.Request.TextDocument.Uri,
ExtractStart = startElementNode.Span.Start,
ExtractEnd = startElementNode.Span.End,
Namespace = @namespace,
usingDirectives = []
};
}

/// <summary>
/// Processes a multi-point selection to determine the correct range for extraction.
/// </summary>
/// <param name="startElementNode">The starting element of the selection.</param>
/// <param name="endElementNode">The ending element of the selection, if it exists.</param>
/// <param name="actionParams">The parameters for the extraction action, which will be updated.</param>
private static void ProcessSelection(MarkupElementSyntax startElementNode, MarkupElementSyntax? endElementNode, ExtractToComponentCodeActionParams actionParams)
{
// If there's no end element, we can't process a multi-point selection
if (endElementNode is null)
{
return;
}

var startNodeContainsEndNode = endElementNode.Ancestors().Any(node => node == startElementNode);

// If the start element is an ancestor, keep the original end; otherwise, use the end of the end element
if (startNodeContainsEndNode)
{
actionParams.ExtractEnd = startElementNode.Span.End;
return;
}

// If the start element is not an ancestor of the end element, we need to find a common parent
// This conditional handles cases where the user's selection spans across different levels of the DOM.
// For example:
// <div>
// {|result:<span>
// {|selection:<p>Some text</p>
// </span>
// <span>
// <p>More text</p>
// </span>
// <span>
// </span>|}|}
// </div>
// In this case, we need to find the smallest set of complete elements that covers the entire selection.

// Find the closest containing sibling pair that encompasses both the start and end elements
var (extractStart, extractEnd) = FindContainingSiblingPair(startElementNode, endElementNode);

// If we found a valid containing pair, update the extraction range
if (extractStart is not null && extractEnd is not null)
{
actionParams.ExtractStart = extractStart.Span.Start;
actionParams.ExtractEnd = extractEnd.Span.End;
}
// Note: If we don't find a valid pair, we keep the original extraction range
var diagnostics = markupElement.GetDiagnostics();
return diagnostics.Any(d => d.Severity == RazorDiagnosticSeverity.Error);
}

private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen(returnValue: true)] out string? @namespace)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this can be removed and the extension method can be used directly

@@ -217,118 +111,4 @@ private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen
// and causing compiler errors. Avoid offering this refactoring if we can't accurately get a
// good namespace to extract to
=> codeDocument.TryComputeNamespace(fallbackToRootNamespace: true, out @namespace);

private static (SyntaxNode? Start, SyntaxNode? End) FindContainingSiblingPair(SyntaxNode startNode, SyntaxNode endNode)
{
// Find the lowest common ancestor of both nodes
var nearestCommonAncestor = FindNearestCommonAncestor(startNode, endNode);
if (nearestCommonAncestor is null)
{
return (null, null);
}

SyntaxNode? startContainingNode = null;
SyntaxNode? endContainingNode = null;

// Pre-calculate the spans for comparison
var startSpan = startNode.Span;
var endSpan = endNode.Span;

foreach (var child in nearestCommonAncestor.ChildNodes().Where(static node => node.Kind == SyntaxKind.MarkupElement))
{
var childSpan = child.Span;

if (startContainingNode is null && childSpan.Contains(startSpan))
{
startContainingNode = child;
if (endContainingNode is not null)
break; // Exit if we've found both
}

if (childSpan.Contains(endSpan))
{
endContainingNode = child;
if (startContainingNode is not null)
break; // Exit if we've found both
}
}

return (startContainingNode, endContainingNode);
}

private static SyntaxNode? FindNearestCommonAncestor(SyntaxNode node1, SyntaxNode node2)
{
var current = node1;

while (current is MarkupElementSyntax or
MarkupTagHelperAttributeSyntax or
MarkupBlockSyntax &&
current is not null)
{
if (current.Span.Contains(node2.Span))
{
return current;
}

current = current.Parent;
}

return null;
}

private static void AddUsingDirectivesInRange(SyntaxNode root, IEnumerable<string> usingsInSourceRazor, int extractStart, int extractEnd, ExtractToComponentCodeActionParams actionParams)
{
var components = new HashSet<string>();
var extractSpan = new TextSpan(extractStart, extractEnd - extractStart);

foreach (var node in root.DescendantNodes().Where(node => extractSpan.Contains(node.Span)))
{
if (node is MarkupTagHelperElementSyntax { TagHelperInfo: { } tagHelperInfo })
{
AddUsingFromTagHelperInfo(tagHelperInfo, components, usingsInSourceRazor, actionParams);
}
}
}

private static void AddUsingFromTagHelperInfo(TagHelperInfo tagHelperInfo, HashSet<string> components, IEnumerable<string> usingsInSourceRazor, ExtractToComponentCodeActionParams actionParams)
{
foreach (var descriptor in tagHelperInfo.BindingResult.Descriptors)
{
if (descriptor is null)
{
continue;
}

var typeNamespace = descriptor.GetTypeNamespace();

// Since the using directive at the top of the file may be relative and not absolute,
// we need to generate all possible partial namespaces from `typeNamespace`.

// Potentially, the alternative could be to ask if the using namespace at the top is a substring of `typeNamespace`.
// The only potential edge case is if there are very similar namespaces where one
// is a substring of the other, but they're actually different (e.g., "My.App" and "My.Apple").

// Generate all possible partial namespaces from `typeNamespace`, from least to most specific
// (assuming that the user writes absolute `using` namespaces most of the time)

// This is a bit inefficient because at most 'k' string operations are performed (k = parts in the namespace),
// for each potential using directive.

var parts = typeNamespace.Split('.');
for (var i = 0; i < parts.Length; i++)
{
var partialNamespace = string.Join(".", parts.Skip(i));

if (components.Add(partialNamespace) && usingsInSourceRazor.Contains(partialNamespace))
{
actionParams.usingDirectives.Add($"@using {partialNamespace}");
break;
}
}
}

nodesInBlock.IntersectWith(nodesInScope);
identifiersInBlock.IntersectWith(identifiersInScope);
actionParams.UsedIdentifiers = identifiersInBlock;
}
}
Loading