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

CA2208 Instantiate argument exceptions correctly #58768

Closed
wants to merge 3 commits into from

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Jan 11, 2022

Fixes #45286

Blocking dotnet/msbuild#7187

@elachlan elachlan requested review from a team as code owners January 11, 2022 05:55
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 11, 2022
@@ -158,7 +163,7 @@ private void WriteFile()
case TargetLanguage.VB:
WriteLine("' <auto-generated />"); break;
default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if ExceptionUtilities.Unreachable is more suitable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I will make use of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ExceptionUtilities.UnexpectedValue is available here that might be good to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it isn't available, is it okay to add a using?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If just adding using Roslyn.Utilities; makes it available, then it's fine to use it.

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the occurrences in the build error for dotnet/msbuild#7187 are fixed here.

/home/vsts/.nuget/packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/ImmutableSegmentedList`1.cs(346,23): error CA2208: (NETCORE_ENGINEERING_TELEMETRY=Build) Call the ArgumentException constructor that contains a message and/or paramName parameter
/home/vsts/.nuget/packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/ImmutableSegmentedList`1+ValueBuilder.cs(249,27): error CA2208: (NETCORE_ENGINEERING_TELEMETRY=Build) Call the ArgumentException constructor that contains a message and/or paramName parameter

@elachlan elachlan marked this pull request as draft January 11, 2022 07:05
@elachlan
Copy link
Contributor Author

I don't see the occurrences in the build error for dotnet/msbuild#7187 are fixed here.

/home/vsts/.nuget/packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/ImmutableSegmentedList`1.cs(346,23): error CA2208: (NETCORE_ENGINEERING_TELEMETRY=Build) Call the ArgumentException constructor that contains a message and/or paramName parameter
/home/vsts/.nuget/packages/microsoft.codeanalysis.collections/4.0.0-4.21379.20/contentFiles/cs/netstandard2.0/ImmutableSegmentedList`1+ValueBuilder.cs(249,27): error CA2208: (NETCORE_ENGINEERING_TELEMETRY=Build) Call the ArgumentException constructor that contains a message and/or paramName parameter

Thanks, I am currently working through each of the problems. There are quite a few and I have a feeling some changes might break things. It might be easier if I only fix those things affecting MSBuild.

Comment on lines 11 to 13
# Instantiate argument exceptions correctly
dotnet_diagnostic.CA2208.severity = warning

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could move to Shipping.globalconfig to limit the analysis scope.

@@ -51,7 +51,7 @@ public override bool HasExplicitReturnType(out RefKind refKind, out TypeWithAnno
public override RefKind RefKind(int index) { return Microsoft.CodeAnalysis.RefKind.None; }
public override MessageID MessageID { get { return MessageID.IDS_FeatureQueryExpression; } } // TODO: what is the correct ID here?
public override Location ParameterLocation(int index) { return _parameters[index].Locations[0]; }
public override TypeWithAnnotations ParameterTypeWithAnnotations(int index) { throw new ArgumentException(); } // implicitly typed
public override TypeWithAnnotations ParameterTypeWithAnnotations(int index) { throw new ArgumentException(null, nameof(index)); } // implicitly typed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new ArgumentException(null, nameof(index));

It feels like an "Unreachable" exception is more appropriate here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ArgumentExceptions where parameterName is sent as message
4 participants