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 rule to catch syntax error e.g. "elif" instead of "elseif" #1299

Open
agowa opened this issue Jul 30, 2019 · 8 comments
Open

Add new rule to catch syntax error e.g. "elif" instead of "elseif" #1299

agowa opened this issue Jul 30, 2019 · 8 comments

Comments

@agowa
Copy link

agowa commented Jul 30, 2019

Summary of the new feature

Try formulating it in user story style (if applicable):
'As a python and powershell developer I want ScriptAnalyzer to detect when I use elif (python) instead of elseif (powershell) so that the ci warns me about invalid statements and I can correct the code.'

Proposed technical implementation details (optional)

For powershell modules we need an additional ci check. Currently syntax errors are not always covered. The ci for example does currently not cover usage of elif instead of elseif (former is python later is powershell).

Example shippables: https://app.shippable.com/github/ansible/ansible/runs/133592/1/console (source)
It erroneously reports unused variables instead of a syntax error.

What is the latest version of PSScriptAnalyzer at the point of writing

@bergmeister
Copy link
Collaborator

bergmeister commented Jul 30, 2019

Hmm, that' interesting. PSScriptAnalyzer returns ParserErrors in general. In this special case, this is valid PowerShell syntax as PowerShell's parser interprets elif as a command and you get a relatively good runtime error at least. Therefore I think PSSA is the right place for a rule for this special case where it is highly unlikely someone has a command named elif.
image

@msftrncs
Copy link

I think it may be overreaching to try to catch these kinds of errors in a scripting language, because anything that is not a keyword, is a potentially valid command (native or otherwise). The best solution for this is syntax highlighting! Shell scripting languages probably have the most to gain from syntax highlighting compared to other formal programming languages, as in formal programming languages, any string of characters can usually be fully characterized and accounted for, linting errors as required, but in a shell script, a string of characters may represent a command and that command may not be accessible from the system editing the script.

(Demo of PowerShell/EditorSyntax#156 in VS Code)
image

@agowa
Copy link
Author

agowa commented Aug 23, 2019

Why not just add a check for calling a function right after a closing bracket than?
Shouldn't there according to style guides be a semicolon or line break right after the closing bracket?

I think script analyzer should at least pick up if someone uses keywords for function names like else in your example or if multiple functions are called one after another without a semicolon.

Or is there a legitimate use case for if ($true) {Do-Stuff} Start-Confusion {} Do-MoreConfusion {} that I'm overlooking?

@bergmeister
Copy link
Collaborator

@msftrncs Has a point but I'd also be happy to have a scriptanalzer rule in general as well. The likelihood that someone names a function of binary elif is quite low and I think it's not just Python developers that might make that mistake, some C shells on Unix also use elif for else if.
I suggest to look immediately after an if statement though. Id be happy to accept such a rule, it has low priority for me though, would you be happy to submit a PR @agowa338

@msftrncs
Copy link

@agowa338, remember again, that the language doesn't restrict a person from reusing keywords as functions, the primary reason being, as a scripting shell, it never can no what might have already been created. I could have a set of native tools that share names with keywords.

On the other hand, if one wanted to add a check for a user specified list of commands that should be flagged for errors(warnings), that would work.

It already should be possible to use style to catch this as you mention, but I am not getting that to work. When I attempt to format the example above, I do not get the expected result of the elif being forced on to a new line, even though my settings (in VS Code) suggest that would happen. I have the setting for new line after closing brace, but it doesn't happen.

@RobertKlohr
Copy link

I think this is the best place to add the issue I just came across if not move to new issue as appropriate.

Environment:

ModuleType  Version  Name
----------  -------  ----
Script      1.18.3   PSScriptAnalyzer

I just spent a few hours troubleshooting (custom error checking was masking the root cause - lesson learned there!) a script that passed PSSA but failed to execute. The error stemmed from pasting a scope snippet on the wrong line such that the closing bracket } for the if statement came after else instead of before it.

This seems like the kind of error that PSSA would highlight. I never thought that creating functions or variables that matched keywords would be valid syntax as mentioned previously in the thread. Even if this is technically valid code I think this is the kind of issue that PSSA should highlight, i.e. if it finds a keyword where is expects a functions or variable, etc. The logic and precedence for this type of checking would be the same as the rule that checks for aliases; it makes the code harder to read and understand and may cause issues when used with other scripts or modules.

Below are the valid and invalid scripts.

Valid Code:
function Test-GoodCode { $params = @{ } if ($true) { $params.Add('key', 'value') } else { $params.Add('key', 'value2') } return $params }

Expected Output:

Name          Value
----          -----
key           value

Invalid Code:
function Test-BadCode { $params = @{ } if ($true) { $params.Add('key', 'value') else { $params.Add('key', 'value2') } } return $params }

Error from Test-BadCode

else : The term 'else' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the    
path is correct and try again.
At C:\temp\bugs.ps1:15 char:9
+         else {
+         ~~~~
    + CategoryInfo          : ObjectNotFound: (else:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

@bergmeister
Copy link
Collaborator

bergmeister commented Feb 25, 2020

@RobertKlohr The error that you see stems from PowerShell's parser, on which PSSA depends on, therefore the best it can do is propagate those parser errors back to the user, which is what it does. Even at runtime, the parser cannot make a better judgement on what is wrong here and even more polished languages like C# behave like this if a brace/parenthesis is missing by displaying a lot of follow-up errors. Therefore in this case, all I can suggest is try to dig deep in PowerShell parsers logic and try to improve it but for your specific case, PSSA cannot do much unfortunately

image
image

@RobertKlohr
Copy link

@bergmeister Thanks for the explanation and insight into the root cause. I learned something new today!

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

No branches or pull requests

5 participants