False Positive CSharp - S3457 - when using a property with back field and initialize by a constant?

Rules and Languages Report False-positive / False-negative...
,
1

SonarQube Server: Community Build v25.5.0.107428
No information available about configuration, not maintained by us.

public const String TRACE_FORMAT = "{1:yyyy-MM-dd HH:mm:ss.fff}Z - {2} ({3}) : {4} ({5}) - {6} - {7}";

private static String __TraceFormat = TRACE_FORMAT;

public static String TraceFormat
{
  get => __TraceFormat;
  set
  {
    if(String.IsNullOrWhiteSpace(value))
    {
      value = TRACE_FORMAT;
    }

    __TraceFormat = value;
  }
}

private void WriteLine(Int32 depth,
                       TraceEventType traceEventType,
                       String message,
                       Int32 id)
{
  try
  {
    MethodBase methodBase = null;
    String fullName = null;

    traceEventType.TestEnumValueThrowArgumentOutOfRangeException(nameof(traceEventType));

    message = message ?? "NULL";

    methodBase = new StackTrace().GetFrame(depth)
                                 .GetMethod();

    fullName = $"{methodBase.ReflectedType.FullName.Replace("+", ".")}.{methodBase.Name}";

    _ = this.GetTraceSources(fullName)
            .Where(ts => ts.Switch
                           .Level
                           .HasFlag((SourceLevels)traceEventType))
            .ForEach(ts => ts.Listeners
                             .Cast<TraceListener>()
                             .ToList()
                             .ForEach(tl => tl.WriteLine(String.Format(__TraceFormat,
                                                                       DateTime.Now,
                                                                       DateTime.UtcNow,
                                                                       ProcessName,
                                                                       ProcessId,
                                                                       traceEventType,
                                                                       id,
                                                                       fullName,
                                                                       message,
                                                                       tl.Name,
                                                                       ts.Name))));
  }
  catch(Exception exception)
  {
    this.WriteException(exception);
  }
}

It reports the value of __TraceFormat in WriteLine. Yes there is no 0 indexed value in the initial string. __TraceFormat get initialized with TRACE_FORMAT, which don’t contain a placeholder for index 0. But the developer is able to change the __TraceFormat by static property TraceFormat and then can use the index 0 also.

3

I would argue that this code is hard to read. What you could do, both to improve readability, and bypass S3457 is the following:

internal class S3457
{
	public const string TRACE_FORMAT = "{0:yyyy-MM-dd HH:mm:ss.fff}Z - {1} ({2}) : {3} ({4}) - {5} - {6}";

	public static string TraceFormat { get; set; }

	private void WriteLine(int depth, TraceEventType traceEventType, string message, int id)
	{
		try
		{
			MethodBase methodBase = null;
			string fullName = null;

			traceEventType.TestEnumValueThrowArgumentOutOfRangeException(nameof(traceEventType));

			message = message ?? "NULL";

			methodBase = new StackTrace().GetFrame(depth).GetMethod();

			fullName = $"{methodBase.ReflectedType.FullName.Replace("+", ".")}.{methodBase.Name}";

			_ = this.GetTraceSources(fullName)
					.Where(ts => ts.Switch
								   .Level
								   .HasFlag((SourceLevels)traceEventType))
					.ForEach(ts => ts.Listeners
									 .Cast<TraceListener>()
									 .ToList()
									 .ForEach(tl => tl.WriteLine(Format()));
		}
		catch (Exception exception)
		{
			this.WriteException(exception);
		}

		string Format() => TraceFormat is { Length: > 0 }
		? string.Format(TraceFormat, DateTime.Now, DateTime.UtcNow, ProcessName, ProcessId, traceEventType, id, fullName, message, tl.Name, ts.Name)
		: string.Format(TRACE_FORMAT, DateTime.UtcNow, ProcessName, ProcessId, traceEventType, id, fullName, message, tl.Name, ts.Name);
	}
}

By doing so, you do not need an underlying value or setter logic. Is clear from the Format() method that once specified, you can switch between DateTime.Now and DateTime.UtcNow, but if not the latter is applied. You even could (and may be should) use string interpolation for the latter case.

I would not advise Sonar to change the behavior of this rule.

1 Like
4

Thanks for sharing. On other hand your approach increases the complexity from your SonarQube measure point of view.

5

I’m not working for Sonar (I’m a volunteer). I think this code would benefit from more refactoring.

Another thing that you could consider (based on how it is used) is something like this:

public static bool UtcBased { get; set; } = true;

public DateTime Now() => UtcBased ? DateTime.UtcNow : DateTime.Now;

or

private void WriteLine(int depth, TraceEventType traceEventType, string message, int id, bool utcBased)
// ...

So, this are just my 2cts

1 Like
6

Ok, that’s a good idea in general.

In that case I wouldn’t add the parameter for UTC based in case the property is static, so a direct use of the static property would be preferred.

8

Hello @HBoskugelS,

To me, this is not a false positive.
The rule’s intent is to prevent common pitfalls and errors when using a composite string.
Skipping the index 0 makes the code hard to understand.

In my opinion, the alternatives suggested by @Corniel convey the real intention of what you would like to do with the format.

I hope the alternatives worked for you!

Have a nice day!