-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Conversation
@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. |
@iSazonov I don't understand what you mean about the old code being based on "safe evaluating". 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. |
@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. 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. |
@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: |
@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. |
This reverts commit d2c523e.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
📣 Hey @MartinGC94, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
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 ofGet-Random
.PR Context
Fixes #25205
Regression was added in #19830 and #21143
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).