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

Integrating class symbol support #16

Closed

Conversation

andyleejordan
Copy link

WIP: Attempting to integrate PowerShell#1886 with the changes previously made in PowerShell#1917.

@andyleejordan
Copy link
Author

@SeeminglyScience with the integration pretty much all the changes you had to make to the unit tests I reverted. Can you explain if any of those were intentional (beyond just "making them pass")?

Copy link
Author

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

My own questions for you both.

@andyleejordan
Copy link
Author

Hm ok so the sorting isn't quite right. Debugging FindsReferencesOnTypeConstraint and while all four references are found, they're out of order because the one used as the param type constraint has a StartOffset of 0.

@fflaten
Copy link
Owner

fflaten commented Jan 15, 2023

Currently shows variable-assignments for every parameter and re-assignments inside functions/methods in outline + workspace search. Feels messy compared to stable.

Maybe stick with current behavior for now? If so, it might require either of these:

  • Sticking with FindSymbolsVisitor for documentsymbols and workspacesymbols handlers.
  • Add another flag or bool in SymbolReference to control visibility in the handlers
  • Include a Parent-backlink in the SymbolReference we can filter on in handler (to keep only root-level). This is easier to achieve now with a single reference visitor and is probably required to support DocumentSymbol.children anyway, so I'd prefer this.

Comment on lines 266 to 271
if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))
{
return references;
}
Copy link
Owner

@fflaten fflaten Jan 15, 2023

Choose a reason for hiding this comment

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

Suggested change
if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))
{
return references;
}
if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))
{
return references.Where((r) =>
foundSymbol.SymbolType == r.SymbolType ||
(foundSymbol.SymbolType is SymbolType.Class or SymbolType.Enum or SymbolType.Type
&& r.SymbolType is SymbolType.Class or SymbolType.Enum or SymbolType.Type)
);
}

Filter on symbol type. Will currently match ex. a function and enum/class/type with same name.

@andyleejordan
Copy link
Author

  • Include a Parent-backlink in the SymbolReference we can filter on in handler (to keep only root-level). This is easier to achieve now with a single reference visitor and is probably required to support DocumentSymbol.children anyway, so I'd prefer this.

Same, that's what I was thinking.

@andyleejordan andyleejordan force-pushed the andschwa/better-symbols branch 2 times, most recently from 82ec11f to 604d6a7 Compare January 20, 2023 22:12
return AstVisitAction.Continue;
}

public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst methodCallAst)
Copy link
Owner

Choose a reason for hiding this comment

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

Which scenario is this for? I don't think you'll be able to match it to the correct method to count it as a reference.

Copy link
Author

Choose a reason for hiding this comment

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

So it turns out it works quite well! By bucketing all the overloads (of the same name) together, and returning a set of possible definitions, VS Code just does what it needs so you can do:

class SuperClass : BaseClass {
    [string]MyClassMethod([string]$param1, $param2, [int]$param3) {
        $this.SomePropWithDefault = 'something happend'
        $param1
        return 'finished'
    }

    [string]
    MyClassMethod([MyEnum]$param1) {
        return 'hello world'
    }
    [string]MyClassMethod() {
        return 'hello world'
    }
}

$o = [SuperClass]::new()
$o.MyClassMethod()

and find definition on MyClassMethod() shows the three potential matches:
Screenshot 2023-01-20 at 5 48 12 PM

No, it's not perfect, but it's a whole lot better than nothing!

Copy link
Owner

Choose a reason for hiding this comment

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

True, go to definition makes sense. 👍 You could also filter on argument count, but argument types won't work.

Copy link
Owner

Choose a reason for hiding this comment

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

To make it complete, should ::new() return the constructors? Because that's a reserved method when static (InvokeMemberExpressionAst.Static == true), right?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea!

To actually be an inclusive check of the given point being
between the two boundary points.
Space between name and subsequent parenthesis.
Let it be known that Patrick bets $500 that no one notices this.
And to be fair, searching the whole workspace now works great.
And delete "find details" for non-commands (as they all get their
display strings set on creation now).
The visitor exists but it's not called, probably a module-import issue.
andyleejordan and others added 12 commits January 27, 2023 18:33
Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>
So that we rely on the dictionary to sort our symbols into the equivalent
types instead of having to perform a filter.
I think there's a cleaner way to do this, but this works.
Against the full name and not just the identifier, since it's
what's displayed in the search menu.

Now you can search methods by their parameters' names.
Should have been just `IndexOf`, and also use `+` instead
of `string.Concat()` for simplicity and speed.
@andyleejordan andyleejordan deleted the andschwa/better-symbols branch February 2, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants