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

Find All References 'Kind' column values for Namespaces and Types. #31682

Merged
merged 6 commits into from
Feb 17, 2019

Conversation

mavasani
Copy link
Contributor

#30155 added a new 'Kind' column to FAR window to indicate if a reference to a method/field/property/event/parameter/local is a value read, write, reference or combination of these values.

This PR extends that column to now support rich reference kinds values for Namespaces and Types:

  1. Namespace: Indicates if a namespace is used in a context of a using directive (Using), declaration (Declare) or a name/member qualification, i.e. left side of dot (Qualify).

image

  1. Types: Indicates if a named type is used in a context of a using directive (Using), base type or interface in base list (Base), object creation (new()), a generic type argument (Type Argument) or a name/member qualification, i.e. left side of dot (Qualify).

image

The terminology used here is a rough draft, that would go through a design review prior to merge - any suggestions are welcome!

@mavasani mavasani requested review from heejaechang, CyrusNajmabadi and a team December 11, 2018 00:31
@mavasani mavasani changed the title Find All Reference 'Kind' column values for Namespaces and Types. Find All References 'Kind' column values for Namespaces and Types. Dec 11, 2018
@jinujoseph jinujoseph added this to the 16.0.P2 milestone Dec 11, 2018
@CyrusNajmabadi
Copy link
Member

What's the scenario where i care about this for namespaces?

I totally get the value for Types. But i'm not really seeing the namespace case.

@mavasani
Copy link
Contributor Author

@CyrusNajmabadi Primarily for completeness, especially given the implementation was pretty trivial to extend for namespaces.

@mavasani
Copy link
Contributor Author

Ping for reviews...

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi Primarily for completeness, especially given the implementation was pretty trivial to extend for namespaces.

i would personally remove the namespace bit as i don't see an actual user scenario for it. Will review hte rest today/tonight though!

(Note: you might want to just run the namespace part past @sharwell and @jasonmalinowski as well. if they think this is worthwhile, i'm ok with it).

@mavasani
Copy link
Contributor Author

i would personally remove the namespace bit as i don't see an actual user scenario for it. Will review hte rest today/tonight though!

I have personally felt a weak preference for such a categorization at times. For example, when I wanted to audit all types declared under Microsoft.CodeAnalysis.Operations for big cross cutting change, I want to filter out to only see references in declarations as opposed to references in usings from files that use these operations. I am fine with reverting the namespace part if @dotnet/roslyn-ide feel the same. I will bring this up at the next design meeting, given we need to anyways review the terminology here.

@mavasani mavasani added the Need Design Review The end user experience design needs to be reviewed and approved. label Dec 13, 2018
@mavasani
Copy link
Contributor Author

Btw, I understand that a simple textual search can address the above scenario - hence I don't have a strong push for namespace references categorization.

@CyrusNajmabadi
Copy link
Member

is used in a context of a using directive (Using)

Is this called 'Import' on the VB side? (note: i'm ok with it not being htat way. just curious). There's arguments for consistency (so you can filter down to all imports/usings with a single category), as well as them being different (to match the language).

/// Represents a reference to a namespace or type within a using or imports directive.
/// For example, <code>using NS;</code> or <code>using static NS.Extensions</code> or <code>using Alias = MyType</code>.
/// </summary>
NamespaceOrTypeInUsing = 0x100,
Copy link
Member

Choose a reason for hiding this comment

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

IDE style is to call this "Imports" when it refers to something across VB and C# and "import" is a more general term that fits equally well with both langauges, whereas 'using' is very c# specific.

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 can change the name here.

@CyrusNajmabadi
Copy link
Member

I really like the NamespaceOrTypeUsage + ValueUsage change. Nearly all my points on it are simply nits about particular names/semantics. But the essence of the change is 💯 for me.

@Neme12
Copy link
Contributor

Neme12 commented Dec 16, 2018

Using this feature, will it be easy to find all derived classes of a class?

@mavasani
Copy link
Contributor Author

Using this feature, will it be easy to find all derived classes of a class?

Yes, in fact that was one of my primary use cases. You can filter references with “Base” as the only checked reference kind.

@CyrusNajmabadi
Copy link
Member

Using this feature, will it be easy to find all derived classes of a class?

Note: go-to-impl shoudl work for that case. That's one of the core use cases for it.

@mavasani
Copy link
Contributor Author

@sharwell @jinujoseph can we bring this to design meeting next week?

@jinujoseph jinujoseph modified the milestones: 16.0.P2, Backlog Jan 18, 2019
@mavasani
Copy link
Contributor Author

Update from the design meeting:

  1. Using should be Import.
  2. Keep "Declare"
  3. new() - should be "Construct"
  4. Base - could be "Base Type" - But it won't always be the base type
  5. Keep "Type Argument"
  6. The "blank" Kind category is the common reference for declaring symbols of that type.
  7. Add "Constraint"
  8. Consider: add "Parameter", "Local", "Return type" in the future

@mavasani mavasani removed the Need Design Review The end user experience design needs to be reviewed and approved. label Feb 13, 2019
@mavasani mavasani closed this Feb 14, 2019
@mavasani mavasani reopened this Feb 14, 2019
@mavasani
Copy link
Contributor Author

@dotnet/roslyn-ide @CyrusNajmabadi - Addressed feedback from the design meeting. Please review.

@mavasani
Copy link
Contributor Author

Ping @dotnet/roslyn-ide for reviews.

@mavasani mavasani merged commit de57f6d into dotnet:master Feb 17, 2019
@mavasani mavasani deleted the FAR_Kind branch February 17, 2019 06:32
@jinujoseph jinujoseph modified the milestones: Backlog, 16.1.P1 Feb 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants