Conversation
ghost
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
Community
| public required string NewContents { get; init; } | ||
| } | ||
|
|
||
| // Not sure where to put these two records |
ryzngard
left a comment
ryzngard
left a comment
There was a problem hiding this comment.
Some initial feedback. It's hard to follow how this information will be used. Do you have the razor side ready? It would be helpful to see that side to better comment on this code.
| var project = context.Solution?.GetProject(request.Project); | ||
|
|
||
| if (project is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var document = context.Solution.GetDocument(request.Document); |
There was a problem hiding this comment.
If you're getting a document you can get the project it is in directly
| var project = context.Solution?.GetProject(request.Project); | |
| if (project is null) | |
| { | |
| return null; | |
| } | |
| var document = context.Solution.GetDocument(request.Document); | |
| var document = context.Solution.GetDocument(request.Document); | |
| var project = document.Project; |
There was a problem hiding this comment.
Added some more information in the initial post!
| var document = context.Solution.GetDocument(request.Document); | ||
| var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false); | ||
| var syntaxTree = semanticModel.SyntaxTree; | ||
| var root = (CompilationUnitSyntax)syntaxTree.GetRoot(cancellationToken); |
There was a problem hiding this comment.
I don't think the cast is needed here
| var invocationExpressions = root.DescendantNodes().OfType<InvocationExpressionSyntax>().ToList(); | ||
| var identifierNames = root.DescendantNodes().OfType<IdentifierNameSyntax>().ToList(); |
There was a problem hiding this comment.
This looks like you're getting all the invocations and identifiers in the whole file, not just ones used by razor.
| var invocationExpressions = root.DescendantNodes().OfType<InvocationExpressionSyntax>().ToList(); | ||
| var identifierNames = root.DescendantNodes().OfType<IdentifierNameSyntax>().ToList(); | ||
|
|
||
| List<MethodInsideRazorElementInfo> methods = []; |
There was a problem hiding this comment.
Likely these should be pooled types
| foreach (var invocation in invocationExpressions) | ||
| { | ||
| var invocationOperation = semanticModel.GetOperation(invocation, cancellationToken) as IInvocationOperation; | ||
| var invocationDataFlow = semanticModel.AnalyzeDataFlow(invocation); |
There was a problem hiding this comment.
This appears to be unused
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
I'm not sure what the aim of this endpoint is, so some of my comments might be irrelevant. Feel free to ignore :)
|
|
||
| public async Task<RazorComponentInfo?> HandleRequestAsync(RazorComponentInfoParams request, RequestContext context, CancellationToken cancellationToken) | ||
| { | ||
| var project = context.Solution?.GetProject(request.Project); |
There was a problem hiding this comment.
I don't know what is in request.Project, but generally speaking the way things work in LSP would be to send the Uri of the document in the request, and then access everything via the context parameter. So var document = context.GetRequiredDocument();, and from there if you need the project, it would just be document.Project.
Seems like this project isn't needed though.
| var invocationExpressions = root.DescendantNodes().OfType<InvocationExpressionSyntax>().ToList(); | ||
| var identifierNames = root.DescendantNodes().OfType<IdentifierNameSyntax>().ToList(); |
There was a problem hiding this comment.
Rather that descending through the whole tree twice, is it better to do it once and look for both of the node types you care about?
| foreach (var invocation in invocationExpressions) | ||
| { | ||
| var invocationOperation = semanticModel.GetOperation(invocation, cancellationToken) as IInvocationOperation; | ||
| var invocationDataFlow = semanticModel.AnalyzeDataFlow(invocation); |
There was a problem hiding this comment.
Can you include a comment on what this is analyzing? Is it the parameters to the method, or the method itself?
| var field = new SymbolInsideRazorElementInfo | ||
| { | ||
| Name = symbolInfo.Symbol.Name, | ||
| Type = symbolInfo.Symbol.GetType().ToString() |
There was a problem hiding this comment.
Pretty sure this is going to end up being Microsoft.CodeAnalysis.SourceFieldSymbol or something. Unlikely to be what you intended.
| Type = symbolInfo.Symbol.GetType().ToString() | ||
| }; | ||
|
|
||
| fields.Add(field); |
There was a problem hiding this comment.
Seems like there should be some filtering of duplicates here, if a field is referred to more than once
| public required int HostDocumentVersion { get; init; } | ||
|
|
||
| [JsonPropertyName("newContents")] | ||
| public required string NewContents { get; init; } |
| public required TextDocumentIdentifier Document { get; init; } | ||
|
|
||
| [JsonPropertyName("newDocument")] | ||
| public required TextDocumentIdentifier NewDocument { get; init; } |
|
|
||
| public required string ReturnType { get; set; } | ||
|
|
||
| public required List<string> ParameterTypes { get; set; } |
There was a problem hiding this comment.
Since these types are all for serialization, the List here, and the two in RazorComponentInfo should probably just be arrays.
| } | ||
|
|
||
| // Not sure where to put these two records | ||
| internal sealed record RazorComponentInfo |
There was a problem hiding this comment.
This doesn't seem to have anything to do with Razor components. Doesn't this contain C# info, not Razor info?
| public required string NewContents { get; init; } | ||
| } | ||
|
|
||
| // Not sure where to put these two records |
…ic info in specific range
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
I think there are two areas to focus on here: Firstly, generally speaking this is pretty clean code, and I can see what it's doing. What is not clear is why it is doing it, and that is where code comments would be helpful.
Secondly, C# is very complicated, and it's usually a worrying sign to take shortcuts and make assumptions. eg, assuming that the first token of something will be important is unlikely to hold in all cases. Assuming that a matching on strings, names, ToDisplayString() etc. will work is similarly unlikely to hold in all cases. Roslyn is very powerful, and when things get reduced to comparing strings, it usually means that power is being thrown away.
| var document = solution.GetDocument(request.Document); | ||
| if (document is null) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
There is no need to get the document from the solution, its already in the context.Document property.
| return null; | ||
| } | ||
|
|
||
| var blockNode = classDeclarationNode.DescendantNodes().OfType<BlockSyntax>().FirstOrDefault(); |
There was a problem hiding this comment.
What is the first block expected to be? Perhaps a comment, and or some tests, or something? Would it be more appropriate to find the first block that overlaps the generatedSpans?
| .Where(n => n != null).Select(n => n!); | ||
|
|
||
| var methodsInRange = methodsInClass.Where(m => identifiersInRange.Contains(m.Identifier.Text)); | ||
| var fieldsInRange = fieldsInClass.Where(f => f.Declaration.Variables.Any(v => identifiersInRange.Contains(v.Identifier.Text))); |
There was a problem hiding this comment.
How do you know the identifier being used, that has the same name as the field, actually represents the field?
eg. this snippet has 4 identifier names that match a field name, but only two refer to the field: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzhgHwAEAmARgFgAoIgZgAJT6Bhegb2vq/s+4AcoASwBu2DDEZkADPQB22ALYwA3Ly5rGDIinoBZABSswaegBF6AEwCU7Dd0kz5S+gF56AIneqq9+3e5EZACc+k4wVt6+AcH6GAAWgrgAdGER/lyBIWApiuGRUZIhFjlKaT7cAL7UVVTUdIwkZtQc5Rla0nK53hVAA===
Same question applies to properties in the line below.
| var identifiersInClass = classDeclarationNode.DescendantNodes().OfType<IdentifierNameSyntax>(); | ||
| var methodsInClass = classDeclarationNode.DescendantNodes().OfType<MethodDeclarationSyntax>(); | ||
| var fieldsInClass = classDeclarationNode.DescendantNodes().OfType<FieldDeclarationSyntax>(); | ||
| var propertiesInClass = classDeclarationNode.DescendantNodes().OfType<PropertyDeclarationSyntax>(); |
There was a problem hiding this comment.
It would be much better to loop through the tree once, and grab the information as it comes, rather than multiple times like this.
| var parameterTypes = method.ParameterList.Parameters.Count > 0 | ||
| ? method.ParameterList.Parameters | ||
| .Where(p => p.Type != null) | ||
| .Select(p => p.Type!.GetFirstToken().Text) |
There was a problem hiding this comment.
| pooledMethods.Add(new MethodSymbolicInfo | ||
| { | ||
| Name = method.Identifier.Text, | ||
| ReturnType = method.ReturnType.GetFirstToken().Text, |
There was a problem hiding this comment.
As above re: the first token.
| var expressionIdentifiers = expressionsInClass.SelectMany(e => e.DescendantNodes().OfType<IdentifierNameSyntax>()); | ||
| var expressionIdentifierNames = expressionIdentifiers.Select(i => semanticModel.GetSymbolInfo(i).Symbol?.Name) | ||
| .Where(n => n != null).Select(n => n!); |
There was a problem hiding this comment.
These lists will be a subset of the identifiers found above, right? Why does it need a separate list?
| if (node is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(node)); | ||
| } | ||
|
|
||
| if (typeSyntax is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(typeSyntax)); | ||
| } | ||
|
|
||
| if (semanticModel is null) | ||
| { | ||
| throw new ArgumentNullException(nameof(semanticModel)); | ||
| } |
There was a problem hiding this comment.
This is annotated code, so these are all unnecessary, right?
|
|
||
| // The 'isWrittenTo' property is not critical to functionality in current usage; it's only used in ExtractToComponent | ||
| // to determine if a code attribute that has been promoted to a parameter in a component should include a comment warning. | ||
| if (typeInfo.Type.ToDisplayString() == "string" || typeInfo.Type.IsValueType) |
| if (typeSymbol is INamedTypeSymbol namedTypeSymbol) | ||
| { | ||
| // Get the base name of the type, e.g., "List" | ||
| var typeName = namedTypeSymbol.Name; |
There was a problem hiding this comment.
The case with integer bothers me a bit: "int" will become "Int32" (etc.) after accessing the type symbol name. This is technically correct and causes no errors, but I wonder if there's a way to just keep the type name as the user defined it.
| return null; | ||
| } | ||
|
|
||
| var classDeclarationNode = root.DescendantNodes().OfType<ClassDeclarationSyntax>().FirstOrDefault(classSymbol => InheritsFromComponentBase(componentBaseSymbol, classSymbol, semanticModel)); |
There was a problem hiding this comment.
Nit: The parameter is not a classSymbol it is a classNode. "Symbol" has a specific meaning in Roslyn, in that it comes from the semantic model not the syntax tree.
| var identifiersInRange = identifiersInClass.Where(identifier => generatedSpans.Any(span => span.Contains(identifier.Span))) | ||
| .Select(identifier => new IdentifierAndSymbol | ||
| { | ||
| Identifier = identifier, | ||
| Symbol = semanticModel.GetSymbolInfo(identifier).Symbol | ||
| }) | ||
| .Where(x => x.Symbol is not null); |
There was a problem hiding this comment.
Consider moving this into the above loop, and simply building identifiersInRange inside that loop, rather than collecting all identifiers and filtering later.
|
|
||
| var fieldsInRange = fieldsInClass.Where(field => field.Declaration.Variables | ||
| .Any(variable => identifiersInRange | ||
| .Any(identifier => SymbolEqualityComparer.Default.Equals(identifier.Symbol, semanticModel.GetDeclaredSymbol(variable))))); |
There was a problem hiding this comment.
This comment applies to the methodsInRange and propertyInRange calculation too, but this happens to be the worst offender: Consider how many times GetDeclaredSymbol is called in this calculation, for each specific value of variable.
| }); | ||
| } | ||
|
|
||
| var expressionIdentifiersInRange = identifiersInRange.Where(i => i.Identifier.Ancestors().OfType<ExpressionStatementSyntax>().Any()); |
There was a problem hiding this comment.
Comment what this is finding? Why isn't this needed for methods? etc.
| return false; | ||
| } | ||
|
|
||
| foreach (var baseTypeSyntax in baseTypes) |
There was a problem hiding this comment.
This loop seems unnecessary. I think it should be possible to get the symbol info for the class declaration itself (GetDeclaredSymbol I think) and then InheritsFrom could be called directly with that type symbol.
| if (namedTypeSymbol.TypeArguments.Length > 0) | ||
| { | ||
| var typeArguments = string.Join(", ", namedTypeSymbol.TypeArguments.Select(FormatType)); | ||
| return $"{typeName}<{typeArguments}>"; // Returning a formatted string seems hacky so might need to be revisited. |
There was a problem hiding this comment.
Best not to do this manually, but rather use a call to ToDisplayString() that specifies an appropriate set of SymbolDisplayFormat values to create the desired result.
3 participants
Summary of changes
Created handler corresponding to a new LSP request endpoint in razor, to be used in Extract To Component project.
This handler analyzes methods and fields present in the generated C# file of a Razor document and returns information about them (return type, parameter types, symbol names).
See dotnet/razor#10760 file
ExtractToComponentCodeActionResolver.csfor details.