Fix S6588 FN: Rule should cover case with epoch ticks #7550
Conversation
cristian-ambrosini-sonarsource
requested review from
csaba-sagi-sonarsource and
zsolt-kolbay-sonarsource
| var literalsArguments = Language.Syntax.ArgumentExpressions(c.Node).OfType<TLiteralExpression>(); | ||
| if (!literalsArguments.Any(x => IsValueEqualTo(x, EpochYear) || literalsArguments.Count(x => IsValueEqualTo(x, EpochMonth)) != 2)) | ||
| var arguments = Language.Syntax.ArgumentExpressions(c.Node); | ||
| var literalsArguments = arguments?.OfType<TLiteralExpression>(); |
There was a problem hiding this comment.
If arguments can be null then literalsArguments needs to be checked before calling Any() on it in the if statement. Otherwise the ?. must be changed to .
| if (type.IsAny(TypesWithUnixEpochField) && IsEpochCtor(c.Node, c.SemanticModel)) | ||
| else if (arguments.Count() == 1 | ||
| && ((literalsArguments.Count() == 1 && IsValueEqualTo(literalsArguments.First(), EpochTicks)) | ||
| || (Language.FindConstantValue(c.SemanticModel, arguments.First()) is long ticks && ticks == EpochTicks)) |
There was a problem hiding this comment.
I don't think this extra check is necessary. The previous check is more than enough. If someone gives the Epoch ticks in hexa or binary format, then I think we can live with that 😎
There was a problem hiding this comment.
The previous check is just for the hardcoded number:
_ = new DateTime(621355968000000000); // Noncompliantwhile this check is actually looking for constants through the tracker:
private const long EpochTicks = 621355968000000000;
...
_ = new DateTime(EpochTicks); // Noncompliant| var arguments = Language.Syntax.ArgumentExpressions(c.Node); | ||
| var literalsArguments = arguments?.OfType<TLiteralExpression>(); | ||
|
|
||
| if (literalsArguments.Any(x => IsValueEqualTo(x, EpochYear) |
There was a problem hiding this comment.
The two if statements can be merged if you move CheckAndGetTypeName to the end of the conditions.
There was a problem hiding this comment.
Unfortunately, the conditions are different: the first one is checking whether you have 3 literals (2 with number 1, 1 with number 1970) and then proceeds to check the type and constructor.
The second one is only searching for the constructor with a single argument of 621355968000000000 or pointing at a const with that value, and then proceeds to check semantically the type (not the constructor, since we have only 1 ctor with 1 argument for DateTime/DateTimeOffset).
Although it's feasible to merge everything in a single if statement, all the variants I tried are really cumbersome to read and understand. Happy to discuss it further
| var type = c.SemanticModel.GetTypeInfo(c.Node).Type; | ||
| if (type.IsAny(TypesWithUnixEpochField) && IsEpochCtor(c.Node, c.SemanticModel)) | ||
| else if (arguments.Count() == 1 | ||
| && ((literalsArguments.Count() == 1 && IsValueEqualTo(literalsArguments.First(), EpochTicks)) |
There was a problem hiding this comment.
This can also be done inside the IsEpochCtor method:
|| IsParameterExistingAndLiteralEqualTo("ticks", EpochYear, lookup)
| @@ -33,7 +33,8 @@ public abstract class UseUnixEpochBase<TSyntaxKind, TLiteralExpression, TMemberA | |||
| where TLiteralExpression : SyntaxNode | |||
| where TMemberAccessExpression : SyntaxNode | |||
| { | |||
| private const int EpochYear = 1970; | |||
| private const long EpochTicks = 0x89F7FF5F7B58000; | |||
There was a problem hiding this comment.
Why did you change this to hexa? In my opinion it is more understandable in decimal. We use that value more often than hexa numbers.
There was a problem hiding this comment.
I think we should go with decimal with underscores: 621_355_968_000_000_000
| @@ -33,7 +33,8 @@ public abstract class UseUnixEpochBase<TSyntaxKind, TLiteralExpression, TMemberA | |||
| where TLiteralExpression : SyntaxNode | |||
| where TMemberAccessExpression : SyntaxNode | |||
| { | |||
| private const int EpochYear = 1970; | |||
| private const long EpochTicks = 0x89F7FF5F7B58000; | |||
| private const int EpochYear = 1970; // 621_355_968_000_000_000 | |||
There was a problem hiding this comment.
Why this comment at the year? That is the number of ticks. If we want to add that as a comment it should go with the ticks. However, I think it is not needed.
There was a problem hiding this comment.
This comment was about not forgetting to add once again it in decimal. I failed :D. Thanks for noticing it I'll fix it immediately
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed! |
Fixes #7547