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

Add new parser/lexer to the StackTraceAnalyzer #57598

Merged
merged 21 commits into from
Dec 1, 2021
Merged

Add new parser/lexer to the StackTraceAnalyzer #57598

merged 21 commits into from
Dec 1, 2021

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Nov 5, 2021 •
edited
Loading

No functional changes, just moving to the new API and cleaning up unused code

@ryzngard ryzngard force-pushed the features/stackframe_parser_ui branch from 63823bb to c909bd2 Compare November 5, 2021 17:41
@ryzngard ryzngard marked this pull request as ready for review November 5, 2021 18:18
@ryzngard ryzngard requested a review from a team as a code owner November 5, 2021 18:18
CyrusNajmabadi
CyrusNajmabadi reviewed Nov 5, 2021
View reviewed changes
src/VisualStudio/Core/Def/StackTraceExplorer/StackFrameViewModel.cs Outdated
var tree = _frame.Tree;
var className = methodDeclaration.MemberAccessExpression.Left;
var leadingTrivia = className.GetLeadingTrivia();
yield return MakeClassifiedRun(ClassificationTypeNames.Text, leadingTrivia.CreateString());
Copy link
Member

Choose a reason for hiding this comment

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

don't love IEnuemrables. it makes it unclear to me on the consumption side that we're not accidentallly running all those code multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷‍♂️ yield return is easier to find than builder.Add. Agree it does make consumption ambiguous. Other than intrinsic knowledge of how property binding works... which guarantees this is only read once.

I'll think on it more, may just switch to ImmutableArray + builder.

This was referenced Nov 8, 2021
Open
Closed
@CyrusNajmabadi
Copy link
Member

image

Can you add one more line to make this balanced (as all things should be)?

CyrusNajmabadi
CyrusNajmabadi reviewed Nov 29, 2021
View reviewed changes
src/Features/Core/Portable/StackTraceExplorer/IStackTraceExplorerService.cs
/// </summary>
string GetTypeMetadataName(string className);
(Document? document, int line) GetDocumentAndLine(Solution solution, ParsedFrame frame);
Copy link
Member

Choose a reason for hiding this comment

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

we have a type called DocumentSpan that would likely be appropriate here.

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'm not fully convinced, mostly because it would still be mapping from span to line as an exercise of the caller. It's not terribly hard, but still tedious. And then it's unclear... is the span the line? The symbol definition location?

CyrusNajmabadi
CyrusNajmabadi reviewed Nov 29, 2021
View reviewed changes
src/Features/Core/Portable/StackTraceExplorer/IStackTraceExplorerService.cs
/// </summary>
string GetTypeMetadataName(string className);
(Document? document, int line) GetDocumentAndLine(Solution solution, ParsedFrame frame);
Task<DefinitionItem?> TryFindDefinitionAsync(Solution solution, ParsedFrame frame, StackFrameSymbolPart symbolPart, CancellationToken cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

interesting.. what is a SymbolPart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method or Class currently. Bool seems insufficient, enum naming is hard. Hopefully the enum value names make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving open to discuss naming as needed.

@runfoapp runfoapp bot mentioned this pull request Nov 30, 2021
Closed
@ryzngard ryzngard enabled auto-merge (squash) November 30, 2021 20:40
@runfoapp runfoapp bot mentioned this pull request Nov 30, 2021
Closed
@ryzngard
Copy link
Contributor Author

ryzngard commented Dec 1, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@ryzngard ryzngard merged commit 8f19149 into dotnet:main Dec 1, 2021
@ghost ghost added this to the Next milestone Dec 1, 2021
ryzngard added a commit to ryzngard/roslyn that referenced this pull request Dec 1, 2021
@ryzngard
Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598)
42b324c 
No functional changes, just moving to the new API and cleaning up unused code
@ryzngard ryzngard deleted the features/stackframe_parser_ui branch December 1, 2021 06:08
ryzngard added a commit to ryzngard/roslyn that referenced this pull request Dec 1, 2021
@ryzngard
Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598)
5b25ef9 
No functional changes, just moving to the new API and cleaning up unused code
@runfoapp runfoapp bot mentioned this pull request Dec 1, 2021
Closed
ryzngard added a commit that referenced this pull request Dec 1, 2021
@ryzngard
Add new parser/lexer to the StackTraceAnalyzer (#57598) (#58050)
0ce9a49 
No functional changes, just moving to the new API and cleaning up unused code
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 3, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into interp-nullable-imp…
7663d94 
…rovements

* upstream/main: (310 commits)
  Read SourceLink info and call service to retrieve source from there (dotnet#57978)
  Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) (dotnet#58050)
  Snap 17.1 P2 (dotnet#58041)
  Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (dotnet#57576)
  Shorten paths in VS installation (dotnet#57726)
  Add comments
  Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598)
  Fix await completion for expression body lambda
  Add tests
  Fix comment
  Honor option, and also improve formatting with comment
  Skip TestLargeStringConcatenation (dotnet#58035)
  Log runtime framework of remote host
  Mark EqualityContract property accessor as not auto-implemented (dotnet#57917)
  Fix typo in XML doc for GeneratorExtensions (dotnet#58020)
  Hold Receiver directly in bound node for implicit indexer access (dotnet#58009)
  Pass AnalysisKind instead of int
  Enable nullable reference types for TableDataSource
  Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it (dotnet#57966)
  Add missing test for CallerArgumentExpression (dotnet#57805)
  ...
JoeRobich added a commit that referenced this pull request Dec 6, 2021
@JoeRobich @Youssef1313
@sharwell @bradselw @mavasani @genlu @CyrusNajmabadi @ryzngard @bernd5 @allisonchou @davidwengier
Merge main to main-vs-deps (#58066)
3e5ced3 
* Remove IDE0051 suppression

* Add LayoutKind

* LayoutKind.Sequential

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

* Create azure-pipelines-integration-dartlab.yml

* Update azure-pipelines-integration-dartlab.yml

* [Async lightbulb] Move suppression fixes to 'Low' priority bucket

Addresses second item in #56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.

* Cache last project and compilationWithAnalyzers

* Update test as normal priority bucket has one lesser code action

* Make shallow checkout optional for integration test CI

* Move pipeline checkout skips up a layer.

* Look up VS bootstrapper from build

* Revert skipPipelinesCheckout change

* Comment out checkout: none for now

* Added matrix

* Target topic branch in DartLab.Templates

* Update number of machines to 4

* Run test agent elevated

* Update pipeline description

* Default VS branch to main

* Address feedback

* Exclude unnecessary sqlite assemblies

* Convert namespace to file scoped when typing semicolon

* Working on tests

* Add tests

* Add comments

* Add comments

* Revert

* Revert

* Make static

* Add tests

* Pass AnalysisKind instead of int

* Honor option, and also improve formatting with comment

* Fix comment

* Add tests

* Fix await completion for expression body lambda

* Add new parser/lexer to the StackTraceAnalyzer (#57598)

No functional changes, just moving to the new API and cleaning up unused code

* Add comments

* Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (#57576)

Fixes #57577.

* Snap 17.1 P2 (#58041)

* Add new parser/lexer to the StackTraceAnalyzer (#57598) (#58050)

No functional changes, just moving to the new API and cleaning up unused code

* Read SourceLink info and call service to retrieve source from there (#57978)

* Merge pull request #58100 from dotnet/dev/jorobich/skip-test

Skip flaky CommandLine.ArgumentParsing test

See #58077 for more details

Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
Co-authored-by: Brad White <4261702+bradselw@users.noreply.github.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Brad White <bradwhit@microsoft.com>
Co-authored-by: gel@microsoft.com <gel@microsoft.com>
Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: Andrew Hall <andrha@microsoft.com>
Co-authored-by: Bernd Baumanns <baumannsbernd@gmail.com>
Co-authored-by: Allison Chou <allichou@microsoft.com>
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: Gen Lu <genlu@users.noreply.github.com>
@vatsalyaagrawal vatsalyaagrawal modified the milestones: Next, 17.1.P2 Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoeRobich JoeRobich JoeRobich approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
Approved to merge Area-IDE
Projects
None yet
Milestone
17.1.P2
Development

Successfully merging this pull request may close these issues.
4 participants
@ryzngard @CyrusNajmabadi @JoeRobich @vatsalyaagrawal