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

Keep only first assignment as declaration #1989

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

andyleejordan
Copy link
Member

As noted by @fflaten in PowerShell/vscode-powershell#1465 (comment), our initial logic counted every assignment as a declaration. While there's no true way to determine which is really the declaration, we can make a relatively safe assumption that the first encountered declaration is it. This is efficient, easy, and seems to work quite well.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

I'm cool with this since there isn't really a good solution without some sort of flow analysis work. Just want to put it on record that we know this will miss a lot of stuff in large files, but that we think it'll be better to do that than to list every single assignment.

For example, folks building one large psm1 with all their functions, if they reuse variable names we'll miss all but the first one.

(ofc please correct me if I'm wrong)

@andyleejordan
Copy link
Member Author

Also quoting a great example from @fflaten in PowerShell/vscode-powershell#1465 (comment):

I believe that would make the inner assignment the definition in the example below.

Function abc {
    $myvar = 'inner'
}

$myvar = 'outer'

Maybe track the first assignment per parent (when that is tracked)?

He's absolutely right that's going to happen, but we can't improve it until we have parent/child relationship tracking (and even then it might prove tricky).

@andyleejordan
Copy link
Member Author

andyleejordan commented Feb 7, 2023

I think the most annoying bit will be that for a commonly re-used symbol name like $i, if you jump to definition on a local use of it, instead of showing you all the potential definitions, it'll just take you to the very first one.

That one has me thinking that maybe we should just filter for outline view?

Edit: I don't think that'll gain much and will be a lot trickier.

@andyleejordan andyleejordan merged commit 368dcfa into main Feb 7, 2023
@andyleejordan andyleejordan deleted the andschwa/first-assignment branch February 7, 2023 21:16
@fflaten
Copy link
Contributor

fflaten commented Feb 7, 2023

Just want to put it on record that we know this will miss a lot of stuff in large files, but that we think it'll be better to do that than to list every single assignment.

This 👍. Before this PR, open ex. Pester.psm1 and get overwhelmed by variables in outline and go to symbol.

That one has me thinking that maybe we should just filter for outline view?

Yes. I could see a value in listing all possible definitions, but outline and symbol search get easily crowded.

Something to revisit later :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants