Fix S1144: Nested type constructor accessibility is wrong in the rule message
Closed this issue · 7 comments
Description
The message I got in VS IDE is:
This is not private ctor, but public.
And also this private class is used.
Repro steps
This is the code example
public class TagDto
{
public int Id { get; set; }
public string? Name { get; set; }
public bool System { get; set; }
public bool Active { get; set; }
public DateTime? SynchronizedOn { get; set; }
private class Mapping : Profile
{
public Mapping()
{
CreateMap<Tag, TagDto>();
}
}
}And this is used as
.ProjectTo<TagDto>(_mapper.ConfigurationProvider),
Related information
- C#/VB.NET Plugins version: SonarAnalyzer.CSharp Version 9.7.0.75501
- Visual Studio version: Version 17.7.0 Preview 5.0
- MSBuild / dotnet version: .NET 8 Preview 5
- Operating System: Windows
Thank you @neman for the report. The message is indeed wrong. Regarding the second issue:
And also this private class is used.
Do you use assembly scanning via AddMaps like e.g. so:
var configuration = new MapperConfiguration(cfg => cfg.AddMaps(myAssembly));If so, there is nothing we can do about it at the moment. Automapper uses reflection at runtime to find the type and create an instance of it. There are no compile-time dependencies on the constructor.
If you are using .Net 5 or above, we could consider adding support for the DynamicallyAccessedMembersAttribute that is used by e.g. the trimmer for such cases:
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
public class TagDto
{
public int Id { get; set; }
private class Mapping : Profile
{
public Mapping()
{
CreateMap<Tag, TagDto>();
}
}
}Would you apply this attribute to your Dtos to help our analyzer? If so, I would create a new issue to add support for that attribute.
I can try to add that attribute, although I do not like to add to my code more than required for the implementation.
Adding additional code just for the sake of the 3rd party tooling is code smell :) (Maybe source generator is ideal case for this scenario).
I am willing to help, so you can create new issue and I will test it.
Is it possible to exclude some classes from analyzer (I am using SonarCloud) e.g. using asterisk and create some rule to exclude all *Dto from this concrete rule?
I can try to add that attribute
This will not work because we do not support that attribute yet. It was a solution proposal to allow you to deal with such a problem in the future. I agree with you here: adding the attribute to silence a tool isn't ideal. That's why I was asking if you would add such an attribute if we add support for it. Given your feedback, I will not create an issue as it seems not worth the effort.
I think your proposed solution to exclude the files from the analysis is indeed the better approach. You can do so by specifying sonar.exclusions in SonarCloud.
The constructor's effective visibility is private since it lives in a private class. The wording is not optimal, but I'm struggling to find a better one. Any suggestions?
We and Roslyn call it EffectiveAccessibility
Given that I would say "Remove this effectively private constructor" if the modifier does not match the effective accessibility.
I can try to add that attribute, although I do not like to add to my code more than required for the implementation. Adding additional code just for the sake of the 3rd party tooling is code smell.
The documentation for DynamicallyAccessedMembersAttribute states
Indicates that certain members on a specified Type are accessed dynamically, for example, through System.Reflection.
Adding this attribute is more than just pleasing a third-party tool. It indicates that this seemingly unused class is accessed via reflection at runtime. It therefore also serves documentation purposes to any code reader and satisfies the "clear" clean-code-attribute.
Clear: The code is self-explanatory, transparently communicating its functionality. It is written in a straightforward way that minimizes ambiguity, avoiding unnecessary clever or intricate solutions.
We and Roslyn call it EffectiveAccessibility
Given that I would say "Remove this effectively private constructor" if the modifier does not match the effective accessibility.
I think you forgot the "unused" word, so it will be something like: Remove the unused, effectively private constructor blabla
Which sounds a bit weird to me.