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.

Sign up for GitHub

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

Jump to bottom

Fix S6588 FN: Rule should cover case with epoch ticks #7550

Merged
merged 8 commits into from
Jul 10, 2023
Merged

Fix S6588 FN: Rule should cover case with epoch ticks #7550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1bd94d3
First implementation
CristianAmbrosini Jul 6, 2023
64ab097
Simplify implementation
CristianAmbrosini Jul 6, 2023
1c7caeb
Add compliant case
CristianAmbrosini Jul 6, 2023
61a1fcf
Fix null check
CristianAmbrosini Jul 7, 2023
19c362a
Address comments
CristianAmbrosini Jul 10, 2023
cfb791c
Add underscore, binary, hex UTs for VB
CristianAmbrosini Jul 10, 2023
99158df
Improve coverage
CristianAmbrosini Jul 10, 2023
ff82b9a
Go back to decimal
CristianAmbrosini Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions analyzers/src/SonarAnalyzer.Common/Rules/UseUnixEpochBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public abstract class UseUnixEpochBase<TSyntaxKind, TLiteralExpression, TMemberA
where TLiteralExpression : SyntaxNode
where TMemberAccessExpression : SyntaxNode
{
private const long EpochTicks = 621_355_968_000_000_000;
private const int EpochYear = 1970;
private const int EpochMonth = 1;
private const int EpochDay = 1;
Expand All @@ -57,23 +58,29 @@ protected sealed override void Initialize(SonarAnalysisContext context) =>
Language.GeneratedCodeRecognizer,
c =>
{
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>();

if (literalsArguments.Any(x => IsValueEqualTo(x, EpochYear)
Copy link
Contributor

Choose a reason for hiding this comment

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

The two if statements can be merged if you move CheckAndGetTypeName to the end of the conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

&& literalsArguments.Count(x => IsValueEqualTo(x, EpochMonth)) == 2)
&& CheckAndGetTypeName(c.Node, c.SemanticModel) is { } name
&& IsEpochCtor(c.Node, c.SemanticModel))
{
return;
c.ReportIssue(Diagnostic.Create(Rule, c.Node.GetLocation(), name));
}

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

This can also be done inside the IsEpochCtor method:

|| IsParameterExistingAndLiteralEqualTo("ticks", EpochYear, lookup)

|| (Language.FindConstantValue(c.SemanticModel, arguments.First()) is long ticks && ticks == EpochTicks))
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous check is just for the hardcoded number:

_ = new DateTime(621355968000000000); // Noncompliant

while this check is actually looking for constants through the tracker:

private const long EpochTicks = 621355968000000000;
...
_ = new DateTime(EpochTicks); // Noncompliant

&& CheckAndGetTypeName(c.Node, c.SemanticModel) is { } typeName)
{
c.ReportIssue(Diagnostic.Create(Rule, c.Node.GetLocation(), type.Name));
c.ReportIssue(Diagnostic.Create(Rule, c.Node.GetLocation(), typeName));
}
},
Language.SyntaxKind.ObjectCreationExpressions);
});

protected static bool IsValueEqualTo(TLiteralExpression literal, int value) =>
int.TryParse(literal.ChildTokens().First().ValueText, out var parsedValue) && parsedValue == value;
protected static bool IsValueEqualTo(TLiteralExpression literal, long value) =>
long.TryParse(literal.ChildTokens().First().ValueText, out var parsedValue) && parsedValue == value;

private bool IsEpochCtor(SyntaxNode node, SemanticModel model)
{
Expand Down Expand Up @@ -102,6 +109,9 @@ private static bool IsParameterNonExistingOrLiteralEqualTo(string parameterName,
private static bool IsLiteralAndEqualTo(SyntaxNode node, int value) =>
node is TLiteralExpression literal && IsValueEqualTo(literal, value);

private static string CheckAndGetTypeName(SyntaxNode node, SemanticModel model) =>
model.GetTypeInfo(node).Type is var type && type.IsAny(TypesWithUnixEpochField) ? type.Name : null;

private bool IsDateTimeKindNonExistingOrUtc(IMethodParameterLookup lookup) =>
!lookup.TryGetSyntax("kind", out var expressions)
|| (expressions[0] is TMemberAccessExpression memberAccess && IsDateTimeKindUtc(memberAccess));
Expand Down
19 changes: 18 additions & 1 deletion analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UseUnixEpoch.Fixed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ public class Program

private readonly DateTimeOffset EpochOff = DateTimeOffset.UnixEpoch; // Fixed

private const long EpochTicks = 621355968000000000;
private const long EpochTicksUnderscores = 621_355_968_000_000_000;
private const long EpochTicksBinary = 0b100010011111011111111111010111110111101101011000000000000000;
private const long EpochTicksHex = 0x89F7FF5F7B58000;
private const long SomeLongConst = 6213;

void BasicCases(DateTime dateTime)
{
var timeSpan = dateTime - DateTime.UnixEpoch; // Fixed
Expand Down Expand Up @@ -46,6 +52,12 @@ void DateTimeConstructors(int ticks, int year, int month, int day, int hour, int
var ctor1_0 = new DateTime(1970); // Compliant
var ctor1_1 = new DateTime(ticks); // Compliant
var ctor1_2 = new DateTime(ticks: ticks); // Compliant
var ctor1_3 = DateTime.UnixEpoch; // Fixed
var ctor1_4 = DateTime.UnixEpoch; // Fixed
var ctor1_5 = DateTime.UnixEpoch; // Fixed
var ctor1_6 = DateTime.UnixEpoch; // Fixed
var ctor1_7 = DateTime.UnixEpoch; // Fixed
var ctor1_8 = new DateTime(SomeLongConst); // Compliant

// year, month, and day
var ctor2_0 = DateTime.UnixEpoch; // Fixed
Expand Down Expand Up @@ -202,10 +214,15 @@ void DateTimeOffsetConstructors(TimeSpan timeSpan, DateTime dateTime, int ticks,

public class FakeDateTime
{
void MyMethod() => new DateTime(1970, 1, 1); // Compliant
void MyMethod()
{
_ = new DateTime(1970, 1, 1); // Compliant
_ = new DateTime("hello"); // Compliant
}

public class DateTime
{
public DateTime(int year, int month, int day) { }
public DateTime(string ticks) { }
}
}
12 changes: 12 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UseUnixEpoch.Fixed.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ Public Class Program

Private ReadOnly EpochOff As DateTimeOffset = DateTimeOffset.UnixEpoch ' Fixed

Private Const EpochTicks As Long = 621355968000000000
Private Const EpochTicksUnderscores As Long = 621_355_968_000_000_000
Private Const EpochTicksBinary As Long = &B100010011111011111111111010111110111101101011000000000000000
Private Const EpochTicksHex As Long = &H89F7FF5F7B58000
Private Const SomeLongConst As Long = 6213

Private Sub BasicCases(ByVal dateTime As Date)
Dim timeSpan = dateTime - DateTime.UnixEpoch ' Fixed

Expand Down Expand Up @@ -40,6 +46,12 @@ Public Class Program
Dim ctor1_0 = New DateTime(1970) ' Compliant
Dim ctor1_1 = New DateTime(ticks) ' Compliant
Dim ctor1_2 = New DateTime(ticks:=ticks) ' Compliant
Dim ctor1_3 = DateTime.UnixEpoch ' Fixed
Dim ctor1_4 = DateTime.UnixEpoch ' Fixed
Dim ctor1_5 = DateTime.UnixEpoch ' Fixed
Dim ctor1_6 = DateTime.UnixEpoch ' Fixed
Dim ctor1_7 = DateTime.UnixEpoch ' Fixed
Dim ctor1_8 = New DateTime(SomeLongConst) ' Compliant

' year, month, and day
Dim ctor2_0 = DateTime.UnixEpoch ' Fixed
Expand Down
19 changes: 18 additions & 1 deletion analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UseUnixEpoch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ public class Program
private readonly DateTimeOffset EpochOff = new DateTimeOffset(1970, 1, 1, 0, 0, 0, 0, TimeSpan.Zero); // Noncompliant {{Use "DateTimeOffset.UnixEpoch" instead of creating DateTimeOffset instances that point to the unix epoch time}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

private const long EpochTicks = 621355968000000000;
private const long EpochTicksUnderscores = 621_355_968_000_000_000;
private const long EpochTicksBinary = 0b100010011111011111111111010111110111101101011000000000000000;
private const long EpochTicksHex = 0x89F7FF5F7B58000;
private const long SomeLongConst = 6213;

void BasicCases(DateTime dateTime)
{
var timeSpan = dateTime - new DateTime(1970, 1, 1); // Noncompliant
Expand Down Expand Up @@ -48,6 +54,12 @@ void DateTimeConstructors(int ticks, int year, int month, int day, int hour, int
var ctor1_0 = new DateTime(1970); // Compliant
var ctor1_1 = new DateTime(ticks); // Compliant
var ctor1_2 = new DateTime(ticks: ticks); // Compliant
var ctor1_3 = new DateTime(621355968000000000); // Noncompliant
var ctor1_4 = new DateTime(EpochTicks); // Noncompliant: const variables are tracked
var ctor1_5 = new DateTime(EpochTicksUnderscores); // Noncompliant: const variables are tracked
var ctor1_6 = new DateTime(EpochTicksBinary); // Noncompliant: const variables are tracked
var ctor1_7 = new DateTime(EpochTicksHex); // Noncompliant: const variables are tracked
var ctor1_8 = new DateTime(SomeLongConst); // Compliant

// year, month, and day
var ctor2_0 = new DateTime(1970, 1, 1); // Noncompliant
Expand Down Expand Up @@ -204,10 +216,15 @@ void DateTimeOffsetConstructors(TimeSpan timeSpan, DateTime dateTime, int ticks,

public class FakeDateTime
{
void MyMethod() => new DateTime(1970, 1, 1); // Compliant
void MyMethod()
{
_ = new DateTime(1970, 1, 1); // Compliant
_ = new DateTime("hello"); // Compliant
}

public class DateTime
{
public DateTime(int year, int month, int day) { }
public DateTime(string ticks) { }
}
}
12 changes: 12 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UseUnixEpoch.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ Public Class Program
Private ReadOnly EpochOff As DateTimeOffset = New DateTimeOffset(1970, 1, 1, 0, 0, 0, 0, TimeSpan.Zero) ' Noncompliant {{Use "DateTimeOffset.UnixEpoch" instead of creating DateTimeOffset instances that point to the unix epoch time}}
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Private Const EpochTicks As Long = 621355968000000000
csaba-sagi-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
Private Const EpochTicksUnderscores As Long = 621_355_968_000_000_000
Private Const EpochTicksBinary As Long = &B100010011111011111111111010111110111101101011000000000000000
Private Const EpochTicksHex As Long = &H89F7FF5F7B58000
Private Const SomeLongConst As Long = 6213

Private Sub BasicCases(ByVal dateTime As Date)
Dim timeSpan = dateTime - New DateTime(1970, 1, 1) ' Noncompliant

Expand Down Expand Up @@ -42,6 +48,12 @@ Public Class Program
Dim ctor1_0 = New DateTime(1970) ' Compliant
Dim ctor1_1 = New DateTime(ticks) ' Compliant
Dim ctor1_2 = New DateTime(ticks:=ticks) ' Compliant
Dim ctor1_3 = New DateTime(621355968000000000) ' Noncompliant
Dim ctor1_4 = New DateTime(EpochTicks) ' Noncompliant: const variables are tracked
Dim ctor1_5 = New DateTime(EpochTicksUnderscores) ' Noncompliant: const variables are tracked
Dim ctor1_6 = New DateTime(EpochTicksBinary) ' Noncompliant: const variables are tracked
Dim ctor1_7 = New DateTime(EpochTicksHex) ' Noncompliant: const variables are tracked
Dim ctor1_8 = New DateTime(SomeLongConst) ' Compliant

' year, month, and day
Dim ctor2_0 = New DateTime(1970, 1, 1) ' Noncompliant
Expand Down