★ wanayoo — archive 1999 https://github.com/PowerShell/PSScriptAnalyzer/pull/1348Nouvelle recherche | Portail wanayoo
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

New rule: AvoidOverwritingBuiltInCmdlets #1348

Merged
merged 26 commits into from Dec 9, 2019

Conversation

@thomasrayner
Copy link
Contributor

thomasrayner commented Oct 2, 2019

PR Summary

Addresses #1023 .

This rule (PSAvoidOverwritingBuiltInCmdlets) looks for FunctionDefinitionAsts that have the same name as a cmdlet that comes with PowerShell. This rule leverages the JSON files which contain information on all the cmdlets for PS 3, 4, 5, and 6 (and presumably 7 later) that already ship with PSScriptAnalzyer.

I've edited GetScriptAnalyzerRule.tests.ps1 to reflect the addition of this new rule, but suspect that if the other rule I recently created a PR for is merged before this one, that I'll need to edit that test again.

PR Checklist

@thomasrayner
Copy link
Contributor Author

thomasrayner commented Oct 2, 2019

It appears that a whole bunch of tests are busted but I'm not sure it's a result of the changes in this PR, however, I haven't dug that deep into them on the assumption that this is already known.

@bergmeister
Copy link
Collaborator

bergmeister commented Oct 4, 2019

Please look into the test failures, they are definitely caused by your code as I see a lot of exceptions of type ArgumentNullException that likely comes from your rule as it is being run by default. What kind of performance impact does running this rule by default have (cold and warm run)? I've just merged your other PR, so please resolve the merge conflict as well (and you probably need to re-generate the resource strings as well).
The JSON files that you use are from an old compatibility rule, it is probably OK for this use case but maybe @rjmholt can make a statement whether his compatibility profiles contain more commands? They are here btw: https://github.com/PowerShell/PSScriptAnalyzer/tree/master/PSCompatibilityCollector/profiles

@rjmholt
Copy link
Member

rjmholt commented Oct 5, 2019

@rjmholt can make a statement whether his compatibility profiles contain more commands?

If it's just builtin commands, I would imagine the old profiles have the same information (a list of those commands).

Depending on the full compatibility profiles would be a resource drain. In a case like this, since you only need a list, either using those old profiles or just storing a list in the rule's code will probably suffice for now.

@rjmholt
Copy link
Member

rjmholt commented Oct 5, 2019

Ah but I just saw it's a configuration thing... Hmmm, not sure what the right answer is there, since it's public, so we don't want to break it in future. Those old profiles aren't about to get any updates as far as I know, so it's not ideal to depend on them publicly.

It might be that it's time to solve the question of how to specify a PowerShell platform nicely...

@bergmeister
Copy link
Collaborator

bergmeister commented Oct 5, 2019

The reason why those old command data files contain less cmdlets is because of the way they are generated here: https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Utils/New-CommandDataFile.ps1

Rules/AvoidOverwritingBuiltInCmdlets.cs Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
}


private HashSet<string> GetCmdletsFromData(dynamic deserializedObject)

This comment has been minimized.

@rjmholt

rjmholt Oct 5, 2019 Member

This is the other disadvantage of the old profiles; they don't have a formalised schema. The more efficient way to do this would be to get the JObject instead though.

This comment has been minimized.

@thomasrayner

thomasrayner Oct 16, 2019 Author Contributor

@rjmholt do you recommend I change it? Perpetuating legacy practices isn't high on my to-do list, but I'm wondering how far removed from the desired practice it is.

@thomasrayner thomasrayner force-pushed the thomasrayner:avoidoverwritingcmdlets branch from 1d46ca4 to aeea36c Oct 16, 2019
@thomasrayner thomasrayner force-pushed the thomasrayner:avoidoverwritingcmdlets branch from 07825ce to 6fb31b6 Oct 16, 2019
@thomasrayner
Copy link
Contributor Author

thomasrayner commented Oct 16, 2019

Obviously I've got more to do here, like dig into those test failures, but I wanted to get some of these changes back to you folks to discuss.

@thomasrayner
Copy link
Contributor Author

thomasrayner commented Oct 23, 2019

I took a shot at fixing the test errors that were definitely my fault 😂, contrary to my original thought. The four remaining tests that fail are for the UseCompatibleCommands rule test, and only appear to occur on the Ubuntu test env.

@bergmeister
Copy link
Collaborator

bergmeister commented Oct 23, 2019

@thomasrayner Thanks, I will look at the PR this weekend. You are right, the 4 failing Ubuntu tests are a new integration failure on master caused by the last merged PR #1331 (@rjmholt please fix them please). The sporadically failing test around runspace disposal, you can ignore as well (the maximum number needs increasing).
Most of it looks ok but I can already tell you we'll need to have a conversation around that test process stuff (sorry, but just wrong on so many levels IMHO). First of all, the user should be explicit in the settings and second, this is how you can determine in which version of PowerShell you are currently running in order to determine a useful default

bool runningInPowerShellCore = false;
#if CORECLR
runningInPowerShellCore = true;
#endif
@thomasrayner
Copy link
Contributor Author

thomasrayner commented Oct 23, 2019

Well, I'm not sure how I missed that, but I'm looking forward to changing it 😁

Copy link
Collaborator

bergmeister left a comment

Just some minor comments to be discussed but otherwise I'm pretty happy with it and I've done a quick performance test and can say that this does not seem to have an impact on PSSA performance despite it being enabled by default

RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
Rules/Strings.resx Outdated Show resolved Hide resolved
Copy link
Collaborator

bergmeister left a comment

Some more minor comments but after that I'm happy to sign it off. What do you think @rjmholt

RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Outdated Show resolved Hide resolved
RuleDocumentation/AvoidOverwritingBuiltInCmdlets.md Outdated Show resolved Hide resolved
Rules/AvoidOverwritingBuiltInCmdlets.cs Outdated Show resolved Hide resolved
thomasrayner and others added 2 commits Oct 29, 2019
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
Copy link
Collaborator

bergmeister left a comment

I'm happy, what about you @rjmholt ?

Christoph Bergmeister added 2 commits Nov 19, 2019
@bergmeister
Copy link
Collaborator

bergmeister commented Dec 3, 2019

I'm happy, what about you @rjmholt ? I'd merge it if you're happy.
You don't need to do a full review, just checking you're happy with it on a high level.

@rjmholt
rjmholt approved these changes Dec 9, 2019
Copy link
Member

rjmholt left a comment

LGTM

bergmeister added 2 commits Dec 9, 2019
@bergmeister
Copy link
Collaborator

bergmeister commented Dec 9, 2019

Still looks good to me, I resolved the merge conflict. The test failures on Ubuntu are new in master as well @rjmholt ...

@bergmeister bergmeister merged commit ab69069 into PowerShell:master Dec 9, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.