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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions eng/config/globalconfigs/Shipping.globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ is_global = true
dotnet_diagnostic.CA1802.severity = warning
dotnet_diagnostic.CA2007.severity = warning

# Instantiate argument exceptions correctly
dotnet_diagnostic.CA2208.severity = warning

dotnet_diagnostic.RS0005.severity = warning

# CA2016: Forward the 'CancellationToken' parameter to methods
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.


public override void GenerateAnonymousFunctionConversionError(BindingDiagnosticBag diagnostics, TypeSymbol targetType)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private void VerifySpanForGetDiagnostics(TextSpan? span)
{
if (span.HasValue && !this.Root.FullSpan.Contains(span.Value))
{
throw new ArgumentException("span");
throw new ArgumentException(null, nameof(span));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ internal sealed class BoundNodeClassWriter

private BoundNodeClassWriter(TextWriter writer, Tree tree, TargetLanguage targetLang)
{
if (!Enum.IsDefined(targetLang))
{
throw new ArgumentException($"Unexpected target language: '{_targetLang}'", nameof(targetLang));
}

_writer = writer;
_tree = tree;
_targetLang = targetLang;
Expand Down Expand Up @@ -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.

}

Blank();
Expand Down Expand Up @@ -210,7 +215,7 @@ private void WriteUsing(string nsName)
case TargetLanguage.VB:
WriteLine("Imports {0}", nsName); break;
default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -227,7 +232,7 @@ private void WriteStartNamespace()
Indent();
break;
default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -243,7 +248,7 @@ private void WriteEndNamespace()
WriteLine("End Namespace");
break;
default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -269,7 +274,7 @@ private void WriteKinds()
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -319,7 +324,7 @@ private void WriteClassHeader(TreeType node)
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -337,7 +342,7 @@ private void WriteClassFooter(TreeType node)
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -381,7 +386,7 @@ private void Or<T>(IEnumerable<T> items, Func<T, string> func)
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -559,7 +564,7 @@ private void WriteConstructorWithHasErrors(TreeType node, bool isPublic, bool ha
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -666,7 +671,7 @@ private void WriteConstructorWithoutHasErrors(TreeType node, bool isPublic)
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -817,7 +822,7 @@ private NullHandling FieldNullHandling(TreeType node, string fieldName)
case "":
break;
default:
throw new ArgumentException("Unexpected value", nameof(f.Null));
throw new ArgumentException($"Field '{fieldName}' on node '{node.Name}' has an unexpected value for attribute '{nameof(f.Null)}'");
}
}

Expand Down Expand Up @@ -888,7 +893,7 @@ private void WriteField(TreeType node, Field field)
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -912,7 +917,7 @@ private void WriteAccept(string name)
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1024,7 +1029,7 @@ private void WriteUpdateMethod(Node node)
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}

string wasUpdatedCheck(Field field)
Expand Down Expand Up @@ -1164,7 +1169,7 @@ private void WriteVisitor()
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1222,7 +1227,7 @@ private void WriteWalker()
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1350,7 +1355,7 @@ private void WriteTreeDumperNodeProducer()
break;

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1446,7 +1451,7 @@ private void WriteRewriter()
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1636,7 +1641,7 @@ static bool typeIsUpdated(string type)
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -1656,7 +1661,7 @@ private bool IsImmutableArray(string typeName, out string elementType)
{
TargetLanguage.CSharp => "ImmutableArray<",
TargetLanguage.VB => "ImmutableArray(Of ",
_ => throw new InvalidOperationException($"Unknown target language {_targetLang}")
_ => throw new InvalidOperationException($"Unknown target language '{_targetLang}'")
};

if (typeName.StartsWith(immutableArrayPrefix, StringComparison.Ordinal))
Expand All @@ -1680,7 +1685,7 @@ private bool IsNodeList(string typeName)
return typeName.StartsWith("IList(Of", StringComparison.OrdinalIgnoreCase) ||
typeName.StartsWith("ImmutableArray(Of", StringComparison.OrdinalIgnoreCase);
default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1712,7 +1717,7 @@ private string GetGenericType(string typeName)
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1747,7 +1752,7 @@ private string GetElementType(string typeName)
}

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down Expand Up @@ -1834,7 +1839,7 @@ private string FixKeyword(string name)
return "[" + name + "]";

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -1852,7 +1857,7 @@ private bool IsKeyword(string name)
return name.IsVBKeyword();

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand All @@ -1867,7 +1872,7 @@ private string GetVisitFunctionDeclaration(string nodeName, bool isOverride)
return $"Public {(isOverride ? "Overrides" : "Overridable")} Function Visit{StripBound(nodeName)}(node As {nodeName}) As BoundNode";

default:
throw new ArgumentException("Unexpected target language", nameof(_targetLang));
throw new InvalidOperationException($"Unexpected target language: '{_targetLang}'");
}
}

Expand Down