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

Merge features/extract-to-component to main #11019

Merged
merged 73 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from 71 commits
Commits
Show all changes
73 commits
Select commit Hold shift + click to select a range
9ba71db
Added basic extract to component functionality on cursor over html tag
marcarro Jul 3, 2024
7e3ef75
PR Feedback
marcarro Jul 8, 2024
74956e1
Merge branch 'features/extract-to-component' into dev/t-hearroyo/Extr…
marcarro Jul 8, 2024
bce001c
Changed JToken Use in PR
marcarro Jul 8, 2024
60d5881
PR Feedback and added GenerateUniquePath
marcarro Jul 9, 2024
8ab2133
Changed usage from GenerateCodeBehindPath and GenerateComponentPath t…
marcarro Jul 9, 2024
c63e51c
More PR Feedback
marcarro Jul 10, 2024
09435b4
Reverted nameof change for LanguageServerConstant 'EditBasedCodeActio…
marcarro Jul 10, 2024
921aef1
Update feature branch (#10652)
ryzngard Jul 19, 2024
afc61ea
Changed class constructor to primary constructor
marcarro Jul 19, 2024
6d8e44c
Merge from main
marcarro Jul 22, 2024
2908e6c
PR Feedback and update according to new IRazorCodeActionProvider API
marcarro Jul 22, 2024
9d7a565
Correct for test issues
marcarro Jul 22, 2024
371b09a
Fixed test issue
marcarro Jul 23, 2024
8725265
Added basic extract to component functionality on cursor over html ta…
phil-allen-msft Jul 24, 2024
6f56d75
Update feature branch (#10680)
phil-allen-msft Aug 14, 2024
142c3ed
Arranging selection logic
marcarro Jul 8, 2024
ef995cd
Basic select range feature: practically not functional
marcarro Jul 10, 2024
a81a177
Completed and corrected selected range extraction functionality
marcarro Jul 15, 2024
ea0ac53
Tried fixing nullability issues
marcarro Jul 19, 2024
0deb915
Commented out incomplete test. Must be finished
marcarro Jul 19, 2024
46932b3
Rebased to feature branch and updated according to new API
marcarro Jul 24, 2024
a939694
Changed null assert to empty assert according to ci test results
marcarro Jul 24, 2024
a814ca8
Refactored element selection logic into methods for easier readability
marcarro Jul 25, 2024
8ba59f2
Removed unnecessary using
marcarro Jul 25, 2024
cd6b24a
Added check for IsInsideProperHtmlContent
marcarro Jul 25, 2024
55669a7
Removed unnecessary iteration variable in FindContainingSiblignPair
marcarro Jul 25, 2024
8ee84de
nits and test fixes
marcarro Jul 26, 2024
eba1b01
Functioning tests and fixed selection issue
marcarro Jul 26, 2024
6656f7a
Added null check
marcarro Jul 26, 2024
31282c8
Test updates
marcarro Jul 30, 2024
04aa349
PR Feedback
marcarro Jul 30, 2024
8dbd672
Adapted to new TryGetSourceLocation and clarified ambiguous Range ref…
marcarro Aug 20, 2024
8a7d0bb
Changed tests to use VsLspFactory utiltiies for multi point selection
marcarro Aug 20, 2024
7d3c78a
PR Feedback
marcarro Aug 20, 2024
842b162
Added early return in ProcessMultiPointSelection
marcarro Aug 21, 2024
4bab4dd
No need to check for multipoint selection
marcarro Aug 21, 2024
4297e6f
PR Feedback
marcarro Aug 21, 2024
f1923ed
Select range feature (#10621)
phil-allen-msft Aug 22, 2024
0471dbf
Arranging selection logic
marcarro Jul 8, 2024
1b6e035
Completed and corrected selected range extraction functionality
marcarro Jul 15, 2024
cba6cf1
Base component dependency functionality and fixed range selection bug
marcarro Jul 18, 2024
cf33fd7
Include only dependencies included in the selected range
marcarro Jul 19, 2024
019d6a4
Test updates
marcarro Jul 30, 2024
dcee923
Completed and corrected selected range extraction functionality
marcarro Jul 15, 2024
6e8d2aa
Nits
marcarro Jul 30, 2024
17479d4
Added method to scan identifiers
marcarro Aug 1, 2024
2c4b709
Corrected naming and nits
marcarro Aug 5, 2024
5bab548
Set up basic end to end test for extract component
marcarro Aug 7, 2024
83703f6
Finished fixing rebase stuff
marcarro Aug 22, 2024
33c66f4
Some of the PR feedback that wasn't deferred to the next one, and add…
marcarro Aug 25, 2024
b170f2a
More PR Feedback
marcarro Aug 25, 2024
5b7e0c6
Naming corrections
marcarro Aug 25, 2024
3d9614c
Exclude usings that are already in _Imports.razor
marcarro Aug 26, 2024
d83031b
Another approach for refining new component usings
marcarro Aug 26, 2024
1c239a1
Use syntaxTree instead of sourceText
marcarro Aug 26, 2024
cd677c7
Adjustments to addUsingDirectives and adjusted for relative using nam…
marcarro Aug 27, 2024
851fb45
Nits. I'm not sure why tests are failing
marcarro Aug 27, 2024
be3686f
Including @using for Out-of-Scope Razor Component References (#10651)
ryzngard Aug 30, 2024
be1f0d7
Merge branch 'main' into features/extract-to-component
ryzngard Sep 30, 2024
ea556b9
Fix merge
ryzngard Oct 1, 2024
b776c60
Fix moq in tests
ryzngard Oct 4, 2024
17dbcdb
Merge main into features/extract-to-component (#10948)
ryzngard Oct 5, 2024
e060ce0
Some Cleanup For ExtractToComponent (#11008)
ryzngard Oct 15, 2024
fccbbfc
Merge branch 'main' into features/extract-to-component
ryzngard Oct 15, 2024
da1b19b
Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeAc…
ryzngard Oct 15, 2024
95123dc
Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeAc…
ryzngard Oct 15, 2024
f07da46
PR feedback
ryzngard Oct 15, 2024
fa3b2bf
Merge branch 'merge_to_main' of https://github.com/ryzngard/razor int…
ryzngard Oct 15, 2024
c873dc6
Add expected original document to E2E tests
ryzngard Oct 15, 2024
94407a0
More tests. Fix some bugs. Dedent new file
ryzngard Oct 17, 2024
d64e2b3
PR feedback and move unindent logic to FormattingUtilities
ryzngard Oct 18, 2024
a49c67d
USINGS....
ryzngard Oct 18, 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
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(
}

var tree = context.CodeDocument.GetSyntaxTree();
var node = tree.Root.FindInnermostNode(context.Location.AbsoluteIndex);
var node = tree.Root.FindInnermostNode(context.StartLocation.AbsoluteIndex);
var isInImplicitExpression = node?.AncestorsAndSelf().Any(n => n is CSharpImplicitExpressionSyntax) ?? false;

var allowList = isInImplicitExpression
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ static bool TryGetOwner(RazorCodeActionContext context, [NotNullWhen(true)] out
return false;
}

owner = syntaxTree.Root.FindInnermostNode(context.Location.AbsoluteIndex);
owner = syntaxTree.Root.FindInnermostNode(context.StartLocation.AbsoluteIndex);
if (owner is null)
{
Debug.Fail("Owner should never be null.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,16 +161,22 @@ public void ApplyCapabilities(VSInternalServerCapabilities serverCapabilities, V
request.Range = vsCodeActionContext.SelectionRange;
}

if (!sourceText.TryGetSourceLocation(request.Range.Start, out var location))
if (!sourceText.TryGetSourceLocation(request.Range.Start, out var startLocation))
{
return null;
}

if (!sourceText.TryGetSourceLocation(request.Range.End, out var endLocation))
{
endLocation = startLocation;
}

var context = new RazorCodeActionContext(
request,
documentSnapshot,
codeDocument,
location,
startLocation,
endLocation,
sourceText,
_languageServerFeatureOptions.SupportsFileManipulation,
_supportsCodeActionResolve);
Expand All @@ -180,7 +186,7 @@ public void ApplyCapabilities(VSInternalServerCapabilities serverCapabilities, V

private async Task<ImmutableArray<RazorVSInternalCodeAction>> GetDelegatedCodeActionsAsync(DocumentContext documentContext, RazorCodeActionContext context, Guid correlationId, CancellationToken cancellationToken)
{
var languageKind = context.CodeDocument.GetLanguageKind(context.Location.AbsoluteIndex, rightAssociative: false);
var languageKind = context.CodeDocument.GetLanguageKind(context.StartLocation.AbsoluteIndex, rightAssociative: false);

// No point delegating if we're in a Razor context
if (languageKind == RazorLanguageKind.Razor)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Text.Json.Serialization;

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

internal sealed class ExtractToComponentCodeActionParams
{
[JsonPropertyName("uri")]
public required Uri Uri { get; set; }

[JsonPropertyName("start")]
public int Start { get; set; }

[JsonPropertyName("end")]
public int End { get; set; }

[JsonPropertyName("namespace")]
public required string Namespace { get; set; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ internal sealed class ComponentAccessibilityCodeActionProvider : IRazorCodeActio
public async Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken)
{
// Locate cursor
var node = context.CodeDocument.GetSyntaxTree().Root.FindInnermostNode(context.Location.AbsoluteIndex);
var node = context.CodeDocument.GetSyntaxTree().Root.FindInnermostNode(context.StartLocation.AbsoluteIndex);
if (node is null)
{
return [];
Expand All @@ -44,7 +44,7 @@ public async Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorC
return [];
}

if (context.Location.AbsoluteIndex < startTag.SpanStart)
if (context.StartLocation.AbsoluteIndex < startTag.SpanStart)
{
// Cursor is before the start tag, so we shouldn't show a light bulb. This can happen
// in cases where the cursor is in whitespace at the beginning of the document
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeAct
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

var owner = syntaxTree.Root.FindInnermostNode(context.Location.AbsoluteIndex);
var owner = syntaxTree.Root.FindInnermostNode(context.StartLocation.AbsoluteIndex);
if (owner is null)
{
_logger.LogWarning($"Owner should never be null.");
Expand Down Expand Up @@ -84,7 +84,7 @@ public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeAct
}

// Do not provide code action if the cursor is inside the code block
if (context.Location.AbsoluteIndex > csharpCodeBlockNode.SpanStart)
if (context.StartLocation.AbsoluteIndex > csharpCodeBlockNode.SpanStart)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ internal sealed class ExtractToCodeBehindCodeActionResolver(
return null;
}

var codeBehindPath = GenerateCodeBehindPath(path);
var codeBehindPath = FileUtilities.GenerateUniquePath(path, $"{Path.GetExtension(path)}.cs");

// VS Code in Windows expects path to start with '/'
var updatedCodeBehindPath = _languageServerFeatureOptions.ReturnCodeActionAndRenamePathsWithPrefixedSlash && !codeBehindPath.StartsWith("/")
Expand Down Expand Up @@ -111,33 +111,6 @@ internal sealed class ExtractToCodeBehindCodeActionResolver(
};
}

/// <summary>
/// Generate a file path with adjacent to our input path that has the
/// correct codebehind extension, using numbers to differentiate from
/// any collisions.
/// </summary>
/// <param name="path">The origin file path.</param>
/// <returns>A non-existent file path with the same base name and a codebehind extension.</returns>
private static string GenerateCodeBehindPath(string path)
{
var baseFileName = Path.GetFileNameWithoutExtension(path);
var extension = Path.GetExtension(path);
var directoryName = Path.GetDirectoryName(path).AssumeNotNull();

var n = 0;
string codeBehindPath;
do
{
var identifier = n > 0 ? n.ToString(CultureInfo.InvariantCulture) : string.Empty; // Make it look nice

codeBehindPath = Path.Combine(directoryName, $"{baseFileName}{identifier}{extension}.cs");
n++;
}
while (File.Exists(codeBehindPath));

return codeBehindPath;
}

private async Task<string> GenerateCodeBehindClassAsync(IProjectSnapshot project, Uri codeBehindUri, string className, string namespaceName, string contents, RazorCodeDocument razorCodeDocument, CancellationToken cancellationToken)
{
using var _ = StringBuilderPool.GetPooledObject(out var builder);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using ICSharpCode.Decompiler.CSharp.Syntax;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Syntax;
using Microsoft.AspNetCore.Razor.LanguageServer.CodeActions.Models;
using Microsoft.AspNetCore.Razor.PooledObjects;
using Microsoft.AspNetCore.Razor.Threading;
using Microsoft.CodeAnalysis.Razor.Logging;
using Microsoft.CodeAnalysis.Razor.Workspaces;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.LanguageServer.Protocol;

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

internal sealed class ExtractToComponentCodeActionProvider() : IRazorCodeActionProvider
{
public Task<ImmutableArray<RazorVSInternalCodeAction>> ProvideAsync(RazorCodeActionContext context, CancellationToken cancellationToken)
{
if (!context.SupportsFileCreation)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

if (!FileKinds.IsComponent(context.CodeDocument.GetFileKind()))
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

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

if (!TryGetNamespace(context.CodeDocument, out var @namespace))
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be code here that doesn't offer the action if there is C# code in the selection? Or are we just going with the approach of offering to do something even if its not great?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the latter, but @leslierichardson95 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine if that is the determination. Though would love to see tests that have it. In particular I'm curious about things like:

<div>blah</div>

[|Hello

@code {
  |]
}

or:

<div>blah</div>

[|Hello

@if (true) {
	<div>  |] </div>
}

or the very exciting

<div>blah</div>

[|Hello

 @{
     RenderFragment fragment =
     @<Component1 Id="Comp1"
                  Caption="Title">|]
     </Component1>;
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, I left your comment open on tests. Adding more today for sure

Choose a reason for hiding this comment

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

I believe the latter, but @leslierichardson95 what do you think?

Last time we chatted, I think we decided on the latter of showing the option which I'm still ok with as long as we still have telemetry attached that lets us generally know what the user was trying to accomplish.

var (startNode, endNode) = GetStartAndEndElements(context, syntaxTree);
if (startNode is null || endNode is null)
{
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>();
}

var actionParams = CreateActionParams(context, startNode, endNode, @namespace);

var resolutionParams = new RazorCodeActionResolutionParams()
{
Action = LanguageServerConstants.CodeActions.ExtractToNewComponentAction,
Language = LanguageServerConstants.CodeActions.Languages.Razor,
Data = actionParams,
};

var codeAction = RazorCodeActionFactory.CreateExtractToComponent(resolutionParams);
return Task.FromResult<ImmutableArray<RazorVSInternalCodeAction>>([codeAction]);
}

private static (SyntaxNode? Start, SyntaxNode? End) GetStartAndEndElements(RazorCodeActionContext context, RazorSyntaxTree syntaxTree)
{
var owner = syntaxTree.Root.FindInnermostNode(context.StartLocation.AbsoluteIndex, includeWhitespace: true);
if (owner is null)
{
return (null, null);
}

var startElementNode = owner.FirstAncestorOrSelf<SyntaxNode>(IsBlockNode);

if (startElementNode is null || LocationInvalid(context.StartLocation, startElementNode))
{
return (null, null);
}

var endElementNode = context.StartLocation == context.EndLocation
? startElementNode
: GetEndElementNode(context, syntaxTree);

return (startElementNode, endElementNode);

static bool LocationInvalid(SourceLocation location, SyntaxNode node)
{
// Make sure to test for cases where selection
// is inside of a markup tag such as <p>hello$ there</p>
if (node is MarkupElementSyntax markupElement)
{
return location.AbsoluteIndex > markupElement.StartTag.Span.End &&
location.AbsoluteIndex < markupElement.EndTag.SpanStart;
}

return !node.Span.Contains(location.AbsoluteIndex);
}
}

private static SyntaxNode? GetEndElementNode(RazorCodeActionContext context, RazorSyntaxTree syntaxTree)
{
var endOwner = syntaxTree.Root.FindInnermostNode(context.EndLocation.AbsoluteIndex, includeWhitespace: 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<SyntaxNode>(IsBlockNode);
}

private static bool IsBlockNode(SyntaxNode node)
=> node.Kind is
SyntaxKind.MarkupElement or
SyntaxKind.MarkupTagHelperElement or
SyntaxKind.CSharpCodeBlock;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably okay for now, but the Razor syntax tree is annoying and CSharpCodeBlock is pretty ubiquitous. I wonder if it's worth being more explicit with the check here, or at least logging an issue for follow up. I expect this will be a source of future quirks. eg given @attribute [StreamR$$endering], the first CSharpCodeBlock ancestor of the cursor is " [StreamRendering]" and in <div>@curren$$tCount</div> it's @currentCount, though one could argue that is extract-able.

I don't feel super strongly about it, though, because it's very much in the "if it hurts when you do that, don't do that" category IMO. Assuming the editor implements undo correctly anyway :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea... I wanted to keep it open until we find cases where it's hurting too much. Then add more tests and improve :)


private static ExtractToComponentCodeActionParams CreateActionParams(
RazorCodeActionContext context,
SyntaxNode startNode,
SyntaxNode endNode,
string @namespace)
{
var selectionSpan = AreSiblings(startNode, endNode)
? TextSpan.FromBounds(startNode.Span.Start, endNode.Span.End)
: GetEncompassingTextSpan(startNode, endNode);

return new ExtractToComponentCodeActionParams
{
Uri = context.Request.TextDocument.Uri,
Start = selectionSpan.Start,
End = selectionSpan.End,
Namespace = @namespace
};
}

private static TextSpan GetEncompassingTextSpan(SyntaxNode startNode, SyntaxNode endNode)
{
// Find a valid node that encompasses both the start and the end to
// become the selection.
var commonAncestor = endNode.Span.Contains(startNode.Span)
? endNode
: startNode;

while (commonAncestor is MarkupElementSyntax or
MarkupTagHelperAttributeSyntax or
MarkupBlockSyntax)
{
if (commonAncestor.Span.Contains(startNode.Span) &&
commonAncestor.Span.Contains(endNode.Span))
{
break;
}

commonAncestor = commonAncestor.Parent;
}

// If walking up the tree was required then make sure to reduce
// selection back down to minimal nodes needed.
// For example:
// <div>
// {|result:<span>
// {|selection:<p>Some text</p>
// </span>
// <span>
// <p>More text</p>
// </span>
// <span>
// </span>|}|}
// </div>
if (commonAncestor != startNode &&
commonAncestor != endNode)
{
SyntaxNode? modifiedStart = null, modifiedEnd = null;
foreach (var child in commonAncestor.ChildNodes().Where(static node => node.Kind == SyntaxKind.MarkupElement))
{
if (child.Span.Contains(startNode.Span))
{
modifiedStart = child;
if (modifiedEnd is not null)
break; // Exit if we've found both
}

if (child.Span.Contains(endNode.Span))
{
modifiedEnd = child;
if (modifiedStart is not null)
break; // Exit if we've found both
}
}

if (modifiedStart is not null && modifiedEnd is not null)
{
return TextSpan.FromBounds(modifiedStart.Span.Start, modifiedEnd.Span.End);
}
}

// Fallback to extracting the nearest common ancestor span
return commonAncestor.Span;
}

private static bool AreSiblings(SyntaxNode? node1, SyntaxNode? node2)
{
if (node1 is null)
{
return false;
}

if (node2 is null)
{
return false;
}

return node1.Parent == node2.Parent;
}

private static bool TryGetNamespace(RazorCodeDocument codeDocument, [NotNullWhen(returnValue: true)] out string? @namespace)
// If the compiler can't provide a computed namespace it will fallback to "__GeneratedComponent" or
// similar for the NamespaceNode. This would end up with extracting to a wrong namespace
// 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);
}
Loading
Loading