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

Produce warnings for definite assignment issues with private fields from metadata. #46221

Merged
merged 10 commits into from
Aug 1, 2020
8 changes: 8 additions & 0 deletions docs/compilers/CSharp/Warnversion Warning Waves.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,11 @@ The table below describes all of the warnings controlled by warning levels `5` o
| CS7023 | 5 | [A static type is used in an 'is' or 'as' expression](https://github.com/dotnet/roslyn/issues/30198) |
| CS8073 | 5 | [Expression always true (or false) when comparing a struct to null](https://github.com/dotnet/roslyn/issues/45744) |
| CS8848 | 5 | [Diagnose precedence error with query expression](https://github.com/dotnet/roslyn/issues/30231) |
| CS8880 | 5 | [Struct constructor does not assign auto property (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8881 | 5 | [Struct constructor does not assign field (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8882 | 5 | [Out parameter not assigned (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8883 | 5 | [Auto-property used before assigned in struct constructor (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8884 | 5 | [Field used before assigned in struct constructor (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8885 | 5 | [Struct constructor reads 'this' before assigning all fields (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8886 | 5 | [Out parameter used before being assigned (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
| CS8887 | 5 | [Local variable used before being assigned (imported struct type with private fields)](https://github.com/dotnet/roslyn/issues/30194) |
50 changes: 49 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,12 @@
<data name="ERR_UseDefViolation" xml:space="preserve">
<value>Use of unassigned local variable '{0}'</value>
</data>
<data name="WRN_UseDefViolation" xml:space="preserve">
<value>Use of unassigned local variable '{0}'</value>
</data>
<data name="WRN_UseDefViolation_Title" xml:space="preserve">
<value>Use of unassigned local variable</value>
</data>
<data name="WRN_UnreferencedVar" xml:space="preserve">
<value>The variable '{0}' is declared but never used</value>
</data>
Expand All @@ -834,12 +840,30 @@
<data name="ERR_UseDefViolationField" xml:space="preserve">
<value>Use of possibly unassigned field '{0}'</value>
</data>
<data name="WRN_UseDefViolationField" xml:space="preserve">
<value>Use of possibly unassigned field '{0}'</value>
</data>
<data name="WRN_UseDefViolationField_Title" xml:space="preserve">
<value>Use of possibly unassigned field</value>
</data>
<data name="ERR_UseDefViolationProperty" xml:space="preserve">
<value>Use of possibly unassigned auto-implemented property '{0}'</value>
</data>
<data name="WRN_UseDefViolationProperty" xml:space="preserve">
<value>Use of possibly unassigned auto-implemented property '{0}'</value>
</data>
<data name="WRN_UseDefViolationProperty_Title" xml:space="preserve">
<value>Use of possibly unassigned auto-implemented property</value>
</data>
<data name="ERR_UnassignedThis" xml:space="preserve">
<value>Field '{0}' must be fully assigned before control is returned to the caller</value>
</data>
<data name="WRN_UnassignedThis" xml:space="preserve">
<value>Field '{0}' must be fully assigned before control is returned to the caller</value>
</data>
<data name="WRN_UnassignedThis_Title" xml:space="preserve">
<value>Fields of a struct must be fully assigned in a constructor before control is returned to the caller</value>
</data>
<data name="ERR_AmbigQM" xml:space="preserve">
<value>Type of conditional expression cannot be determined because '{0}' and '{1}' implicitly convert to one another</value>
</data>
Expand All @@ -858,6 +882,12 @@
<data name="ERR_ParamUnassigned" xml:space="preserve">
<value>The out parameter '{0}' must be assigned to before control leaves the current method</value>
</data>
<data name="WRN_ParamUnassigned" xml:space="preserve">
<value>The out parameter '{0}' must be assigned to before control leaves the current method</value>
</data>
<data name="WRN_ParamUnassigned_Title" xml:space="preserve">
<value>An out parameter must be assigned to before control leaves the method</value>
</data>
<data name="ERR_InvalidArray" xml:space="preserve">
<value>Invalid rank specifier: expected ',' or ']'</value>
</data>
Expand Down Expand Up @@ -901,7 +931,13 @@
<value>Use of default literal is not valid in this context</value>
</data>
<data name="ERR_UseDefViolationThis" xml:space="preserve">
<value>The 'this' object cannot be used before all of its fields are assigned to</value>
<value>The 'this' object cannot be used before all of its fields have been assigned</value>
</data>
<data name="WRN_UseDefViolationThis" xml:space="preserve">
<value>The 'this' object cannot be used before all of its fields have been assigned</value>
</data>
<data name="WRN_UseDefViolationThis_Title" xml:space="preserve">
<value>The 'this' object cannot be used in a constructor before all of its fields have been assigned</value>
</data>
<data name="ERR_ArgsInvalid" xml:space="preserve">
<value>The __arglist construct is valid only within a variable argument method</value>
Expand Down Expand Up @@ -1116,6 +1152,12 @@
<data name="ERR_UseDefViolationOut" xml:space="preserve">
<value>Use of unassigned out parameter '{0}'</value>
</data>
<data name="WRN_UseDefViolationOut" xml:space="preserve">
<value>Use of unassigned out parameter '{0}'</value>
</data>
<data name="WRN_UseDefViolationOut_Title" xml:space="preserve">
<value>Use of unassigned out parameter</value>
</data>
<data name="ERR_ArraySizeInDeclaration" xml:space="preserve">
<value>Array size cannot be specified in a variable declaration (try initializing with a 'new' expression)</value>
</data>
Expand Down Expand Up @@ -2249,6 +2291,12 @@ If such a class is used as a base class and if the deriving class defines a dest
<data name="ERR_UnassignedThisAutoProperty" xml:space="preserve">
<value>Auto-implemented property '{0}' must be fully assigned before control is returned to the caller.</value>
</data>
<data name="WRN_UnassignedThisAutoProperty" xml:space="preserve">
<value>Auto-implemented property '{0}' must be fully assigned before control is returned to the caller.</value>
</data>
<data name="WRN_UnassignedThisAutoProperty_Title" xml:space="preserve">
<value>An auto-implemented property must be fully assigned before control is returned to the caller.</value>
</data>
<data name="ERR_VariableUsedBeforeDeclarationAndHidesField" xml:space="preserve">
<value>Cannot use local variable '{0}' before it is declared. The declaration of the local variable hides the field '{1}'.</value>
</data>
Expand Down
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1874,6 +1874,17 @@ internal enum ErrorCode
ERR_CopyConstructorWrongAccessibility = 8878,
ERR_NonPrivateAPIInRecord = 8879,

// The following warnings correspond to errors of the same name, but are reported
// when a definite assignment issue is reported due to private fields imported from metadata.
WRN_UnassignedThisAutoProperty = 8880,
333fred marked this conversation as resolved.
Show resolved Hide resolved
WRN_UnassignedThis = 8881,
WRN_ParamUnassigned = 8882,
WRN_UseDefViolationProperty = 8883,
WRN_UseDefViolationField = 8884,
WRN_UseDefViolationThis = 8885,
WRN_UseDefViolationOut = 8886,
WRN_UseDefViolation = 8887,

#endregion diagnostics introduced for C# 9.0
// Note: you will need to re-generate compiler code after adding warnings (eng\generate-compiler-code.cmd)
}
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,14 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_NubExprIsConstBool2:
case ErrorCode.WRN_StaticInAsOrIs:
case ErrorCode.WRN_PrecedenceInversion:
case ErrorCode.WRN_UnassignedThisAutoProperty:
case ErrorCode.WRN_UnassignedThis:
case ErrorCode.WRN_ParamUnassigned:
case ErrorCode.WRN_UseDefViolationProperty:
case ErrorCode.WRN_UseDefViolationField:
case ErrorCode.WRN_UseDefViolationThis:
case ErrorCode.WRN_UseDefViolationOut:
case ErrorCode.WRN_UseDefViolation:
// Warning level 5 is exclusively for warnings introduced in the compiler
// shipped with dotnet 5 (C# 9) and that can be reported for pre-existing code.
return 5;
Expand Down
129 changes: 109 additions & 20 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
#define REFERENCE_STATE
#endif

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using Microsoft.CodeAnalysis.CSharp.Symbols;
Expand Down Expand Up @@ -123,13 +123,14 @@ internal DefiniteAssignmentPass(
CSharpCompilation compilation,
Symbol member,
BoundNode node,
bool strictAnalysis,
bool trackUnassignments = false,
HashSet<PrefixUnaryExpressionSyntax> unassignedVariableAddressOfSyntaxes = null,
bool requireOutParamsAssigned = true,
bool trackClassFields = false,
bool trackStaticMembers = false)
: base(compilation, member, node,
compilation.FeatureStrictEnabled ? EmptyStructTypeCache.CreatePrecise() : EmptyStructTypeCache.CreateForDev12Compatibility(compilation),
strictAnalysis ? EmptyStructTypeCache.CreatePrecise() : EmptyStructTypeCache.CreateForDev12Compatibility(compilation),
trackUnassignments)
{
this.initiallyAssignedVariables = null;
Expand All @@ -149,9 +150,7 @@ internal DefiniteAssignmentPass(
EmptyStructTypeCache emptyStructs,
bool trackUnassignments = false,
HashSet<Symbol> initiallyAssignedVariables = null)
: base(compilation, member, node,
emptyStructs ?? (compilation.FeatureStrictEnabled ? EmptyStructTypeCache.CreatePrecise() : EmptyStructTypeCache.CreateForDev12Compatibility(compilation)),
trackUnassignments)
: base(compilation, member, node, emptyStructs, trackUnassignments)
{
this.initiallyAssignedVariables = initiallyAssignedVariables;
_sourceAssembly = ((object)member == null) ? null : (SourceAssemblySymbol)member.ContainingAssembly;
Expand Down Expand Up @@ -378,33 +377,123 @@ protected virtual void ReportUnassignedOutParameter(ParameterSymbol parameter, S
/// <summary>
/// Perform data flow analysis, reporting all necessary diagnostics.
/// </summary>
public static void Analyze(CSharpCompilation compilation, MethodSymbol member, BoundNode node, DiagnosticBag diagnostics, bool requireOutParamsAssigned = true)
public static void Analyze(
CSharpCompilation compilation,
MethodSymbol member,
BoundNode node,
DiagnosticBag diagnostics,
bool requireOutParamsAssigned = true)
{
Debug.Assert(diagnostics != null);

var walker = new DefiniteAssignmentPass(
compilation,
member,
node,
requireOutParamsAssigned: requireOutParamsAssigned);
walker._convertInsufficientExecutionStackExceptionToCancelledByStackGuardException = true;
// Run the strongest version of analysis
DiagnosticBag strictDiagnostics = analyze(strictAnalysis: true);
if (strictDiagnostics.IsEmptyWithoutResolution)
{
// If it reports nothing, there is nothing to report and we are done.
strictDiagnostics.Free();
return;
}

try
// Also run the compat (weaker) version of analysis to see if we get the same diagnostics.
// If any are missing, the extra ones from the strong analysis will be downgraded to a warning.
DiagnosticBag compatDiagnostics = analyze(strictAnalysis: false);

// If the compat diagnostics caused a stack overflow, the two analyses might not produce comparable sets of diagnostics.
// So we just report the compat ones including that error.
if (compatDiagnostics.AsEnumerable().Any(d => (ErrorCode)d.Code == ErrorCode.ERR_InsufficientStack))
{
bool badRegion = false;
walker.Analyze(ref badRegion, diagnostics);
Debug.Assert(!badRegion);
diagnostics.AddRangeAndFree(compatDiagnostics);
strictDiagnostics.Free();
return;
}
catch (BoundTreeVisitor.CancelledByStackGuardException ex) when (diagnostics != null)

// If the compat diagnostics did not overflow and we have the same number of diagnostics, we just report the stricter set.
// It is OK if the strict analysis had an overflow here, causing the sets to be incomparable: the reported diagnostics will
// include the error reporting that fact.
if (strictDiagnostics.Count == compatDiagnostics.Count)
{
ex.AddAnError(diagnostics);
diagnostics.AddRangeAndFree(strictDiagnostics);
compatDiagnostics.Free();
return;
}
finally

HashSet<Diagnostic> compatDiagnosticSet = new HashSet<Diagnostic>(compatDiagnostics.AsEnumerable(), SameDiagnosticComparer.Instance);
333fred marked this conversation as resolved.
Show resolved Hide resolved
compatDiagnostics.Free();
foreach (var diagnostic in strictDiagnostics.AsEnumerable())
{
walker.Free();
// If it is a warning (e.g. WRN_AsyncLacksAwaits), or an error that would be reported by the compatible analysis, just report it.
if (diagnostic.Severity != DiagnosticSeverity.Error || compatDiagnosticSet.Contains(diagnostic))
{
diagnostics.Add(diagnostic);
continue;
}

// Otherwise downgrade the error to a warning.
ErrorCode oldCode = (ErrorCode)diagnostic.Code;
ErrorCode newCode = oldCode switch
{
#pragma warning disable format
ErrorCode.ERR_UnassignedThisAutoProperty => ErrorCode.WRN_UnassignedThisAutoProperty,
ErrorCode.ERR_UnassignedThis => ErrorCode.WRN_UnassignedThis,
ErrorCode.ERR_ParamUnassigned => ErrorCode.WRN_ParamUnassigned,
ErrorCode.ERR_UseDefViolationProperty => ErrorCode.WRN_UseDefViolationProperty,
ErrorCode.ERR_UseDefViolationField => ErrorCode.WRN_UseDefViolationField,
ErrorCode.ERR_UseDefViolationThis => ErrorCode.WRN_UseDefViolationThis,
ErrorCode.ERR_UseDefViolationOut => ErrorCode.WRN_UseDefViolationOut,
ErrorCode.ERR_UseDefViolation => ErrorCode.WRN_UseDefViolation,
_ => oldCode, // rare but possible, e.g. ErrorCode.ERR_InsufficientStack occurring in strict mode only due to needing extra frames
#pragma warning restore format
};

// We don't know any other way this can happen, but if it does we recover gracefully in production.
Debug.Assert(newCode != oldCode || oldCode == ErrorCode.ERR_InsufficientStack, oldCode.ToString());

var args = diagnostic is DiagnosticWithInfo { Info: { Arguments: var arguments } } ? arguments : diagnostic.Arguments.ToArray();
diagnostics.Add(newCode, diagnostic.Location, args);
}

strictDiagnostics.Free();
return;

DiagnosticBag analyze(bool strictAnalysis)
{
DiagnosticBag result = DiagnosticBag.GetInstance();
var walker = new DefiniteAssignmentPass(
compilation,
member,
node,
strictAnalysis: strictAnalysis,
requireOutParamsAssigned: requireOutParamsAssigned);
walker._convertInsufficientExecutionStackExceptionToCancelledByStackGuardException = true;

try
{
bool badRegion = false;
walker.Analyze(ref badRegion, result);
Debug.Assert(!badRegion);
}
catch (BoundTreeVisitor.CancelledByStackGuardException ex) when (diagnostics != null)
{
ex.AddAnError(result);
}
finally
{
walker.Free();
}

return result;
}
}

private sealed class SameDiagnosticComparer : EqualityComparer<Diagnostic>
{
public static readonly SameDiagnosticComparer Instance = new SameDiagnosticComparer();
public override bool Equals([AllowNull] Diagnostic x, [AllowNull] Diagnostic y) => x.Equals(y);
public override int GetHashCode([DisallowNull] Diagnostic obj) =>
Hash.Combine(Hash.CombineValues(obj.Arguments), Hash.Combine(obj.Location.GetHashCode(), obj.Code));
}

/// <summary>
/// Analyze the body, reporting all necessary diagnostics.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ protected LocalDataFlowPass(
int maxSlotDepth = 0)
: base(compilation, member, node, nonMonotonicTransferFunction: trackUnassignments)
{
Debug.Assert(emptyStructs != null);
_maxSlotDepth = maxSlotDepth;
_emptyStructTypeCache = emptyStructs;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand All @@ -16,7 +14,7 @@ namespace Microsoft.CodeAnalysis.CSharp
internal class UnassignedAddressTakenVariablesWalker : DefiniteAssignmentPass
{
private UnassignedAddressTakenVariablesWalker(CSharpCompilation compilation, Symbol member, BoundNode node)
: base(compilation, member, node)
: base(compilation, member, node, strictAnalysis: true)
{
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ internal sealed class UnassignedFieldsWalker : DefiniteAssignmentPass
private readonly DiagnosticBag _diagnostics;

private UnassignedFieldsWalker(CSharpCompilation compilation, MethodSymbol method, BoundNode node, DiagnosticBag diagnostics)
: base(compilation, method, node, trackClassFields: true, trackStaticMembers: method.MethodKind == MethodKind.StaticConstructor)
: base(compilation, method, node, strictAnalysis: true, trackClassFields: true, trackStaticMembers: method.MethodKind == MethodKind.StaticConstructor)
{
_diagnostics = diagnostics;
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading