Add new InvalidMultiDotValue rule#2180
Conversation
iRon7
requested review from
a team,
bergmeister,
michaeltlombardi and
sdwheeler
as code owners
|
👋 Looks useful, thanks! The naming of this rule is a little awkward. It's listed as This rule has a severity of In your tests, you have 2 You are using the temp folder in some of your testing; I'd suggest looking into and using Pester's Test Drive for temporary file usage during tests. It helpfully takes care of relevant cleanup for you too at the right time. I don't think the This has the same copy-pasta as your other PRs. |
iRon7
changed the title
|
I am trying to be precise but it is amazing how many issues you still find in PRs. |
Fixed, although I have kept the questionable name
Fixed
Fixed
Removed
Changed accordingly |
Co-authored-by: Copilot <copilot@github.com>
iRon7
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new built-in PSScriptAnalyzer rule to detect unquoted literals containing multiple dots (which PowerShell can parse as member-access on a double and end up evaluating to $null), along with documentation and test coverage.
Changes:
- Introduces the
InvalidMultiDotValuerule implementation with a-Fixsuggestion to quote the multi-dot value. - Adds localized rule strings and new rule documentation.
- Adds Pester tests covering violations, compliant cases, suppressions, and fix behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Rules/README.md | Registers the new rule in the rules index table. |
| docs/Rules/InvalidMultiDotValue.md | New rule documentation with examples. |
| Tests/Rules/InvalidMultiDotValue.tests.ps1 | New test suite for detection, suppression, and -Fix. |
| Rules/Strings.resx | Adds name/description/error/correction strings for the rule. |
| Rules/InvalidMultiDotValue.cs | Implements AST-based detection and suggested correction extents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Thanks @iRon7 — and thanks @liamjpeters for the thorough review. The rename of the markdown title, the dropped CompilerServices import, the dedup'd Context "Suppressed" block, and the severity alignment are all in. This is the closest to ready-to-merge of the four.
Two small things before I'm comfortable merging, plus one open question:
The if (invalidAsts != null) guard around the foreach in Rules/InvalidMultiDotValue.cs is dead code. Ast.FindAll never returns null — it always returns at least an empty enumerable per the .NET API. The foreach will simply not iterate when there are no matches, so the conditional can be removed and the body un-indented. Cosmetic but worth cleaning up while we're here.
Opt-in by default. Same point as the other rules: this is currently IScriptRule, which means enabled-by-default. Even though this is a clear error case (invalid construction that produces $null), our convention for new rules is opt-in via ConfigurableRule with Enable = false, so users get to evaluate impact on existing codebases before flipping it on.
Open question — Liam suggested TestDrive instead of the manual temp folder in Tests/Rules/InvalidMultiDotValue.tests.ps1, but that thread didn't conclude. Are you good with that change, or do you have a reason to prefer the explicit temp folder? Either way is fine, just want to close the loop on Liam's suggestion.
Drafted by Copilot (Claude Opus 4.7)
|
Thank you for your feedback,
I will change the rule and derive it from
Thanks, I missed that (too much PowerShell ballast and just starting with C# 😃)
This is already implemented: |
The problem is that, for better or worse, a lot of people have enabled PSSA with default rules set to on in their CI systems, and so if new rules show up with warnings their builds fail and we get yelled at. Should they have done that? No. But it does mean that for as long as we're doing minor releases (which get you a new package with your rules usable) then the net effect of taking said minor release should be no new warnings/errors. When we do a major release we get to break what's become the equivalent of an API promise and go and enable by default all the great new rules. So yes, good new rules like this will eventually land as enabled by default, but it's a much bigger project for us to go through and do that and make a major release. I'd like for that to happen, we'll just have to get it on the docket.
Don't thank me, thank Claude!
Don't blame me, blame Claude! 🤠 |
| PowerShell does not support an implicit value with multiple dots. | ||
| Any *unquoted* value with 2 or more dots will not be treated as any special type (like a `version` or `IPAddress`) | ||
| but result in `$null`. These objects need to be constructed from either a quoted string (e.g. `[Version]'1.2.3'`) | ||
| or their individual components (e.g. `[Version]::new(1, 2, 3)`). |
There was a problem hiding this comment.
| PowerShell does not support an implicit value with multiple dots. | |
| Any *unquoted* value with 2 or more dots will not be treated as any special type (like a `version` or `IPAddress`) | |
| but result in `$null`. These objects need to be constructed from either a quoted string (e.g. `[Version]'1.2.3'`) | |
| or their individual components (e.g. `[Version]::new(1, 2, 3)`). | |
| PowerShell doesn't support unquoted literal values with multiple dots (`.`). Any value with two or | |
| more dots results in `$null`. This rule identifies instances where such values are used, which can | |
| lead to unexpected behavior or errors in the code. | |
| To create values of the intended type, enclose the value in quotes and use type-casting or use type | |
| constructor methods to create the appropriate object. |
| $version = [Version]'1.2.3' | ||
| # or: | ||
| $version = [Version]::new(1, 2, 3) | ||
| ``` | ||
|
|
||
| ```powershell | ||
| $IP = [System.Net.IPAddress]'127.0.0.1' |
There was a problem hiding this comment.
| $version = [Version]'1.2.3' | |
| # or: | |
| $version = [Version]::new(1, 2, 3) | |
| ``` | |
| ```powershell | |
| $IP = [System.Net.IPAddress]'127.0.0.1' | |
| # Use type-casting with quoted value | |
| $IP = [System.Net.IPAddress]'127.0.0.1' | |
| $version = [Version]'1.2.3' | |
| # Use type constructor method | |
| $version = [Version]::new(1, 2, 3) |
PR Summary
Issue: #1698
PowerShell does not support an implicit value with multiple dots.
Any unquoted value with 2 or more dots will not be treated as any special type (like a
versionorIPAddress)but result in
$null. These objects need to be constructed from either a quoted string (e.g.[Version]'1.2.3')or their individual components (e.g.
[Version]::new(1, 2, 3)).This rule returns an Error for unquoted values that have 2 or more dots (e.g.:
$version = [Version]1.2.3).And implements a
-Fixto surround the concerned violation with single quotes.PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.