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.

Sign up for GitHub

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

Jump to bottom

Handle more stack frames #58769

Merged
merged 13 commits into from
Jan 13, 2022
Merged

Handle more stack frames #58769

merged 13 commits into from
Jan 13, 2022

Conversation

ryzngard
Copy link
Contributor

This adds support for local functions and generated main in the parser.

Symbol resolution now is broken up into resolvers and has special logic for local functions, getters/setters, and ordinary functions.

@ryzngard ryzngard requested a review from a team as a code owner January 11, 2022 06:47
ryzngard
ryzngard commented Jan 11, 2022
View reviewed changes
src/EditorFeatures/Test/StackTraceExplorer/StackTraceExplorerTests.cs
}

[Fact]
public Task TestSymbolFound_ExceptionLine_NestedLocalFunctions()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test currently will fail. I haven't figured out the best way to go from the symbols for LocalFunctions to the generated slots.

Copy link
Member

Choose a reason for hiding this comment

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

what's the issue here?

Copy link
Member

Choose a reason for hiding this comment

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

oh, is it that the outer and inner local function names are teh same? yeah, we may not be able to support hat. do you have tests where the names are different?

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'll add a test with different names. The issue will always be if there is something with the same name in the containing member. We could try to reverse engineer it but likely it would be wrong.

I wonder if just going to the containing method in those cases would be a good compromise.

ryzngard
ryzngard commented Jan 11, 2022
View reviewed changes
...s/SharedUtilitiesAndExtensions/Compiler/Core/EmbeddedLanguages/Common/EmbeddedSyntaxToken.cs
@@ -93,7 +93,7 @@ public string ToFullString()
/// <param name="trailing">If false, trailing trivia will not be added</param>
public void WriteTo(StringBuilder sb, bool leading, bool trailing)
{
if (leading)
if (leading && !LeadingTrivia.IsDefault)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this check, optional children would throw on ToFullString because everything is the default for an EmbeddedSyntaxToken

Copy link
Member

Choose a reason for hiding this comment

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

interesting... LeadingTrivia should probably be non-default... but that can be a change in another PR.

ryzngard
ryzngard commented Jan 11, 2022
View reviewed changes
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs Outdated
Compilation compilation,
CancellationToken cancellationToken)
{
if (typeSymbol is null || compilation.Language != LanguageNames.CSharp)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

❓ is it worth avoiding enumeration of members gated on the compilation language here?

Copy link
Member

Choose a reason for hiding this comment

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

yes. this check seems totally reasonable to me :)

Copy link
Member

Choose a reason for hiding this comment

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

oh... this is a generic helper now? hrmm... we can always keep this here, and remove later if a problem. but add comment for now that VB doesn't have local functions.

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, move this to ISemanticFactsService. then teh VB impl can no-op with an appropriate comment.

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 like this. I forgot about ISemanticFactsService.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 11, 2022
View reviewed changes
src/EditorFeatures/Core/StackTraceExplorer/StackFrameLocalMethodResolver.cs

var candidateFunctions = localFunctions
.Where(m => m.ContainingSymbol.Name == containingMethodName)
.ToImmutableArray();
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is a lot of work trying to get all the local functions, tehn filtering down to those that match the containing symbol name. consider inverting. get the containing symbol, and then look for local functions within it.

consider doc'ing how this works for nested local functions as well.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 11, 2022
View reviewed changes
src/EditorFeatures/Core/StackTraceExplorer/StackTraceExplorerUtilities.cs
{
if (parameters.Length != stackFrameParameters.Parameters.Length)
var compilation = await project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);
var type = compilation.GetTypeByMetadataName(fullyQualifiedTypeName);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var type = compilation.GetTypeByMetadataName(fullyQualifiedTypeName);
var type = compilation.GetBestTypeByMetadataName(fullyQualifiedTypeName);

Copy link
Member

Choose a reason for hiding this comment

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

it would be great if we could avoid getting the compilation if the final part of the type-name is not found in the index for this project. getting a compilation is expensive.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 11, 2022
View reviewed changes
src/EditorFeatures/Test/EmbeddedLanguages/StackFrame/StackFrameParserTests.Utilities.cs Outdated
@@ -86,6 +86,13 @@ private static void FuzzyTest(string input)

private static void VerifyCharacterSpans(string originalText, StackFrameTree tree)
{
// Use the ToFullString to verify
Copy link
Member

Choose a reason for hiding this comment

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

doesn't really tell me anything.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 11, 2022
View reviewed changes
src/Features/Core/Portable/EmbeddedLanguages/StackFrame/StackFrameLexer.cs Outdated
public Result<StackFrameToken> TryScanRequiredGeneratedNameSeparator()
{
var start = Position;
if (IsCharacter(CurrentChar))
Copy link
Member

Choose a reason for hiding this comment

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

so it's ok that it's not necessary g?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I wanted the lexer to know that generated name separators are just {letter}__ , and the parser determines whether the letter accounts for something that is supported.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 11, 2022
View reviewed changes
src/Features/Core/Portable/EmbeddedLanguages/StackFrame/StackFrameParser.cs
/// ^---^----------- "|0_0" is suffix information such as slot
/// ^--------^- "(String s)" identifiers the method paramters
/// </code>
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

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

awesome!

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 11, 2022
View reviewed changes
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs Outdated
var semanticModel = compilation.GetSemanticModel(syntaxReference.SyntaxTree);
var node = syntaxReference.GetSyntax(cancellationToken);

foreach (var localFunction in node.DescendantNodes().Where(syntaxFacts.IsLocalFunctionStatement))
Copy link
Member

Choose a reason for hiding this comment

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

i feel ike this might double count local functions found inside accessors. because we'll hit the prop and the accessor.

CyrusNajmabadi
CyrusNajmabadi approved these changes Jan 11, 2022
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

tentative signoff. there are some things worth looking into.

@ryzngard ryzngard enabled auto-merge (squash) January 12, 2022 22:47
@ryzngard ryzngard merged commit 61e507e into dotnet:main Jan 13, 2022
@ghost ghost added this to the Next milestone Jan 13, 2022
@ryzngard ryzngard deleted the issues/generated_stack_frames branch January 13, 2022 00:46
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.2.P1
Development

Successfully merging this pull request may close these issues.
3 participants
@ryzngard @CyrusNajmabadi @RikkiGibson