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 references of the base field or include descendant fields #9315

Conversation

RealyUniqueName
Copy link
Member

@RealyUniqueName RealyUniqueName commented Apr 15, 2020

Closes #9044
Closes #9139 (using withDescendants kind; see below)

This PR adds optional kind parameter to the display/references request:

  • "direct" - old behavior
  • "withBaseAndDescendants' - find references to the base field instead of the requested one. Also finds references to all descendants of the base field.
  • "withDescendants" - find references to the requested field and references to all descendants of the requested field.

It is possible to use this parameter with command line requests:

$ haxe build.hxml --display src/Main.hx@75@usage@withBaseAndDescendants
$ haxe build.hxml --display src/Main.hx@75@usage@withDescendants

@RealyUniqueName RealyUniqueName added the feature-ide IDE / Editor support label Apr 15, 2020
@RealyUniqueName RealyUniqueName added this to the Release 4.1 milestone Apr 15, 2020
@Simn
Copy link
Member

Simn commented Apr 15, 2020

Shouldn't these be some sort of flags on the "display/references" request instead?

src/core/tFunctions.ml Outdated Show resolved Hide resolved
@nadako
Copy link
Member

nadako commented Apr 15, 2020

Is there performance difference between these methods? Because I'm wondering if we should just always return everything and let IDEs figure out what to show. E.g. I imagine it would be nice to only ask user whether they want override usages if there are any overrides in the first place (not that VSCode supports asking anyway though).

@Gama11
Copy link
Member

Gama11 commented Apr 15, 2020

Well, for that the IDE would still need some metadata for each reference regarding whether it's a base reference or not. I imagined it could work the other way around, with the IDE providing a parameter that indicates whether or not to include descendants (could be a vshaxe setting).

| TClassDecl c :: _ when c.cl_path = cl_path -> c
| _ :: types -> loop types
in
loop com.types
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason we store the class' path and then dig it up here, instead of just storing the tclass reference itself like we usually do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.
tclass instances are not physically equal at this moment and at the moment of find_references invocation.
Maybe that's a bug, but I didn't investigate that.

@RealyUniqueName
Copy link
Member Author

Is there performance difference between these methods?

Yes. These methods perform as many reference searches as the amount of descendant methods in hierarchy.

And now I realized this could be done in one search :) Will update later.

@skial skial mentioned this pull request Apr 16, 2020
1 task
@RealyUniqueName
Copy link
Member Author

Changed to a parameter for display/references request instead of a separate request types.

@Gama11
Copy link
Member

Gama11 commented Apr 16, 2020

Hm, having three kinds confuses me a bit. What exactly is the "old behavior" right now / "normal"?

@RealyUniqueName
Copy link
Member Author

What exactly is the "old behavior" right now / "normal"?

It finds only "direct" references. No parent or overridden methods.

@Gama11
Copy link
Member

Gama11 commented Apr 16, 2020

Right... Not sure Normal is a good name / an enum is a good way to model this. In particular, I find this description a bit confusing:

Find references to the base field instead of the requested one.
Also finds references to all descendants of the base field.

So, not really "instead" if it finds references to the requested one after all? :)

I wonder if two boolean flags would work better?

typedef FindReferencesParams = {
    var ?includeBase:Bool;
    var ?includeDescendants:Bool;
}

Though I'm not sure if we would even want to expose the second as a setting, I think IntelliJ only lets you chose between base fields or not?

@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Apr 16, 2020

I've changed wording:

/**
	Find only direct references to the requested symbol.
	Does not look for references to parent or overriding methods.
**/
var Direct = "direct";
/**
	Find references to the base field and all the overriding fields in the inheritance chain.
**/
var Base = "base";
/**
	Find references to the requested field and references to all
	descendants of the requested field.
**/
var Descendants = "descendants";

I wonder if two boolean flags would work better?

I don't think so. There's actually no practical sence in this combination:

{
  includeBase:true,
  includeDescendants:false
}

@Gama11
Copy link
Member

Gama11 commented Apr 16, 2020

Technically these are Direct, WithBaseAndDescendants and WithDescendants then. Not sure if that's too wordy though.

@RealyUniqueName
Copy link
Member Author

Changed to Direct, WithBaseAndDescendants and WithDescendants.

@RealyUniqueName
Copy link
Member Author

@Simn are you ok with this PR?

@Simn
Copy link
Member

Simn commented Apr 16, 2020

Yes, except for the usage@withDescendants stuff. Let's not extend the old display API, I really don't want to maintain that longer than necessary.

@Gama11
Copy link
Member

Gama11 commented Apr 17, 2020

I wonder if we should think ahead a bit and name it ReferencesKind instead of FindReferencesKind? Most likely we will need the exact same differentiation for an eventual display/rename method, right?

Or perhaps "Find References" makes sense for rename anyway too?

@nadako
Copy link
Member

nadako commented Apr 17, 2020

I'm still not 100% convinced that we need all these settings in practice, but if we do then we should be consistent and also make "find implementations" configurable like that. Because right now when searching for implementations of an interface we also get indirect implementations.

@RealyUniqueName
Copy link
Member Author

I wonder if we should think ahead a bit and name it ReferencesKind

That sounds more like a property for each reference found.

@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Apr 17, 2020

I'm still not 100% convinced that we need all these settings in practice

Based on my experience with a big Haxe project development in IDEA I'm 100% sure these settings are useful in practice :)
Well, except maybe Direct setting, which is the current behavior of "find references"

@RealyUniqueName
Copy link
Member Author

HL failure is not related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-ide IDE / Editor support
Projects
None yet
4 participants