ensure that TryParse is used for all user input in editorconfig #17097
Conversation
|
@dotnet/roslyn-ide please review |
| @@ -27,6 +27,10 @@ public bool TryParseReadonlyDictionary(IReadOnlyDictionary<string, object> allRa | |||
| if (allRawConventions.TryGetValue(KeyName, out object value)) | |||
| { | |||
| result = _parseValue(value.ToString(), type); | |||
| if (result == null) | |||
| @@ -49,11 +53,19 @@ public EditorConfigStorageLocation(string keyName) | |||
| { | |||
| if (type == typeof(int)) | |||
| { | |||
| return int.Parse(s); | |||
| if (int.TryParse(s, out var value)) | |||
There was a problem hiding this comment.
return int.TryParse(...) ? value : null;
| { | ||
| return value; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
what does null mean for an int? should this be 0?
There was a problem hiding this comment.
null means we couldn't parse it so return the default value.
In reply to: 102832529 [](ancestors = 102832529)
| { | ||
| return value; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
should this be false? or does 'null' just mean 'use the default'?
There was a problem hiding this comment.
jmarolf
force-pushed
the
bugfix/do-not-throw-exceptions-on-bad-editorconfig-files
branch
from
0702356
to
f68b04c
Compare
|
retest windows_release_unit64_prtest please |
| @@ -12,79 +12,79 @@ internal static class CSharpCodeStyleOptions | |||
| public static readonly Option<CodeStyleOption<bool>> UseImplicitTypeForIntrinsicTypes = new Option<CodeStyleOption<bool>>( | |||
| nameof(CodeStyleOptions), nameof(UseImplicitTypeForIntrinsicTypes), defaultValue: CodeStyleOption<bool>.Default, | |||
| storageLocations: new OptionStorageLocation[] { | |||
| new EditorConfigStorageLocation("csharp_style_var_for_built_in_types", ParseEditorConfigCodeStyleOption), | |||
| new EditorConfigStorageLocation("csharp_style_var_for_built_in_types"), | |||
There was a problem hiding this comment.
Move ParseEditorConfigCodeStyleOption closer to EditorConfigStorageLocation.
Also consider caching the delegate for the parse function used in the 1-arg version of the EditorConfigStorageLocation constructor
jmarolf
deleted the
bugfix/do-not-throw-exceptions-on-bad-editorconfig-files
branch
We got several reports in RC of Parse(int) throwing exceptions. This PR changes the editorconfig parsing of options to only use TryParse. It also simplifies several cases where we were passing a parsing function unnecessarily.