Rule S1144 is wrong for property setters of deserialized classes

[jaguire]

Description

Sonarlint tells me to remove the setters for classes that are created via deserialization. Doing so causes an error on deserialization. I have seen this using Newtonsoft's Json.Net and ORM mapping tools.

Repro steps

void Main()
{
    var json = "{ \"Name\": \"Jared\" }";
    var model = JsonConvert.DeserializeObject<Person>(json);
    Console.Write(model.Name);
}

public class Person
{
    public string Name { get; set; } // Sonarlint tells me to remove the setter.
}

Expected behavior

I would expect the linter to see that the class gets created by a deserialization method and then assume the setters are used. This may not be possible in every scenario, but the common ones should be supported. (Json.Net, DataContractJsonSerializer, BinaryFormatter, etc...)

Actual behavior

The linter tells me the setter is not used and I can remove it. Doing so causes an exception when trying to deserialize to the class.

Known workarounds

#pragma warning disable S1144 // Unused private types or members should be removed

Related information

Using SonarLint for Visual Studio 2017 v3.10.0.3095

[jaguire]

This may also apply to rule S3459 - Unassigned members should be removed. When the class is only used as a model for an MVC view, the linter doesn't see the properties being used.

[Evangelink]

Hi @jaguire,

Thanks for your feedback! As you may have seen we have quite a lot of problems (false positives) with this rule so I think we need to redesign it completely.

Cheers, Amaury

[valhristov]

Hi @jaguire, in order to not suggest a property setter for removal we need to somehow detect it is being used. In case of serialization, it is difficult, because as you say, there are many ways a string could be deserialized into a type instance. And even if we white-list some, the serializer could be abstracted behind an interface (a common thing IMO) and it would be difficult to detect even the supported serializers.

There is another workaround, though - if you put [JsonProperty("name")] attributes on the deserialized properties, S1144 will not complain about the setters. I usually like to put the attributes to prevent occasional refactoring from breaking contracts.

[BartHuls]

Is this already solved. We still get a lot of these False Positives, and we won't want to add the [JsonProperty("name")] attributes just to get rid of this error

[amitagwl17]

you can use 'init' instead of 'set', this way u won't get any sonar issue.

[BartHuls]

Nope this will not work. I still get the S1144 and the S3459 Code Smell

[jaguire]

you can use 'init' instead of 'set', this way u won't get any sonar issue.

Just like @BartHuls, that doesn't work for me. Also, init won't work for properties that need to be writeable.

Perhaps the linter could look for known deserialization methods, like JsonConvert.DeserializeObject<Person>(json), and dynamically ignore that class for this rule. Or even better, change the errors to warnings and suggest adding the [JsonProperty("name")] attribute.