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

Add new StrictMode version for confusing operator precedence #18134

Open
dkaszews opened this issue Sep 20, 2022 · 4 comments
Open

Add new StrictMode version for confusing operator precedence #18134

dkaszews opened this issue Sep 20, 2022 · 4 comments
Labels
Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group.

Comments

@dkaszews
Copy link
Contributor

dkaszews commented Sep 20, 2022

Summary of the new feature / enhancement

There were multiple discussions about PowerShell's confusing operator order:

> 1 -or 1 -and 0  # expected true like in every programming language
false
> 1, 2+3, 4  # expected @(1, 5, 4) like in Python
1
2
3
4

Just today @SteveL-MSFT accidentally found another example with + and , in #18003 :

> @('a'+'b', 'c'+'d')  # expected @('ab', 'cd') like in Python
ab cd

If even "Principal Software Engineering Manager for PowerShell" (sorry Steve for dragging you into it) cannot get it right, then surely something has to be done.

My proposal to "fix" it without an enormous breaking change of changing operator precedence is to add a new StrictMode version, which emits and error any time a "confusing" operator precedence is encountered without parentheses. Since StrictMode versions are opt-in, this is not a breaking change. I believe this to be a perfect solution, as user simply has to add parentheses to make it explicit what order they expect:

> 1 -or 1 -and 0
Error
> 1 -or (1 -and 0)  # expected behavior
1
> (1 -or 1) -and 0  # current behavior
0
> 0 -and 1 -or 1  # no parentheses required, as both behaviors produce same result
1

Full list of which combination of operators in what order should emit the error is to be determined, but can be gathered from older issues and https://github.com/nightroman/PowerShellTraps

Proposed technical implementation details (optional)

Implementation should have as little performance impact as possible. I hope that simple pattern matching on the AST should be enough, e.g. and/or confusion should be detectable by checking for the pattern x -or y -and z (x -and y -or z is not an issue, as it always executes and first, both in PowerShell's same-precendence and other languages' and-before-or).

If possible, it should be done at parsing stage, to also detect code in not-taken branches.

@dkaszews dkaszews added Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group. labels Sep 20, 2022
@MartinGC94
Copy link
Contributor

I don't think this should be a strict mode feature. It's better as a script analyzer rule so editors like VS code can warn you before you attempt to run the script.

@dkaszews
Copy link
Contributor Author

@MartinGC94 That would limit the feature only to the particular editor. I believe that a shell language is a bit different usage, as a system admin may quickly edit a script in some basic text editor, or somebody may run couple lines of code directly from terminal, without creating a script first. Personally I was bitten by and/or when trying to look up some files with a Select-Object oneliner. If it's possible to integrate those checks into parser, I see no reason not to do it.

What I am questioning instead is whether a single version setting is good enough. As we add more checks, it is more and more difficult to get them all correct. If somebody wants to use a check from version N, they need to first fulfill all of the previous ones. Maybe moving to a flag system would be a good idea, so that if you don't like this particular check but want to use future ones, you could just do Set-StrictMode -Version 7 -Exclude 'OperatorPrecedence'.

@237dmitry
Copy link

You can always read help topic about_Operator_Precedence.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 15, 2023
@dkaszews
Copy link
Contributor Author

Still an issue, shut up bot

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Resolution-No Activity Issue has had no activity for 6 months or more label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group.
Projects
None yet
Development

No branches or pull requests

3 participants