agoda-com / agoda-com/AgodaAnalyzers

AG0051: Reduce false positives — suppress past dates, sentinels, frozen clocks, and copy-through assertions

Đang mở
#241 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
C#
Star
25
Fork
15
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

Issue title


AG0051: Reduce false positives — suppress past dates, sentinels, frozen clocks, and copy-through assertions


Issue body


Summary


AG0051DetectHardcodedDateLiterals currently flags any new DateTime(...)/new DateTimeOffset(...) with all-literal (y, m, d) args and year >= 2020, and any DateTime.Parse("YYYY-MM-DD..."), inside a test context. It never inspects how the value is used, whether the test clock is frozen, or whether the date has already passed.


A triage of a real result set (922 warnings, 106 files, 5 repositories) against a strict time-bomb definition — a test that depends on the real current date internally and has fixed fixture dates that will cross a threshold as wall-clock time advances — found that none of the sampled warnings were confirmed time bombs. The dominant false-positive shapes were:



  1. Fixture dates already years in the past (deterministic DTO/mapper coverage)

  2. Explicitly frozen clocks (FakeTimeProvider, mocked Now/GetUtcNow())

  3. Copy-through / equality-assertion fixtures (Prop = new DateTime(...)result.Prop.ShouldBe(new DateTime(...)))

  4. Open-ended sentinel dates (9999-12-31, 2999-12-31)

  5. Relative-ordering validation where dates are only compared to each other

  6. Pure formatter/parser tests (fixed date in → fixed string out)


This issue proposes four analyzer changes (P1–P4) that eliminate categories 1–4 and most of 6, plus a confidence-tier mechanism (P5) so downstream SCA tooling can measure the redistribution before anything is deleted.


The rule must continue to flag the genuine shape:


// Service reads the real wall clock internally.

var service = new BookingEligibilityService();

var offer = new Offer
{
StartDate = new DateTime(2026, 12, 1), // intended to be "future"
EndDate = new DateTime(2026, 12, 31)
};

service.IsBookable(offer).ShouldBeTrue(); // flips to red after 2026-12-31




P1 — Suppress dates already in the past relative to analysis time


A date that has already passed cannot be a future time bomb: if it were going to flip the test, it already would have. This alone removes the largest false-positive category (historical mock/fixture data such as new DateTime(2020, 12, 21), new DateTime(2021, 1, 1)).


// Replace:

// private const int SafeYearThreshold = 2020;

private static readonly DateTime AnalysisDate = DateTime.UtcNow.Date;

private static bool IsPastDate(int year, int month, int day)
{
if (month < 1 || month > 12 || day < 1 || day > 31) return false;
// Suppress only when the whole month has already elapsed (small grace buffer).
return new DateTime(year, Math.Max(1, month), 1).AddMonths(1) < AnalysisDate;
}


In AnalyzeObjectCreation, the month/day literals are already available in the first three arguments; extract them and bail out when IsPastDate returns true. In AnalyzeInvocation, use DateTime.TryParse on the matched string instead of only extracting the year.


Optionally keep a configurable floor via .editorconfig (e.g. dotnet_diagnostic.AG0051.min_year) for teams that want the old fixed-threshold behavior.


Performance impact: zero — integer comparisons on values already extracted.
Feasibility: trivial.
Trade-off to document: diagnostics become non-deterministic across calendar time — the same commit can be clean in December and warn in January. That is philosophically what the rule is about, but it should be stated in doc/AG0051.md because it can surprise build caching and "who broke main" triage. Computing AnalysisDate once per process (static readonly) keeps a single build self-consistent.




P2 — Suppress open-ended sentinel dates


9999-12-31 / 2999-12-31 / DateTime.MaxValue-adjacent values are domain sentinels for "no end date", not dates that will expire during the test's lifetime.


private static bool IsSentinelDate(int year) => year >= 2999;


One guard in both analyze paths.


Performance impact: zero.
Feasibility: trivial. Highest-precision change in the set — no one writes new DateTime(9999, 12, 31) intending "next quarter".




P3 — Suppress when the test class freezes the clock


If the class freezes time (FakeTimeProvider, SetUtcNow, NSubstitute xxx.Now.Returns(...), Moq Setup(x => x.Now)), fixture dates live in the fake timeline, not the wall clock.


Restructure to RegisterSymbolStartAction (available on the referenced Roslyn 4.5): collect candidate diagnostics and freeze evidence in one class walk; report at SymbolEnd only if no freeze was found.


context.RegisterSymbolStartAction(symbolContext =>

{
var state = new ClassAnalysisState(); // candidate diagnostics + HasFrozenClock flag

symbolContext.RegisterSyntaxNodeAction(c => CollectDateLiteral(c, state),
SyntaxKind.ObjectCreationExpression, SyntaxKind.InvocationExpression);

symbolContext.RegisterSyntaxNodeAction(c => DetectClockFreeze(c, state),
SyntaxKind.ObjectCreationExpression, SyntaxKind.InvocationExpression);

symbolContext.RegisterSymbolEndAction(c =>
{
if (!state.HasFrozenClock)
foreach (var d in state.Candidates) c.ReportDiagnostic(d);
});
}, SymbolKind.NamedType);


Syntactic freeze heuristic (no semantic calls needed for a first cut):


private static bool IsClockFreeze(SyntaxNode node) => node switch

{
ObjectCreationExpressionSyntax o
when o.Type.ToString().Contains("FakeTimeProvider") => true,

InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax m }
when m.Name.Identifier.Text is "SetUtcNow" or "SetLocalTimeZone" => true,

// NSubstitute: provider.Now.Returns(...), provider.GetUtcNow().Returns(...)
InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax { Name.Identifier.Text: "Returns" } m2 }
when m2.Expression.ToString() is var t &&
(t.EndsWith(".Now") || t.EndsWith(".UtcNow") ||
t.EndsWith(".Today") || t.EndsWith("GetUtcNow()")) => true,

_ => false
};


Optionally confirm FakeTimeProvider resolves to Microsoft.Extensions.Time.Testing.FakeTimeProvider with one GetSymbolInfo per candidate.


Performance impact: moderate but acceptable — one extra pattern match per creation/invocation node, still linear in class size. Symbol-start/end analyzers have coarser incremental behavior in the IDE (a keystroke re-analyzes the whole class); acceptable for a test-only rule, and CI cost is unchanged. Side benefit: IsInTestContext currently performs semantic attribute lookups on every node; the symbol-start restructure allows caching it once per class, which likely nets out as a perf improvement.
Feasibility: high for the heuristic version.
Known trade-off: class-level suppression is coarse — one frozen test method suppresses warnings in an unfrozen sibling method. A follow-up refinement is method-level detection first (containing method body), falling back to [SetUp]/constructor/field initializers at class level. Ship coarse first.




P4 — Demote copy-through / equality-assertion usage, keep risky property names flagged


If the literal's only role is "assigned into a mock/expected DTO with a non-risky property name" or "argument to an assertion", it is data-shape coverage, not a wall-clock threshold. This is a bounded, purely syntactic walk up creation.Parent.


private static readonly HashSet<string> AssertionMethods = new()

{ "ShouldBe", "ShouldBeEquivalentTo", "AreEqual", "Equal", "Be", "BeEquivalentTo" };

private static readonly HashSet<string> RiskyPropertyNames = new(StringComparer.OrdinalIgnoreCase)
{ "StartDate", "EndDate", "From", "To", "Expiry", "Expiration", "ExpiresAt",
"CheckIn", "CheckOut", "CheckInDate", "CheckOutDate", "ValidFrom", "ValidTo",
"PayableDate", "FirstLiveDate", "EffectiveDate" };

private static bool IsLowRiskUsage(SyntaxNode creation)
{
switch (creation.Parent)
{
// Prop = new DateTime(...) inside an object initializer
case AssignmentExpressionSyntax { Parent: InitializerExpressionSyntax } assign
when assign.Left is IdentifierNameSyntax id:
return !RiskyPropertyNames.Contains(id.Identifier.Text);

// x.ShouldBe(new DateTime(...)) / Assert.AreEqual(new DateTime(...), ...)
case ArgumentSyntax { Parent.Parent: InvocationExpressionSyntax inv }
when GetInvokedName(inv) is string name && AssertionMethods.Contains(name):
return true;
}
return false;
}


The asymmetry is deliberate: dates assigned to StartDate/CheckIn/Expiry-style properties stay flagged even inside initializers, because that is exactly the shape that feeds wall-clock logic (IsBookable, expiry rules, current-year checks). Dates assigned to arbitrary DTO properties, or sitting inside ShouldBe(...), are suppressed or demoted.


Performance impact: negligible — bounded parent walk with string comparisons, no semantic model.
Feasibility: high.
Known trade-off: false negatives when a blandly-named property (Date = new DateTime(2022, 1, 1)) flows into current-time logic. Combined with P1, most such cases are already-past dates, so the layers compound.




P5 — Emit confidence in the diagnostic properties bag


Rather than hard-suppressing the P4 cases, emit a confidence marker downstream tooling can filter on. The plumbing already exists (KEY_TECH_DEBT_IN_MINUTES ships through Properties today).


var props = Properties.Add("confidence", confidence); // "high" | "low"

context.ReportDiagnostic(Diagnostic.Create(Rule, location, props));

Keep only high-confidence at Warning severity in the IDE (or split into two diagnostic IDs for per-tier .editorconfig control). This de-risks rollout: the redistribution of existing warnings across tiers can be measured on a fresh scan before any behavior is removed.


Performance impact: zero. Feasibility: trivial.




Explicitly out of scope (and why)


"Does production code reachable from the test read the wall clock?" — the highest-value signal, but not feasible inside a Roslyn analyzer: the test compilation references the SUT as a compiled assembly, so metadata symbols are visible but method bodies are not, and Roslyn has no IL inspection. service.IsBookable(offer) is opaque from the test project. This belongs in out-of-band, whole-repo SCA tooling; the analyzer's proxy is the RiskyPropertyNames list in P4.


Local data-flow cases — "dates only compared to each other" (invalid-range validation tests) and "all dates derived from one fixed base date" need RegisterOperationAction + DataFlowAnalysis. Doable but the most complex and slowest piece, and these fixtures age into P1's past-date suppression over time. Defer.




Expected impact on the triage set

False-positive pattern | Eliminated by
-- | --
Historical mock/DTO fixture dates (2020–2021) | P1
Current-year logic with already-past fixture dates | P1
Formatter test with fixed 2024 input | P1
Sentinel 9999-12-31 ranges | P2
NSubstitute frozen Now | P3
FakeTimeProvider.SetUtcNow | P3
DTO copy-through with ShouldBe round-trip | P4
expected*Date modification-date pass-through | P4
Relative-ordering validation (from > to) | deferred (data-flow)
Fixed base date + AddHours offsets | deferred (data-flow; ages into P1)

The genuine time-bomb shape (future date → risky property → unfrozen clock → active/bookable assertion) survives every filter.


Implementation checklist



  • [ ] P1: past-date suppression with process-stable AnalysisDate; TryParse full date in the Parse path; optional .editorconfig floor

  • [ ] P2: sentinel-year guard (year >= 2999)

  • [ ] P4: IsLowRiskUsage parent walk + RiskyPropertyNames keep-list

  • [ ] P5: confidence property on reported diagnostics

  • [ ] Unit tests in AG0051UnitTests style for each suppression and each keep-flagging case (risky property name, future date, unfrozen clock)

  • [ ] Update doc/AG0051.md: new suppression rules + non-determinism note

  • [ ] Follow-up PR — P3: symbol-start restructure with frozen-clock detection (and cached IsInTestContext)

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Start at AG0051DetectHardcodedDateLiterals, especially AnalyzeObjectCreation and AnalyzeInvocation, and trace the existing diagnostic Properties plumbing. Review doc/AG0051.md; done means the proposed past-date, sentinel, frozen-clock, usage, and confidence behaviors are implemented while genuine future-date warnings remain flagged.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
csharp
Lĩnh vực
tooling
Loại issue
Tính năng
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
35/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.