Use correct exceptions

Summary of the new feature / enhancement

In PowerShell code base there are some places where we are not accurate with exceptions.
Typical example:

PowerShell/src/System.Management.Automation/engine/CommandInfo.cs

Lines 286 to 294 in 4314e63

    
           internal void Rename(string newName) 
        
           { 
        
               if (string.IsNullOrEmpty(newName)) 
        
               { 
        
                   throw new ArgumentNullException(nameof(newName)); 
        
               } 
        
               Name = newName; 
        
           }

Here we throw ArgumentNullException that is not correct if the argument is empty. (There are examples with other exceptions.)

My proposal is to fix such code and throw ArgumentNullException if argument is null and ArgumentException is argument is empty.

This is not even a breaking change, as it is not a functional exception, and it is worth correcting it for the correct one. If someone even uses this behavior in their code, it is catastrophically bad code.

The proposal comes from the fact that we are blocked from using new .Net API like ArgumentException.ThrowIfNullOrEmpty(). The API makes code more friendly for .Net Rumtime optimizations (like inlining). We already use this kind of API in a lot of places and it's worth updating the rest.

Proposed technical implementation details (optional)

No response

There is no argument around using ArgumentException.ThrowIfNullOrEmpty() when it's applicable, and #19197 was already merged in, which does exactly this kind of changes.

So, removing the Review - Maintainer label, and marking Up-for-Grabs.

NullOrEmpty is the most common, but there are others (ex., where Count or Length == 0). (Better to search by throw new Argument pattern.)

It's better not mix all together.

For cases where the existing ThrowIfXXX APIs fit properly, it's generally fine to accept the change.
For cases where the existing ThrowIfXXX APIs don't fit, for example, when checking if a collection is null or empty, or when checking if an argument or one of its members is null, we should not apply the ThrowIfXXX APIs blindly. For those cases, please use a different issue to describe the intended changes, for maintainers to review.

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Navigation Menu

Repository navigation

Use correct exceptions #19212

Summary of the new feature / enhancement

Proposed technical implementation details (optional)

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	internal void Rename(string newName)
	{
	if (string.IsNullOrEmpty(newName))
	{
	throw new ArgumentNullException(nameof(newName));
	}

	Name = newName;
	}

Use correct exceptions #19212

Description

Summary of the new feature / enhancement

Proposed technical implementation details (optional)

Activity

daxian-dbw commented on Feb 24, 2023

iSazonov commented on Feb 24, 2023

daxian-dbw commented on Feb 24, 2023

xtqqczze commented on Feb 24, 2023

microsoft-github-policy-service commented on Nov 15, 2023

microsoft-github-policy-service commented on Nov 15, 2023

microsoft-github-policy-service commented on Nov 15, 2023

microsoft-github-policy-service commented on Nov 22, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions