Skip to content

Fix nullref in ComponentsAnalyzer #18608

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

Merged
merged 2 commits into from
Jan 28, 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
3 changes: 3 additions & 0 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
-->
<IsStableBuild>false</IsStableBuild>
<IsStableBuild Condition=" '$(DotNetFinalVersionKind)' == 'release' ">true</IsStableBuild>

<!-- Workaround issue with ComponentsAnalyzer throwing for interfaces -->
<DisableImplicitComponentsAnalyzers>true</DisableImplicitComponentsAnalyzers>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we file a follow up to undo this?

</PropertyGroup>

<Import Project="eng\FlakyTests.BeforeArcade.props" />
Expand Down
4 changes: 2 additions & 2 deletions src/Components/Analyzers/src/InternalUsageAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ private void AnalyzeSymbol(SymbolAnalysisContext context)
// Similar logic here to VisitDeclarationSymbol, keep these in sync.
private void VisitOperationSymbol(OperationAnalysisContext context, ISymbol symbol)
{
if (symbol.ContainingAssembly == context.Compilation.Assembly)
if (symbol == null || symbol.ContainingAssembly == context.Compilation.Assembly)
{
// The type is being referenced within the same assembly. This is valid use of an "internal" type
return;
Expand Down Expand Up @@ -155,7 +155,7 @@ private void VisitOperationSymbol(OperationAnalysisContext context, ISymbol symb
// Similar logic here to VisitOperationSymbol, keep these in sync.
private void VisitDeclarationSymbol(SymbolAnalysisContext context, ISymbol symbol, ISymbol symbolForDiagnostic)
{
if (symbol.ContainingAssembly == context.Compilation.Assembly)
if (symbol == null || symbol.ContainingAssembly == context.Compilation.Assembly)
{
// This is part of the compilation, avoid this analyzer when building from source.
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@ protected override void HandleException(Exception exception)
throw new NotImplementedException();
}

/*MMParameter*/protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
/*MMParameter*/protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
{
throw new NotImplementedException();
}

/*MMReturnType*/private Renderer GetRenderer() => _field;

public interface ITestInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @rynowak \ @SteveSandersonMS \ @mkArtakMSFT :

type.BaseType for interface types is null: https://github.com/dotnet/aspnetcore/pull/18608/files#diff-f7300f2c3fe64cdbb8f5dfd7854b0b56R85. This causes the call to VisitDeclarationSymbol to null-ref. It looks like the compiler was swallowing this in the past, but they now are presented as warnings. We were able to author the unit test to trivially cause this to fail. Should we consider patching this?

{
}
}
}