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

Fix build warnings and update compiler #3400

Merged
merged 4 commits into from
Mar 23, 2020
Merged
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
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<!-- Roslyn -->
<MicrosoftCodeAnalysisVersion>3.0.0</MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisForShippedApisVersion>3.5.0-beta2-20056-01</MicrosoftCodeAnalysisForShippedApisVersion>
<MicrosoftNetCompilersVersion>3.5.0-beta4-20153-05</MicrosoftNetCompilersVersion>
<MicrosoftNetCompilersVersion>3.6.0-1.final</MicrosoftNetCompilersVersion>
<DogfoodAnalyzersVersion>3.0.0-beta2.19529.2+e119d9cf</DogfoodAnalyzersVersion>
<MicrosoftCodeAnalysisBannedApiAnalyzersVersion>$(DogfoodAnalyzersVersion)</MicrosoftCodeAnalysisBannedApiAnalyzersVersion>
<MicrosoftCodeAnalysisFXCopAnalyersVersion>$(DogfoodAnalyzersVersion)</MicrosoftCodeAnalysisFXCopAnalyersVersion>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ private void OnCompilationStart(CompilationStartAnalysisContext compilationConte
return;
}

var entryByAttributeSymbol = entryBySymbol
Dictionary<ISymbol, SymbolIsBannedAnalyzer<TSyntaxKind>.BanFileEntry> entryByAttributeSymbol = entryBySymbol
.Where(pair => pair.Key is ITypeSymbol n && n.IsAttribute())
.ToDictionary(pair => pair.Key, pair => pair.Value);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public override void Initialize(AnalysisContext analysisContext)

analysisContext.RegisterCompilationStartAction(startContext =>
{
var instantiatedTypes = new ConcurrentDictionary<INamedTypeSymbol, object?>();
ConcurrentDictionary<INamedTypeSymbol, object?> instantiatedTypes = new ConcurrentDictionary<INamedTypeSymbol, object?>();
Copy link
Member

@jcouv jcouv Mar 19, 2020

Choose a reason for hiding this comment

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

Those explicit types were not necessary, were they?

Copy link
Member

Choose a reason for hiding this comment

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

The commit introducing these changes is named "Use explicit type where compiler fails to evaluate nullability" so I think they matters.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcouv yes, they are necessary. var doesn't work with local functions, at least not here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to file a compiler bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there are already bugs filed for improving local function support. It hit Roslyn too.

Copy link
Member Author

Choose a reason for hiding this comment

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

var internalTypes = new ConcurrentDictionary<INamedTypeSymbol, object?>();

var compilation = startContext.Compilation;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public override void Initialize(AnalysisContext context)
return;
}

var lazyValueContentResult = new Lazy<DataFlowAnalysisResult<ValueContentBlockAnalysisResult, ValueContentAbstractValue>?>(
Lazy<DataFlowAnalysisResult<ValueContentBlockAnalysisResult, ValueContentAbstractValue>?> lazyValueContentResult = new Lazy<DataFlowAnalysisResult<ValueContentBlockAnalysisResult, ValueContentAbstractValue>?>(
sharwell marked this conversation as resolved.
Show resolved Hide resolved
valueFactory: ComputeValueContentAnalysisResult, isThreadSafe: false);

operationBlockStartContext.RegisterOperationAction(operationContext =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,14 @@ public sealed override void Initialize(AnalysisContext context)
return;
}

var forwardGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>();
var invertedGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>();
ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>> forwardGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please retarget all security rule changes to 2.9.x branch. Also, there is no nullability involved here, so why was this required?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not practical to backport this change. The 2.9.x branch doesn't support this toolchain or compiler version so it would be a large backport of much more than just this pull request.

I don't believe these changes will be a problem in practice. /cc @dotpaul for awareness.

ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>> invertedGraph = new ConcurrentDictionary<ISymbol, ConcurrentDictionary<ISymbol, bool>>();

// It keeps the out Degree of every vertex in the invertedGraph, which is corresponding to the in Degree of the vertex in forwardGraph.
var inDegree = new ConcurrentDictionary<ISymbol, int>();
ConcurrentDictionary<ISymbol, int> inDegree = new ConcurrentDictionary<ISymbol, int>();

// It Keeps the out degree of every vertex in the forwardGraph, which is corresponding to the in Degree of the vertex in invertedGraph.
var outDegree = new ConcurrentDictionary<ISymbol, int>();
ConcurrentDictionary<ISymbol, int> outDegree = new ConcurrentDictionary<ISymbol, int>();

compilationStartAnalysisContext.RegisterSymbolAction(
(SymbolAnalysisContext symbolAnalysisContext) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ public sealed override void Initialize(AnalysisContext context)
}

var nonSerializedAttribute = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemNonSerializedAttribute);
var visitedType = new ConcurrentDictionary<ITypeSymbol, bool>();
var pointerFields = new ConcurrentDictionary<IFieldSymbol, bool>();
ConcurrentDictionary<ITypeSymbol, bool> visitedType = new ConcurrentDictionary<ITypeSymbol, bool>();
ConcurrentDictionary<IFieldSymbol, bool> pointerFields = new ConcurrentDictionary<IFieldSymbol, bool>();

compilationStartAnalysisContext.RegisterSymbolAction(
(SymbolAnalysisContext symbolAnalysisContext) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public override void Initialize(AnalysisContext context)

context.RegisterCompilationStartAction(compilationStartAnalysisContext =>
{
var compilation = compilationStartAnalysisContext.Compilation;
Compilation compilation = compilationStartAnalysisContext.Compilation;
var wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(compilationStartAnalysisContext.Compilation);

if (!wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemWebUIPage, out var pageTypeSymbol) ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public override void Initialize(AnalysisContext context)
// Verify that validate anti forgery token attributes are used somewhere within this project,
// to avoid reporting false positives on projects that use an alternative approach to mitigate CSRF issues.
var usingValidateAntiForgeryAttribute = false;
var onAuthorizationAsyncMethodSymbols = new ConcurrentDictionary<IMethodSymbol, bool>();
ConcurrentDictionary<IMethodSymbol, bool> onAuthorizationAsyncMethodSymbols = new ConcurrentDictionary<IMethodSymbol, bool>();
var actionMethodSymbols = new ConcurrentDictionary<(IMethodSymbol, string), bool>();
var actionMethodNeedAddingHttpVerbAttributeSymbols = new ConcurrentDictionary<IMethodSymbol, bool>();

Expand Down
8 changes: 6 additions & 2 deletions src/Utilities/Compiler/Options/Unit.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ internal struct Unit : IEquatable<Unit>
/// <param name="first">The first <see cref="Unit"/> value to compare.</param>
/// <param name="second">The second <see cref="Unit"/> value to compare.</param>
/// <returns>Because <see cref="Unit"/> has a single value, this always returns <c>true</c>.</returns>
[SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "first", Justification = "Parameter required for operator overloading."), SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "second", Justification = "Parameter required for operator overloading.")]
[SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "first", Justification = "Parameter required for operator overloading.")]
[SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "second", Justification = "Parameter required for operator overloading.")]
[SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "Parameter required for operator overloading.")]
public static bool operator ==(Unit first, Unit second) => true;

/// <summary>
Expand All @@ -54,7 +56,9 @@ internal struct Unit : IEquatable<Unit>
/// <param name="first">The first <see cref="Unit"/> value to compare.</param>
/// <param name="second">The second <see cref="Unit"/> value to compare.</param>
/// <returns>Because <see cref="Unit"/> has a single value, this always returns <c>false</c>.</returns>
[SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "first", Justification = "Parameter required for operator overloading."), SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "second", Justification = "Parameter required for operator overloading.")]
[SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "first", Justification = "Parameter required for operator overloading.")]
[SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "second", Justification = "Parameter required for operator overloading.")]
[SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "Parameter required for operator overloading.")]
public static bool operator !=(Unit first, Unit second) => false;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ public bool Remove(KeyValuePair<TKey, TValue> item)
return Remove(item.Key);
}

#pragma warning disable CS8767 // Nullability of reference types in type of parameter doesn't match implicitly implemented member because of nullability attributes. https://github.com/dotnet/roslyn/issues/42552
public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value)
#pragma warning restore CS8767 // Nullability of reference types in type of parameter doesn't match implicitly implemented member because of nullability attributes.
{
Debug.Assert(!IsDisposed);
return _coreAnalysisData.TryGetValue(key, out value);
Expand Down