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.

Sign up for GitHub

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

Nullable ref type annotation fixes to analyzer APIs #43023

Merged
merged 2 commits into from
Apr 8, 2020
Merged

Nullable ref type annotation fixes to analyzer APIs #43023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c99b069
Fix Diagnostic.Create nullable ref annotations
AArnott Apr 2, 2020
0041de4
Allow null property values in Diagnostics
AArnott Apr 2, 2020
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
2 changes: 1 addition & 1 deletion src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private void AnalyzeOperation(OperationAnalysisContext context)
!tree.OverlapsHiddenPosition(switchBlock.Span, context.CancellationToken))
{
Debug.Assert(missingCases || missingDefaultCase);
var properties = ImmutableDictionary<string, string>.Empty
var properties = ImmutableDictionary<string, string?>.Empty
Copy link
Member

Choose a reason for hiding this comment

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

hrmm... who passes null for the value. I'd prefer we attempt to disallow that. If you make this non-null, can you point to any violators?

Copy link
Contributor

@mavasani mavasani Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we can restrict the values for the property bag to be non-null. I think I have seen some third party analyzers using null as a value for the property bag entry.

.Add(PopulateSwitchStatementHelpers.MissingCases, missingCases.ToString())
.Add(PopulateSwitchStatementHelpers.MissingDefaultCase, missingDefaultCase.ToString());

Expand Down
8 changes: 4 additions & 4 deletions src/Analyzers/Core/CodeFixes/PopulateSwitch/AbstractPopulateSwitchCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ public sealed override Task RegisterCodeFixesAsync(CodeFixContext context)
{
var diagnostic = context.Diagnostics.First();
var properties = diagnostic.Properties;
var missingCases = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingCases]);
var missingDefaultCase = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingDefaultCase]);
var missingCases = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingCases]!);
Copy link
Member

Choose a reason for hiding this comment

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

! [](start = 97, length = 1)

We typically reserve the ! suppressions for things that are checked in the current method (directly or via straightforward helper method) but the compiler is unable to track.
For more complicated invariants, we use assertions instead (Debug.Assert(... is object); or is { } depending on preference).
These two (and two more below in this file) strike me as cases for assertions.

https://github.com/dotnet/roslyn/blob/master/docs/contributing/Nullable%20Annotations.md

Copy link
Member

Choose a reason for hiding this comment

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

An assert seems unnecessary here since bool.Parse() will throw an exception if the argument is null.

In reply to: 405655126 [](ancestors = 405655126)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya. The ! was just to keep the compiler warning quiet so that the original behavior of throwing from bool.Parse would be preserved.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks

var missingDefaultCase = bool.Parse(properties[PopulateSwitchStatementHelpers.MissingDefaultCase]!);

Debug.Assert(missingCases || missingDefaultCase);

Expand Down Expand Up @@ -137,8 +137,8 @@ private async Task FixOneDiagnosticAsync(
bool addCases, bool addDefaultCase, bool onlyOneDiagnostic,
CancellationToken cancellationToken)
{
var hasMissingCases = bool.Parse(diagnostic.Properties[PopulateSwitchStatementHelpers.MissingCases]);
var hasMissingDefaultCase = bool.Parse(diagnostic.Properties[PopulateSwitchStatementHelpers.MissingDefaultCase]);
var hasMissingCases = bool.Parse(diagnostic.Properties[PopulateSwitchStatementHelpers.MissingCases]!);
var hasMissingDefaultCase = bool.Parse(diagnostic.Properties[PopulateSwitchStatementHelpers.MissingDefaultCase]!);

var switchLocation = diagnostic.AdditionalLocations[0];
var switchNode = switchLocation.FindNode(getInnermostNodeForTie: true, cancellationToken) as TSwitchSyntax;
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Core/Portable/CommandLine/SarifErrorLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ protected static string GetLevel(DiagnosticSeverity severity)

case DiagnosticSeverity.Hidden:
default:
// hidden diagnostics are not reported on the command line and therefore not currently given to
// hidden diagnostics are not reported on the command line and therefore not currently given to
// the error logger. We could represent it with a custom property in the SARIF log if that changes.
Debug.Assert(false);
goto case DiagnosticSeverity.Warning;
Expand Down Expand Up @@ -124,7 +124,7 @@ protected static string GetUri(string path)
{
Debug.Assert(!string.IsNullOrEmpty(path));

// Note that in general, these "paths" are opaque strings to be
// Note that in general, these "paths" are opaque strings to be
// interpreted by resolvers (see SyntaxTree.FilePath documentation).

// Common case: absolute path -> absolute URI
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Diagnostic/Diagnostic.DiagnosticWithProgrammaticSuppression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public override IReadOnlyList<Location> AdditionalLocations
get { return _originalUnsuppressedDiagnostic.AdditionalLocations; }
}

public override ImmutableDictionary<string, string> Properties
public override ImmutableDictionary<string, string?> Properties
{
get { return _originalUnsuppressedDiagnostic.Properties; }
}
Expand Down
34 changes: 17 additions & 17 deletions src/Compilers/Core/Portable/Diagnostic/Diagnostic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ public abstract partial class Diagnostic : IEquatable<Diagnostic?>, IFormattable
/// <returns>The <see cref="Diagnostic"/> instance.</returns>
public static Diagnostic Create(
DiagnosticDescriptor descriptor,
Location location,
params object?[] messageArgs)
Location? location,
params object?[]? messageArgs)
{
return Create(descriptor, location, null, null, messageArgs);
}
Expand All @@ -57,9 +57,9 @@ public static Diagnostic Create(
/// <returns>The <see cref="Diagnostic"/> instance.</returns>
public static Diagnostic Create(
DiagnosticDescriptor descriptor,
Location location,
ImmutableDictionary<string, string>? properties,
params object?[] messageArgs)
Location? location,
ImmutableDictionary<string, string?>? properties,
params object?[]? messageArgs)
{
return Create(descriptor, location, null, properties, messageArgs);
}
Expand All @@ -78,9 +78,9 @@ public static Diagnostic Create(
/// <returns>The <see cref="Diagnostic"/> instance.</returns>
public static Diagnostic Create(
DiagnosticDescriptor descriptor,
Location location,
Location? location,
IEnumerable<Location>? additionalLocations,
params object?[] messageArgs)
params object?[]? messageArgs)
{
return Create(descriptor, location, additionalLocations, properties: null, messageArgs: messageArgs);
}
Expand All @@ -104,10 +104,10 @@ public static Diagnostic Create(
/// <returns>The <see cref="Diagnostic"/> instance.</returns>
public static Diagnostic Create(
DiagnosticDescriptor descriptor,
Location location,
Location? location,
IEnumerable<Location>? additionalLocations,
ImmutableDictionary<string, string>? properties,
params object?[] messageArgs)
ImmutableDictionary<string, string?>? properties,
params object?[]? messageArgs)
{
return Create(descriptor, location, effectiveSeverity: descriptor.DefaultSeverity, additionalLocations, properties, messageArgs);
}
Expand All @@ -132,11 +132,11 @@ public static Diagnostic Create(
/// <returns>The <see cref="Diagnostic"/> instance.</returns>
public static Diagnostic Create(
DiagnosticDescriptor descriptor,
Location location,
Location? location,
DiagnosticSeverity effectiveSeverity,
IEnumerable<Location>? additionalLocations,
ImmutableDictionary<string, string>? properties,
params object?[] messageArgs)
ImmutableDictionary<string, string?>? properties,
params object?[]? messageArgs)
{
if (descriptor == null)
{
Expand Down Expand Up @@ -197,7 +197,7 @@ public static Diagnostic Create(
Location? location = null,
IEnumerable<Location>? additionalLocations = null,
IEnumerable<string>? customTags = null,
ImmutableDictionary<string, string>? properties = null)
ImmutableDictionary<string, string?>? properties = null)
{
return Create(id, category, message, severity, defaultSeverity, isEnabledByDefault, warningLevel, false,
title, description, helpLink, location, additionalLocations, customTags, properties);
Expand Down Expand Up @@ -248,7 +248,7 @@ public static Diagnostic Create(
Location? location = null,
IEnumerable<Location>? additionalLocations = null,
IEnumerable<string>? customTags = null,
ImmutableDictionary<string, string>? properties = null)
ImmutableDictionary<string, string?>? properties = null)
{
if (id == null)
{
Expand Down Expand Up @@ -398,8 +398,8 @@ public bool IsWarningAsError
/// if there is no entry. This can be used to put diagnostic specific information you want
/// to pass around. for example, to corresponding fixer.
/// </summary>
public virtual ImmutableDictionary<string, string> Properties
=> ImmutableDictionary<string, string>.Empty;
public virtual ImmutableDictionary<string, string?> Properties
=> ImmutableDictionary<string, string?>.Empty;

string IFormattable.ToString(string? ignored, IFormatProvider? formatProvider)
{
Expand Down
12 changes: 6 additions & 6 deletions src/Compilers/Core/Portable/Diagnostic/Diagnostic_SimpleDiagnostic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ internal sealed class SimpleDiagnostic : Diagnostic
private readonly Location _location;
private readonly IReadOnlyList<Location> _additionalLocations;
private readonly object?[] _messageArgs;
private readonly ImmutableDictionary<string, string> _properties;
private readonly ImmutableDictionary<string, string?> _properties;
private readonly bool _isSuppressed;

private SimpleDiagnostic(
Expand All @@ -36,7 +36,7 @@ private SimpleDiagnostic(
Location location,
IEnumerable<Location>? additionalLocations,
object?[]? messageArgs,
ImmutableDictionary<string, string>? properties,
ImmutableDictionary<string, string?>? properties,
bool isSuppressed)
{
if ((warningLevel == 0 && severity != DiagnosticSeverity.Error) ||
Expand All @@ -51,7 +51,7 @@ private SimpleDiagnostic(
_location = location ?? Location.None;
_additionalLocations = additionalLocations?.ToImmutableArray() ?? SpecializedCollections.EmptyReadOnlyList<Location>();
_messageArgs = messageArgs ?? Array.Empty<object?>();
_properties = properties ?? ImmutableDictionary<string, string>.Empty;
_properties = properties ?? ImmutableDictionary<string, string?>.Empty;
_isSuppressed = isSuppressed;
}

Expand All @@ -62,7 +62,7 @@ internal static SimpleDiagnostic Create(
Location location,
IEnumerable<Location>? additionalLocations,
object?[]? messageArgs,
ImmutableDictionary<string, string>? properties,
ImmutableDictionary<string, string?>? properties,
bool isSuppressed = false)
{
return new SimpleDiagnostic(descriptor, severity, warningLevel, location, additionalLocations, messageArgs, properties, isSuppressed);
Expand All @@ -72,7 +72,7 @@ internal static SimpleDiagnostic Create(string id, LocalizableString title, stri
DiagnosticSeverity severity, DiagnosticSeverity defaultSeverity,
bool isEnabledByDefault, int warningLevel, Location location,
IEnumerable<Location>? additionalLocations, IEnumerable<string>? customTags,
ImmutableDictionary<string, string>? properties, bool isSuppressed = false)
ImmutableDictionary<string, string?>? properties, bool isSuppressed = false)
{
var descriptor = new DiagnosticDescriptor(id, title, message,
category, defaultSeverity, isEnabledByDefault, description, helpLink, customTags.ToImmutableArrayOrEmpty());
Expand Down Expand Up @@ -139,7 +139,7 @@ public override IReadOnlyList<Location> AdditionalLocations
get { return _additionalLocations; }
}

public override ImmutableDictionary<string, string> Properties
public override ImmutableDictionary<string, string?> Properties
{
get { return _properties; }
}
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Core/Portable/InternalUtilities/JsonWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public void WriteKey(string key)
_pending = Pending.None;
}

public void Write(string key, string value)
public void Write(string key, string? value)
{
WriteKey(key);
Write(value);
Expand All @@ -96,7 +96,7 @@ public void Write(string key, bool value)
Write(value);
}

public void Write(string value)
public void Write(string? value)
{
WritePending();
_output.Write('"');
Expand Down
2 changes: 1 addition & 1 deletion src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ static string GetLanguageSpecificId(string? language, string noLanguageId, strin
warningLevel: 0,
projectId: projectId,
customTags: ImmutableArray<string>.Empty,
properties: ImmutableDictionary<string, string>.Empty,
properties: ImmutableDictionary<string, string?>.Empty,
language: language);
}

Expand Down
10 changes: 5 additions & 5 deletions src/Workspaces/Core/Portable/Diagnostics/DiagnosticData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal sealed class DiagnosticData : IEquatable<DiagnosticData?>
public readonly bool IsEnabledByDefault;
public readonly int WarningLevel;
public readonly IReadOnlyList<string> CustomTags;
public readonly ImmutableDictionary<string, string> Properties;
public readonly ImmutableDictionary<string, string?> Properties;

public readonly ProjectId? ProjectId;
public readonly DiagnosticDataLocation? DataLocation;
Expand Down Expand Up @@ -65,7 +65,7 @@ public DiagnosticData(
bool isEnabledByDefault,
int warningLevel,
IReadOnlyList<string> customTags,
ImmutableDictionary<string, string> properties,
ImmutableDictionary<string, string?> properties,
ProjectId? projectId,
DiagnosticDataLocation? location = null,
IReadOnlyCollection<DiagnosticDataLocation>? additionalLocations = null,
Expand Down Expand Up @@ -358,7 +358,7 @@ public static DiagnosticData Create(Diagnostic diagnostic, Document document)
{
if (additionalProperties == null)
{
additionalProperties = ImmutableDictionary.Create<string, string>();
additionalProperties = ImmutableDictionary.Create<string, string?>();
}

additionalProperties = additionalProperties.Add(nameof(documentPropertiesService.DiagnosticsLspClientName), diagnosticsLspClientName);
Expand All @@ -380,7 +380,7 @@ private static DiagnosticData Create(
OptionSet options,
DiagnosticDataLocation? location,
IReadOnlyCollection<DiagnosticDataLocation>? additionalLocations,
ImmutableDictionary<string, string>? additionalProperties)
ImmutableDictionary<string, string?>? additionalProperties)
{
return new DiagnosticData(
diagnostic.Id,
Expand All @@ -403,7 +403,7 @@ private static DiagnosticData Create(
isSuppressed: diagnostic.IsSuppressed);
}

private static ImmutableDictionary<string, string>? GetAdditionalProperties(Document document, Diagnostic diagnostic)
private static ImmutableDictionary<string, string?>? GetAdditionalProperties(Document document, Diagnostic diagnostic)
{
var service = document.GetLanguageService<IDiagnosticPropertiesService>();
return service?.GetAdditionalProperties(diagnostic);
Expand Down
6 changes: 3 additions & 3 deletions src/Workspaces/Core/Portable/Diagnostics/DiagnosticDataSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,11 @@ private static IReadOnlyCollection<DiagnosticDataLocation> ReadAdditionalLocatio
return result;
}

private static ImmutableDictionary<string, string> GetProperties(ObjectReader reader, int count)
private static ImmutableDictionary<string, string?> GetProperties(ObjectReader reader, int count)
{
if (count > 0)
{
var properties = ImmutableDictionary.CreateBuilder<string, string>();
var properties = ImmutableDictionary.CreateBuilder<string, string?>();
for (var i = 0; i < count; i++)
{
properties.Add(reader.ReadString(), reader.ReadString());
Expand All @@ -336,7 +336,7 @@ private static ImmutableDictionary<string, string> GetProperties(ObjectReader re
return properties.ToImmutable();
}

return ImmutableDictionary<string, string>.Empty;
return ImmutableDictionary<string, string?>.Empty;
}

private static IReadOnlyList<string> GetCustomTags(ObjectReader reader, int count)
Expand Down