Skip to content

Fix sentence-case letterless bug, add regex timeout, tidy the generator - #161

Merged
matt-edmondson merged 1 commit into
mainfrom
chore/sonar-generators
Aug 14, 2026
Merged

Fix sentence-case letterless bug, add regex timeout, tidy the generator#161
matt-edmondson merged 1 commit into
mainfrom
chore/sonar-generators

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes 9 more SonarCloud issues. Two are genuine defects rather than style.

Real defects

S2589 — IsSentenceCase rejected letterless values

FirstOrDefault over a string yields char, not char?, returning 0 when no letter is present. Assigning that to a char? made HasValue always true, so the guard the author wrote — only check the first letter if there is one — never fired:

char? firstLetter = value.FirstOrDefault(char.IsLetter);
if (firstLetter.HasValue && !char.IsUpper(firstLetter.Value))   // HasValue is always true

A value such as "123" therefore reached the uppercase check, char.IsUpper(0) returned false, and it was rejected — even though "" is accepted, which is the same "no case to check" situation.

⚠️ Behaviour change. "123" and "42 - 7 = 35" now validate as sentence case where they previously threw ArgumentException. That matches the written intent and the empty-string precedent, but it is a real change — say the word if you would rather keep letterless values invalid and I will instead make the rejection explicit.

A test pins the new behaviour.

S6444 — PatternValidationRule had no regex timeout

Both the pattern and the value being validated come from the caller, so a pathological combination could backtrack unboundedly (catastrophic backtracking / ReDoS). The Regex now carries a one-second match timeout, which surfaces as RegexMatchTimeoutException rather than hanging the caller.

Generator cleanup

Rule Count Change
S3923 2 The operator doc comment interpolated (op.Op == "*" ? "by" : "by") — the same value either way. Replaced with the literal.
S2325 2 EmitVectorType and EmitVectorOverloadType are now static.
S1172 + S107 3 Dropped the unused codeBlocker parameter from GenerateInner, and the unused typeName from AddUnitFactories — taking that method from 8 parameters to the 7 allowed.

Removing codeBlocker also revealed that one of GenerateInners two callers was allocating a CodeBlocker purely to pass it; that is gone too.

Generated output is byte-identical — both S3923 ternary branches already produced the same string, and the removed parameters were unused. Verified by zero drift under Semantics.Quantities/Generated/.

Verification

  • dotnet build — 0 errors, 0 warnings, all target frameworks
  • dotnet test1089/1089 pass
  • No generated-file drift

Remaining after this — ~57

S3776 cognitive complexity ×9 (mostly QuantitiesGenerator, now at 84% coverage so safer to refactor), S1133 deprecation TODOs ×10, S1192 ×15 in the generators, S4136 ×8 in SemanticString.cs, S1172 ×4 (the remaining unused generator parameters), S3267 ×6, S2342 ×2, S4144 ×1.

…enerator

Two of these are genuine defects rather than style:

- S2589 - IsSentenceCase never skipped letterless values. FirstOrDefault
  over a string yields char, not char?, so it returns \x27\0\x27 when no letter is
  present; assigning that to a char? made HasValue always true. The guard
  the author wrote ("only check the first letter if there is one") therefore
  never fired, and a value such as "123" reached the uppercase check and was
  rejected - even though "" is accepted. Now compares against the \x27\0\x27
  sentinel, with a test pinning that letterless values are valid.
  NOTE: this is a behaviour change. "123" and "42 - 7 = 35" now validate as
  sentence case where they previously threw.

- S6444 - PatternValidationRule built its Regex with no match timeout.
  Both the pattern and the value come from the caller, so a pathological
  combination could backtrack unboundedly. Added a one second timeout, which
  surfaces as RegexMatchTimeoutException instead of hanging.

The rest are cleanup in QuantitiesGenerator:
- S3923 (x2): the operator doc comment interpolated
  (op.Op == "*" ? "by" : "by") - the same value either way. Replaced with the
  literal. Generated output is byte-identical, verified by no drift under
  Semantics.Quantities/Generated.
- S2325 (x2): EmitVectorType and EmitVectorOverloadType are now static.
- S1172 (x2) and S107: dropped the unused codeBlocker parameter from
  GenerateInner - which also made the CodeBlocker its second caller
  allocated purely to pass it dead - and the unused typeName parameter from
  AddUnitFactories, taking that method from 8 parameters to the 7 allowed.

Build clean on all TFMs, 1089/1089 tests pass, no generated-file drift.
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
50.0% Coverage on New Code (required ≥ 80%)
5.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@matt-edmondson
matt-edmondson merged commit b2eb55d into main Aug 14, 2026
5 of 6 checks passed
@matt-edmondson
matt-edmondson deleted the chore/sonar-generators branch August 14, 2026 08:31
Damon3000s pushed a commit to Damon3000s/Semantics that referenced this pull request Aug 16, 2026
…t PatternValidationRule

Local Sonar reproduction (ported from KtsuBuild):
  dotnet build -p:CustomBeforeMicrosoftCommonProps=$PWD/.sonarlint/sonar-local.props
CI injects the Sonar analyzers via the scanner, so a plain build never runs
them and findings only surface after a push - a ~10 minute round trip per
attempt, which this session spent a lot of time on. Documented in CLAUDE.md
including the limitation that it currently only reaches
Semantics.SourceGenerators: that project declares its SDK with the
<Project Sdk="..."> attribute form, and CustomBeforeMicrosoftCommonProps does
not reach the ktsu.Sdk projects that use <Sdk Name="..." /> elements.

S3776 - AdjustForContrast was at complexity 19:
- The WCAG thresholds were spelled out twice, as a switch here and as a pair
  of ternaries in AccessibilityLevelAgainst. Both now call RequiredContrast.
- The binary search had a four-way branch. The interval always shrinks toward
  the end satisfying the requirement, and when darkening the roles simply
  swap, so it collapses to "did the midpoint land on the goLighter side".

PatternValidationRule had no tests at all, which is what failed the coverage
gate on ktsu-dev#161 and matters more now that it carries a match timeout. Added six,
including one proving the catastrophic-backtracking case
("^(a+)+$" against a non-matching run of 40 a s) raises
RegexMatchTimeoutException instead of hanging - so the S6444 fix from ktsu-dev#161 is
now actually verified rather than assumed.

Build clean on all TFMs, 1095/1095 tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant