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

Seal internal private types analyzer #5594

Merged

Conversation

NewellClark
Copy link
Contributor

@NewellClark NewellClark commented Oct 4, 2021

Fix #49944.
As per the design meeting, this analyzer ignores InternalsVisibleToAttributes.
I've implemented a fixer, but due to this issue the code fixer won't show up in the IDE because it is reported at the end of compilation.

I've also found violations in dotnet/runtime (violations in src/tasks, violations in libs).

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #5594 (afccbbc) into main (334bb06) will decrease coverage by 0.00%.
The diff coverage is 91.68%.

@@            Coverage Diff             @@
##             main    #5594      +/-   ##
==========================================
- Coverage   96.04%   96.03%   -0.01%     
==========================================
  Files        1323     1326       +3     
  Lines      305370   305731     +361     
  Branches     9687     9693       +6     
==========================================
+ Hits       293294   293621     +327     
- Misses       9739     9774      +35     
+ Partials     2337     2336       -1     

@mavasani
Copy link
Contributor

As per the design meeting, this analyzer ignores InternalsVisibleToAttributes.

This means the logic will differ from CA1812 analyzer, which skips compilations which have IVT. I presume this was considered in the design meeting though.

// If the assembly being built by this compilation exposes its internals to
// any other assembly, don't report any "uninstantiated internal class" errors.
// If we were to report an error for an internal type that is not instantiated
// by this assembly, and then it turned out that the friend assembly did
// instantiate the type, that would be a false positive. We've decided it's
// better to have false negatives (which would happen if the type were *not*
// instantiated by any friend assembly, but we didn't report the issue) than
// to have false positives.
var internalsVisibleToAttributeSymbol = wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesInternalsVisibleToAttribute);
if (compilation.Assembly.HasAttribute(internalsVisibleToAttributeSymbol))
{
return;
}

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Note that the code fixer is never going to show up in VS as this is a compilation end diagnostic. It is still useful to retain it so a fix all can be applied from command line tooling, such as AnalyzerRunner or dotnet-format. In future, if we add some incremental analysis API such that compilation end analyzers and their code fixes can be enabled in the IDE, hopefully we will get the code fix to show up in VS.

Also calling out that compilation end diagnostics are essentially build-only diagnostics, which will only show up on build if the user bumps the severity to a warning or sets AnalysisMode to AllEnabledByDefault. They will only show up in live/intellisense diagnostics if full solution analysis mode is enabled. So basically, this will be disabled by default analyzer.

@mavasani
Copy link
Contributor

@NewellClark Can you please resolve the merge conflicts?

@mavasani mavasani self-assigned this Jan 31, 2022
@pranavkm
Copy link

@mavasani is this good to merge?

@Evangelink
Copy link
Member

It would be nice to add the test @Youssef1313 was mentioning regarding partial classes with some part auto-generated.

@mavasani
Copy link
Contributor

@pranavkm We need to resolve merge conflicts and respond to review feedback from @Youssef1313 and @Evangelink. This should be very close to merge.

@meziantou
Copy link

@ericstj ask me to post my remark here

I think the analyzer should exclude classes declared as [ComImport]/[CoClass] from the analysis as the fix may break the compilation:

using System.Runtime.InteropServices;

_ = (IFileSaveDialog)new FileSaveDialogRCW(); // If FileSaveDialogRCW is sealed: error CS0030: Cannot convert type 'FileSaveDialogRCW' to 'IFileSaveDialog'

[ComImport]
[Guid("C0B4E2F3-BA21-4773-8DBA-335EC946EB8B")]
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
[CoClass(typeof(FileSaveDialogRCW))]
internal interface IFileSaveDialog
{
}

[ComImport]
[ClassInterface(ClassInterfaceType.None)]
[Guid("C0B4E2F3-BA21-4773-8DBA-335EC946EB8B")]
class FileSaveDialogRCW // 👈 sealed is not valid here because of `(IFileSaveDialog)new NativeFileSaveDialog()`
{
}

@stephentoub
Copy link
Member

@NewellClark, I'd like to see this merged. Are you still able to work on it? If not, I'll take it over. Thanks for your work on it!

@mavasani mavasani assigned stephentoub and unassigned mavasani Apr 11, 2022
@stephentoub stephentoub force-pushed the seal-internal-private-types-analyzer branch from 4b264fa to 1e7b136 Compare April 15, 2022 21:00
@stephentoub stephentoub force-pushed the seal-internal-private-types-analyzer branch from 1e7b136 to 8c7b41d Compare April 17, 2022 03:18
@stephentoub stephentoub force-pushed the seal-internal-private-types-analyzer branch from 8c7b41d to 1ac1aad Compare April 17, 2022 03:19
…InternalTypes.cs

Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
@stephentoub stephentoub merged commit 91188b6 into dotnet:main Apr 17, 2022
@github-actions github-actions bot added this to the vNext milestone Apr 17, 2022
@jmarolf jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022
smfeest added a commit to smfeest/buttercup that referenced this pull request Mar 5, 2023
.NET 7 includes a new analyzer [1] that checks for internal types that
can be sealed. We therefore need to seal all private classes that don't
have derived types to avoid CA1852 compiler warnings on upgrading to
.NET 7.

There's probably a case for also sealing all public types that we don't
intend to extend, but that can be deferred to a separate branch.

[1] dotnet/roslyn-analyzers#5594
smfeest added a commit to smfeest/buttercup that referenced this pull request Mar 5, 2023
As mentioned in the previous commit, .NET 7 includes a new analyzer [1]
that checks for internal types that can be sealed.

Unfortunately that analyzer is currently generating false positives for
generated `Program` classes [2] and therefore has to be suppressed
manually.

A fix [3] has already been committed to the Roslyn Analyzers repo so we
should be able to remove this suppression once that fix has made its way
into an SDK update.

[1] dotnet/roslyn-analyzers#5594
[2] dotnet/roslyn-analyzers#6141
[3] dotnet/roslyn-analyzers#6278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analyzer Proposal: Seal internal/private types