Skip to content

Add inheritance margin service and tagger#51929

Merged
Cosifne merged 35 commits intodotnet:feature/inheritanceMarginfrom
Cosifne:dev/shech/inheritanceMargin
Mar 24, 2021
Merged

Add inheritance margin service and tagger#51929
Cosifne merged 35 commits intodotnet:feature/inheritanceMarginfrom
Cosifne:dev/shech/inheritanceMargin

Conversation

@Cosifne
Copy link
Member

@Cosifne Cosifne commented Mar 17, 2021

What is added:

  1. An InheritanceMarginService used to get all the information needed for the margin from a document.
  2. A tagger that tag the GlyphMarginTag
  3. Options to control the feature
  4. Some tests

What's is not included:

  1. UI (planning to have it in another PR)

@Cosifne Cosifne requested review from a team as code owners March 17, 2021 06:20
@Cosifne Cosifne requested a review from a team March 17, 2021 06:20
@ghost ghost added the Area-IDE label Mar 17, 2021
@Cosifne Cosifne requested review from CyrusNajmabadi and sharwell and removed request for a team March 17, 2021 06:20
sharwell
sharwell reviewed Mar 17, 2021
View reviewed changes
sharwell
sharwell reviewed Mar 17, 2021
View reviewed changes
src/EditorFeatures/Core/InheritanceMargin/AbstractInheritanceMarginService_Helpers.cs
/// <summary>
/// For the <param name="memberSymbol"/>, get all the symbols implementing it.
/// </summary>
private static async Task<ImmutableArray<ISymbol>> GetImplementedSymbolsAsync(
Copy link
Contributor

@sharwell sharwell Mar 17, 2021

Choose a reason for hiding this comment

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

Suggested change
private static async Task<ImmutableArray<ISymbol>> GetImplementedSymbolsAsync(
private static async Task<ImmutableArray<ISymbol>> GetImplementingSymbolsAsync(

Copy link
Member Author

@Cosifne Cosifne Mar 18, 2021

Choose a reason for hiding this comment

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

: ) It should be implemented, since the 'implemented' is the down arrow. I'll update the comments here

Copy link
Contributor

@sharwell sharwell Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I↓ indicates that the current symbol (memberSymbol) is implemented by one or more other symbols. Those other symbols are the implementing symbols of memberSymbol.

Copy link
Contributor

@sharwell sharwell Mar 19, 2021

Choose a reason for hiding this comment

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

I'll try to send a small PR tomorrow to make all the cases consistent throughout this PR. Will be easier than itemizing them all in review.

Copy link
Member Author

@Cosifne Cosifne Mar 19, 2021

Choose a reason for hiding this comment

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

I think I found the gap, KnowMoniker.Implemented is I↓ and I basically follow its definition. And I think you treat is in the opsite way.

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 23, 2021
View reviewed changes
src/EditorFeatures/Core/InheritanceMargin/AbstractInheritanceMarginService.cs Outdated
// 2. System.ValueType. (otherwise margin would be shown for all structs)
// 3. System.Enum. (otherwise margin would be shown for all enum)
var baseTypes = allBaseTypes
.WhereAsArray(symbol => !IsUnwantedBaseType((ITypeSymbol)symbol));
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 23, 2021

Choose a reason for hiding this comment

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

the cast here seems unnecessary.

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 23, 2021
View reviewed changes
src/EditorFeatures/Core/InheritanceMargin/InheritanceRelationship.cs
Overriding = 4,

/// <summary>
/// Indicate the target is overridden by the member. It would be shown as O↓.
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 23, 2021

Choose a reason for hiding this comment

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

i don't love us encoding this. it's unclear to me where we'll land in the future with the UI. but this is just docs, so i'm ok with it for now.

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Mar 23, 2021

this is looking very good. no major concerns. almost all nits.

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 24, 2021
View reviewed changes
src/EditorFeatures/Core/InheritanceMargin/AbstractInheritanceMarginService.cs Outdated
{
var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var allDeclarationNodes = GetMembers(root, spanToSearch);
var allDeclarationNodes = GetMembers(root.DescendantNodes(spanToSearch).ToImmutableArray());
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 24, 2021

Choose a reason for hiding this comment

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

i'm on the fence about this DescendantNodes call. it will cause us to look at a lot of nodes. that isn't really awesome, esp as normally we would only have to find the TypeDecls, and that would be a very shallow and fast enumeration.

also, ToImmutableArray isn't really necessary here. it will force us to realize a potentially very large array full of pointers to nodes we don't care about.

Copy link
Member Author

@Cosifne Cosifne Mar 24, 2021

Choose a reason for hiding this comment

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

removed ToImmutableArray call.

I agree this might cause memory & perf problem and here are my thoughts.

  1. For this call, the searching might be faster if when we do DFS, stop and start backtracking if it meets method/property/etc.., it would save a lot cost. ( I could do it in following PRs)
  2. When I wrote this code, I was thinking we have many other command handler/ UI that requires knowing the inheritance chain info, we could cache these information per snapshot (maybe further, also per git commit?). But that's really a future work.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 24, 2021

Choose a reason for hiding this comment

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

For this call, the searching might be faster if when we do DFS, stop and start backtracking if it meets method/property/etc.., it would save a lot cost. ( I could do it in following PRs)

I think it's much easier than that. Just have each language walk the compilation unit, returning type declaration nodes. They only need to walk into namespaces, and into typedecls (because of nested types). they don't need to walk any further. this is a very quick walk that terminates quickly and only returns what is needed. then just process those type-decl nodes in order, gathering the members of interest.

@Cosifne
Copy link
Member Author

Cosifne commented Mar 24, 2021

FindBaseTypesAndInterfaces

I am guessing you are comment BaseTypeFinder.FindBaseTypesAndIntherfaces, it is returning ImmutableArray instread of ImmutableArray (doing an cast) I can update it

@Cosifne Cosifne merged commit 8dc8345 into dotnet:feature/inheritanceMargin Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Cosifne @CyrusNajmabadi @sharwell