Skip to content

Fix infinite loop in variable type inference #25206

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

Merged
merged 4 commits into from
Mar 26, 2025

Conversation

MartinGC94
Copy link
Contributor

PR Summary

Fixes a crash due to an infinite loop when inferring the type of a variable that is being reused, like here: $x = Get-Random; $x = $x.Where{$_} where the last assignment of $x was considered the $x.Where... statement instead of Get-Random.

PR Context

Fixes #25205
Regression was added in #19830 and #21143

PR Checklist

@iSazonov
Copy link
Collaborator

@MartinGC94 I had a lot of doubts about this approach. On the one hand, it is very tempting to have it, users get useful features when editing scripts. On the other hand, this approach has many problems. This code is difficult to understand, has limitations in its capabilities, and most importantly, it is almost impossible to cover it with exhaustive tests. I expected users to report problems, but it turned out to be even worse - the first message was about a crash.

So I think we should revert these two PRs and use a different approach to realize these opportunities.
I think it is more correct to use the already used approach based on "safe evaluating".

@MartinGC94
Copy link
Contributor Author

@iSazonov I don't understand what you mean about the old code being based on "safe evaluating".
The old code was also finding and analyzing relevant asts, the only difference is that I use a custom visitor while the old approach used an AstSearcher. However, it's not like the custom visitor is fundamentally less safe than the old approach. I managed to make a similar infinite loop bug when updating the type inference for $_ in switch statements: #21221 and here I used the old loop approach.
I'm also using a very similar approach in the variable completion code and that has been running fine since June 2023: #19595

I think it would be a mistake to completely abandon a perfectly fine way to handle type inference for variables just because of one mistake that happened to cause a crash.

@iSazonov
Copy link
Collaborator

@MartinGC94 It's not just a single mistake or that this approach has been used before. The problem is how to prove the correctness of this code - although we added a lot of tests, we did not see this problem.
Moreover, using this approach in the past does not guarantee that it is the best one.

I have no intention of forcing you to rewrite all the code in this feature, I'm just expressing my opinion and I feel that the code is becoming more complex, although it was originally conceived as a simplified workaround that works on older computers twenty years ago. Now we have more powerful computers (and besides, we prefer the IDE to the command line). That's why I keep coming back to the idea that maybe it's time to do these calculations in a more advanced way.

@MartinGC94
Copy link
Contributor Author

@iSazonov What do you mean with safe eval and more powerful computers? We can't just execute all the previous statements to get the actual value because those statements could have side effects. A quick example would be: $VM = New-VM; $VM.<Tab>. where tab completing obviously shouldn't result in a new VM being created every time.
Static analysis that determines what command set a value, and then checking the possible output from that command seems like the only reasonable way to do this, but if you can think of a better way then feel free to share it.

@kborowinski
Copy link
Contributor

@MartinGC94 Here are my 2 cents. I'm a heavy terminal user, and the enhancements you've delivered have significantly improved my experience. I understand Ilya's point about proceeding with care, but from my perspective, the advantages are well worth it—even if minor bugs pop up along the way.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 24, 2025
@iSazonov iSazonov self-assigned this Mar 24, 2025
@iSazonov
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@iSazonov

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@iSazonov iSazonov merged commit dea739c into PowerShell:master Mar 26, 2025
38 of 40 checks passed
@github-project-automation github-project-automation bot moved this from PR In Progress to Done in PowerShell Team Reviews/Investigations Mar 26, 2025
Copy link
Contributor

microsoft-github-policy-service bot commented Mar 26, 2025

📣 Hey @MartinGC94, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Sysoiev-Yurii pushed a commit to Sysoiev-Yurii/PowerShell that referenced this pull request May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
Development

Successfully merging this pull request may close these issues.

Latest PowerShell build from main crashes on property completion
4 participants