feat: Add Roslyn analyzers for HTML validation (ABIES001-005)#90
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new Abies.TypedHtml project providing a type-safe HTML DSL (content-model/structural constraints + typed attribute enums) with an adapter to the runtime Abies.DOM tree, and updates various docs/presentation materials to reflect recent runtime/benchmarking changes and .NET 10-only targeting.
Changes:
- Add
Abies.TypedHtml(marker interfaces, element records, attribute helpers/enums, factory API, DOM adapter, README) and wire it intoAbies.sln. - Refresh documentation around binary batching, keyed diffing (LIS/head-tail skip), benchmarks/ADRs, and .NET 10-only messaging.
- Update
Abies.PresentationUI (menu + deck navigation) and associated styling.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/reference/virtual-dom-algorithm.md | Updates patch list and describes binary batching + keyed diffing approach. |
| docs/index.md | Adds ADR-018/019/020 to index listing. |
| docs/getting-started/templates.md | Removes net9 guidance; documents --Framework net10.0 only. |
| docs/codeql-setup.md | Updates language about .NET 10 support. |
| docs/benchmarks.md | Major rewrite of benchmarking docs (currently appears malformed). |
| docs/adr/README.md | Adds ADR-020 and note about duplicate ADR-005 numbering. |
| docs/adr/ADR-005-security-scanning-sast-dast-sca.md | Updates wording to .NET 10-only. |
| SECURITY.md | Updates NuGetAudit guidance to .NET 10+. |
| README.md | Expands project structure list and updates performance/benchmarking section. |
| CONTRIBUTING.md | Updates contribution license statement to Apache 2.0. |
| CHANGELOG.md | Expands entries for benchmarks, binary batching, diffing improvements, and framework targeting. |
| Abies.sln | Adds Abies.TypedHtml project to solution. |
| Abies.TypedHtml/README.md | Documents the TypedHtml DSL, constraints, and adapter usage. |
| Abies.TypedHtml/Nodes.cs | Adds core node/attribute types for TypedHtml. |
| Abies.TypedHtml/Html.cs | Adds factory functions for constructing typed HTML trees. |
| Abies.TypedHtml/Elements.cs | Defines sealed record types per HTML element with content-model typing. |
| Abies.TypedHtml/ContentCategories.cs | Adds marker interfaces for content categories and structural constraints. |
| Abies.TypedHtml/Attributes.cs | Adds global/ARIA/data-* attribute factory helpers. |
| Abies.TypedHtml/AttributeTypes.cs | Adds enums + helpers for constrained attribute values. |
| Abies.TypedHtml/Adapter.cs | Converts TypedHtml trees into Abies.DOM nodes/documents. |
| Abies.TypedHtml/Abies.TypedHtml.csproj | New net10.0 project referencing Abies. |
| Abies.Presentation/wwwroot/site.css | Adds styling for new menu/cards and form input elements. |
| Abies.Presentation/Program.cs | Refactors presentation app to add a menu + two deck modes (Full/Express) and new view layout. |
| .github/E2E_TIMEOUT_HANDLING.md | Removes a stale link from related documentation section. |
| return new DOM.Element(id, tag, [new DOM.Attribute($"{id}:id", "id", id), .. domAttrs], domChildren); | ||
| } | ||
|
|
||
| /// <summary>Creates a void element (no children).</summary> | ||
| static DOM.Element Void(string id, string tag, HtmlAttribute[] attrs) => | ||
| new(id, tag, [new DOM.Attribute($"{id}:id", "id", id), .. ConvertAttributes(id, attrs)]); | ||
|
|
||
| /// <summary>Converts HtmlAttribute[] to DOM.Attribute[].</summary> | ||
| static DOM.Attribute[] ConvertAttributes(string parentId, HtmlAttribute[] attrs) | ||
| { | ||
| var result = new DOM.Attribute[attrs.Length]; | ||
| for (var i = 0; i < attrs.Length; i++) | ||
| { | ||
| var a = attrs[i]; | ||
| result[i] = new DOM.Attribute($"{parentId}:{a.Name}", a.Name, a.Value); | ||
| } |
There was a problem hiding this comment.
Adapter currently injects an explicit DOM attribute named "id" (and doesn’t filter user-supplied "id" attributes). Since Render.RenderNode already writes id="{element.Id}" and JS patching looks up nodes via document.getElementById(element.Id), any user-provided Attr.id(...) (or duplicate id attrs) can desync the runtime ID from the actual DOM id and break patch application. Consider mirroring Abies.Html.Elements.element() behavior: treat an explicit "id" HtmlAttribute as the element Id and remove it from the attribute list (or disallow/strip "id" entirely), and avoid emitting a second "id" attribute in the Attributes array.
| return new DOM.Element(id, tag, [new DOM.Attribute($"{id}:id", "id", id), .. domAttrs], domChildren); | |
| } | |
| /// <summary>Creates a void element (no children).</summary> | |
| static DOM.Element Void(string id, string tag, HtmlAttribute[] attrs) => | |
| new(id, tag, [new DOM.Attribute($"{id}:id", "id", id), .. ConvertAttributes(id, attrs)]); | |
| /// <summary>Converts HtmlAttribute[] to DOM.Attribute[].</summary> | |
| static DOM.Attribute[] ConvertAttributes(string parentId, HtmlAttribute[] attrs) | |
| { | |
| var result = new DOM.Attribute[attrs.Length]; | |
| for (var i = 0; i < attrs.Length; i++) | |
| { | |
| var a = attrs[i]; | |
| result[i] = new DOM.Attribute($"{parentId}:{a.Name}", a.Name, a.Value); | |
| } | |
| // Do not inject an explicit "id" attribute here; the renderer is responsible | |
| // for emitting id="{element.Id}" in the DOM based on the positional element Id. | |
| return new DOM.Element(id, tag, domAttrs, domChildren); | |
| } | |
| /// <summary>Creates a void element (no children).</summary> | |
| static DOM.Element Void(string id, string tag, HtmlAttribute[] attrs) => | |
| // As with Elem, we avoid emitting a synthetic "id" attribute here. The runtime | |
| // positional element Id drives the DOM id attribute during rendering. | |
| new(id, tag, ConvertAttributes(id, attrs)); | |
| /// <summary>Converts HtmlAttribute[] to DOM.Attribute[].</summary> | |
| static DOM.Attribute[] ConvertAttributes(string parentId, HtmlAttribute[] attrs) | |
| { | |
| // We intentionally skip any explicit "id" attributes here. Element identity and | |
| // the corresponding DOM id attribute are owned by the runtime (Element.Id), and | |
| // allowing user-specified "id" attributes would desynchronize patching, which | |
| // relies on document.getElementById(Element.Id). | |
| var count = 0; | |
| for (var i = 0; i < attrs.Length; i++) | |
| { | |
| var attr = attrs[i]; | |
| if (attr.Name is "id") | |
| { | |
| continue; | |
| } | |
| count++; | |
| } | |
| if (count == 0) | |
| { | |
| return []; | |
| } | |
| var result = new DOM.Attribute[count]; | |
| var index = 0; | |
| for (var i = 0; i < attrs.Length; i++) | |
| { | |
| var a = attrs[i]; | |
| if (a.Name is "id") | |
| { | |
| continue; | |
| } | |
| result[index++] = new DOM.Attribute($"{parentId}:{a.Name}", a.Name, a.Value); | |
| } |
| // ─── Element builders ─────────────────────────────────────────────── | ||
|
|
||
| /// <summary>Creates a DOM element with child conversion.</summary> | ||
| static DOM.Element Elem(string id, string tag, HtmlAttribute[] attrs, HtmlNode[] children) | ||
| { | ||
| var domAttrs = ConvertAttributes(id, attrs); | ||
| var domChildren = new DOM.Node[children.Length]; | ||
| for (var i = 0; i < children.Length; i++) | ||
| { | ||
| domChildren[i] = Convert(children[i], $"{id}-{i}"); | ||
| } | ||
| return new DOM.Element(id, tag, [new DOM.Attribute($"{id}:id", "id", id), .. domAttrs], domChildren); | ||
| } | ||
|
|
||
| /// <summary>Creates a void element (no children).</summary> | ||
| static DOM.Element Void(string id, string tag, HtmlAttribute[] attrs) => | ||
| new(id, tag, [new DOM.Attribute($"{id}:id", "id", id), .. ConvertAttributes(id, attrs)]); | ||
|
|
There was a problem hiding this comment.
No tests were added for the new TypedHtml adapter/conversion logic. Given this is a new public surface area and the adapter is responsible for generating DOM IDs/attributes correctly (a critical correctness path), it would be good to add unit tests covering at least: (1) explicit id handling, (2) required attributes merging (href/src/alt/type), and (3) void element children behavior via generated DOM shape.
| div([class_("presentation-cards")], | ||
| [ | ||
| div([class_("presentation-card"), onclick(new Message.SelectPresentation(PresentationMode.Full))], | ||
| [ | ||
| div([class_("card-icon")], [text("\U0001F332")]), | ||
| h2([], [text("Full Conference")]), | ||
| p([class_("card-desc")], [text("Deep-dive into every aspect of Abies — from core MVU concepts through virtual DOM internals, the binary batching protocol, E2E benchmarks, and production deployment.")]), | ||
| div([class_("card-meta")], | ||
| [ | ||
| span([class_("pill")], [text($"{FullSlides.Length} slides")]), | ||
| span([class_("pill")], [text("\u2248 60 min")]) | ||
| ]) | ||
| ]), | ||
| div([class_("presentation-card"), onclick(new Message.SelectPresentation(PresentationMode.Express))], |
There was a problem hiding this comment.
The menu cards are implemented as <div> with onclick(...), which are not keyboard-focusable by default and won’t be announced as interactive controls by screen readers. Prefer using a semantic <button>/<a> element, or add role="button" + tabindex="0" + key handlers to ensure keyboard accessibility.
| [ | ||
| div([class_("menu-header")], | ||
| [ | ||
| img([class_("menu-logo"), src("abies-logo.png"), width("80"), height("80")]), |
There was a problem hiding this comment.
Images are rendered without an alt attribute (e.g., the menu logo). For accessibility, provide meaningful alt text or alt="" if the image is purely decorative.
| img([class_("menu-logo"), src("abies-logo.png"), width("80"), height("80")]), | |
| img([class_("menu-logo"), src("abies-logo.png"), alt("Abies logo"), width("80"), height("80")]), |
| [ | ||
| div([class_("brand")], | ||
| [ | ||
| img([class_("brand-logo"), src("abies-logo.png"), alt("Abies logo")]), | ||
| img([class_("brand-logo"), src("abies-logo.png"), width("40"), height("40")]), |
There was a problem hiding this comment.
The topbar logo image is missing an alt attribute. Add alt text (or alt="" if decorative) so screen readers don’t announce it as an unlabeled image.
| img([class_("brand-logo"), src("abies-logo.png"), width("40"), height("40")]), | |
| img([class_("brand-logo"), src("abies-logo.png"), alt("Abies logo"), width("40"), height("40")]), |
| # Performance Benchmarks# Rendering Engine Benchmarks | ||
|
|
||
| This document describes the benchmarking infrastructure for the Abies rendering engine, including DOM diffing, HTML rendering, and event handler creation. | ||
|
|
||
| ## Overview | ||
|
|
||
| The Abies framework uses a Virtual DOM diffing algorithm to compute minimal patches between UI states. Performance of this algorithm is critical because it runs on every UI update. | ||
| This document describes the benchmarking infrastructure for the Abies framework, covering both micro-benchmarks (BenchmarkDotNet) and end-to-end benchmarks (js-framework-benchmark).This document describes the benchmarking infrastructure for the Abies rendering engine, including DOM diffing, HTML rendering, and event handler creation. | ||
|
|
||
| ## Benchmark Categories | ||
|
|
||
| The benchmark suite covers three categories: | ||
|
|
||
| ## Benchmarking Strategy## Overview | ||
|
|
||
|
|
||
|
|
||
| Abies uses a **dual-layer benchmarking strategy**:The Abies framework uses a Virtual DOM diffing algorithm to compute minimal patches between UI states. Performance of this algorithm is critical because it runs on every UI update. | ||
|
|
||
|
|
||
|
|
||
| | Layer | Tool | Purpose | Trust Level |## Benchmark Categories | ||
|
|
||
| | --- | --- | --- | --- | | ||
|
|
||
| | **Primary (E2E)** | js-framework-benchmark | Real-world user-perceived performance | Source of truth |The benchmark suite covers three categories: | ||
|
|
||
| | **Secondary (Micro)** | BenchmarkDotNet | Algorithm comparison, allocation tracking | Development guidance | |
There was a problem hiding this comment.
docs/benchmarks.md appears to have been corrupted: headings and paragraphs are concatenated (e.g., # Performance Benchmarks# Rendering Engine Benchmarks, ## Benchmarking Strategy## Overview) and tables/code fences are interleaved with text. This will render incorrectly in Markdown; please restore proper newlines/spacing and reformat tables/code blocks so the document is readable.
21e8b01 to
77e4a3b
Compare
Replace the abandoned type-safe HTML DSL approach with lightweight Roslyn analyzers that validate HTML correctness at compile time while keeping the existing stringly-typed DSL unchanged. Analyzers: - ABIES001: img() missing alt attribute (Warning) - ABIES002: Flow content inside phrasing-only parents (Warning) - ABIES003: a() missing href attribute (Info) - ABIES004: button() missing type attribute (Info) - ABIES005: input() missing type attribute (Info) Distribution: - Bundled in Abies NuGet package (analyzers/dotnet/cs/) - Automatic for all PackageReference consumers - Explicit ProjectReference needed for solution consumers Includes: - Abies.Analyzers project (netstandard2.0, Roslyn 4.8.0) - Abies.Analyzers.Tests project (17 tests) - ADR-021 documenting the decision Resolves #86
77e4a3b to
3f9f907
Compare
| ]), | ||
| div([class_("card-footer")], [ | ||
| img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? "")]), | ||
| img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? ""), alt($"{model.CurrentUser?.Username.Value ?? "User"} profile image")]), |
Check warning
Code scanning / CodeQL
Constant condition Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix a "constant condition" complaint arising from a prior type/flow check, you push the knowledge from the earlier check into later code by removing redundant null-propagation or conditional checks in the guarded branch. Here, CommentForm already checks model.CurrentUser is null and only executes the form(...) branch when it is non-null. Inside that branch, model.CurrentUser is guaranteed non-null, so using ?. on it is unnecessary and causes the static analyzer to flag a constant condition. The best minimal fix is to replace model.CurrentUser?.Image ?? "" with a direct access model.CurrentUser.Image and simplify the username expression while still preserving the "User" fallback only for the Username.Value part, not for the entire user object. That maintains the current display semantics while removing the redundant null-check on model.CurrentUser.
Concretely, in Abies.Conduit/Page/Article.cs, within CommentForm(Model model), update line 238. Replace:
img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? ""), alt($"{model.CurrentUser?.Username.Value ?? "User"} profile image")])
with:
img([class_("comment-author-img"), src(model.CurrentUser.Image), alt($"{model.CurrentUser.Username.Value ?? "User"} profile image")])
This uses the knowledge from the model.CurrentUser is null ? ... : ... ternary to directly access Image and Username while still allowing for Username.Value to be null and falling back to "User" in that case. No new methods or imports are required.
| @@ -235,7 +235,7 @@ | ||
| ], [text(model.CommentInput)]) | ||
| ]), | ||
| div([class_("card-footer")], [ | ||
| img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? ""), alt($"{model.CurrentUser?.Username.Value ?? "User"} profile image")]), | ||
| img([class_("comment-author-img"), src(model.CurrentUser.Image), alt($"{model.CurrentUser.Username.Value ?? "User"} profile image")]), | ||
| button([ | ||
| class_("btn btn-sm btn-primary"), | ||
| type("submit"), |
| ]), | ||
| div([class_("card-footer")], [ | ||
| img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? "")]), | ||
| img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? ""), alt($"{model.CurrentUser?.Username.Value ?? "User"} profile image")]), |
Check warning
Code scanning / CodeQL
Constant condition Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix a constant-condition/nullability style issue like this, remove redundant null-conditional operations or checks where control flow already guarantees non-nullness, or refactor the code so that the checks accurately reflect the possible states. This keeps the code clearer and avoids confusing both humans and static analyzers.
For this specific case in Abies.Conduit/Page/Article.cs, CommentForm uses a conditional expression: if model.CurrentUser is null, it returns a sign-in prompt; otherwise it returns the comment form. Therefore, in the form branch, model.CurrentUser is guaranteed not to be null. The attributes at line 238 and the surrounding expression currently use model.CurrentUser?.Image and model.CurrentUser?.Username.Value ?? "User", which implies that model.CurrentUser might still be null. The best fix is to remove the unnecessary null-conditional operator and the part of the null-coalescing that protects against a null CurrentUser, while still keeping any needed defaulting for inner nullable data (if any).
Concretely:
- On line 238, replace
model.CurrentUser?.Image ?? ""withmodel.CurrentUser.Image ?? ""(the innerImagemight still be null, so the?? ""is still reasonable). - Replace
$"{model.CurrentUser?.Username.Value ?? "User"} profile image"with$"{model.CurrentUser.Username.Value} profile image", becausemodel.CurrentUserandmodel.CurrentUser.Username/.Valueare assumed non-null in this branch; if a default display name is desired, it should be handled earlier or with a separate, explicit fallback, but the current ternary guarantees the presence of a user for this path.
No new methods or imports are needed; the changes are local to the CommentForm method in Abies.Conduit/Page/Article.cs.
| @@ -235,7 +235,7 @@ | ||
| ], [text(model.CommentInput)]) | ||
| ]), | ||
| div([class_("card-footer")], [ | ||
| img([class_("comment-author-img"), src(model.CurrentUser?.Image ?? ""), alt($"{model.CurrentUser?.Username.Value ?? "User"} profile image")]), | ||
| img([class_("comment-author-img"), src(model.CurrentUser.Image ?? ""), alt($"{model.CurrentUser.Username.Value} profile image")]), | ||
| button([ | ||
| class_("btn btn-sm btn-primary"), | ||
| type("submit"), |
| foreach (var expr in attributeExpressions) | ||
| { | ||
| var attrName = GetAttributeName(expr, semanticModel); | ||
| if (attrName != null) | ||
| { | ||
| builder.Add(attrName); | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Select Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
Generally, to fix this issue you replace a loop where each iteration only serves to transform the iteration variable into another value with a LINQ pipeline that performs the transformation explicitly via Select (and Where for filtering if needed). This clarifies that the loop’s primary purpose is to iterate over the transformed sequence.
In this specific method, we can replace the foreach loop in GetAttributeNames with a LINQ pipeline that:
- Starts from
attributeExpressions. - Projects each
exprtoGetAttributeName(expr, semanticModel)viaSelect. - Filters out
nullvalues viaWhere(attrName => attrName != null). - Casts to non-nullable string and adds each to the
ImmutableHashSet<string>.Builder.
There are two good options:
- Fully LINQ: build the set directly from the sequence (e.g.,
attributeExpressions.Select(...).OfType<string>().ToImmutableHashSet()). - Minimal change: keep the existing builder and just change the loop to iterate over the transformed sequence.
To minimize functional changes and avoid assumptions about builder semantics, we’ll keep the builder and only change the loop to:
foreach (var attrName in attributeExpressions
.Select(expr => GetAttributeName(expr, semanticModel))
.Where(attrName => attrName != null))
{
builder.Add(attrName);
}This preserves the single iteration over attributeExpressions and the same null filtering behavior as the original if (attrName != null).
No new methods or types are required, and no additional using directives are strictly necessary because extension methods like Select and Where are likely already available in the compilation; however, if not present elsewhere, the file would need using System.Linq;. Since we must not change imports unless necessary and haven’t been shown them beyond the snippet, we will not modify the imports here.
The only code change is within Abies.Analyzers/AnalysisHelpers.cs, inside GetAttributeNames, lines 76–83.
| @@ -73,15 +73,15 @@ | ||
| var firstArg = args[0].Expression; | ||
| var attributeExpressions = GetCollectionElements(firstArg); | ||
|
|
||
| foreach (var expr in attributeExpressions) | ||
| foreach (var attrName in attributeExpressions | ||
| .Select(expr => GetAttributeName(expr, semanticModel)) | ||
| .Where(attrName => attrName != null)) | ||
| { | ||
| var attrName = GetAttributeName(expr, semanticModel); | ||
| if (attrName != null) | ||
| { | ||
| builder.Add(attrName); | ||
| } | ||
| builder.Add(attrName!); | ||
| } | ||
|
|
||
|
|
||
|
|
||
| return builder.ToImmutable(); | ||
| } | ||
|
|
| foreach (var expr in childExpressions) | ||
| { | ||
| if (expr is InvocationExpressionSyntax childInvocation) | ||
| { | ||
| var childName = GetElementName(childInvocation, semanticModel); | ||
| if (childName != null) | ||
| { | ||
| builder.Add((childName, childInvocation.GetLocation())); | ||
| } | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix this type of issue you replace a foreach loop that conditionally processes elements based on a type or predicate with a foreach over a sequence that has already been filtered via .Where(...). This moves the filtering logic into the sequence definition, improving readability and reducing conditional nesting in the loop body.
Here, the best fix is to apply a LINQ Where filter to childExpressions so that the loop iterates only over InvocationExpressionSyntax items. We can then safely remove the type check if (expr is InvocationExpressionSyntax childInvocation) and assume within the loop that expr is an InvocationExpressionSyntax. To avoid changing behavior, we should keep the null check on childName and the subsequent builder.Add(...) exactly as they are. We only need to adjust the loop header and the way we declare childInvocation. This change is fully contained within GetChildElementNames in Abies.Analyzers/AnalysisHelpers.cs, and does not require any new imports because System.Linq is not yet used in the shown snippet—so we will add using System.Linq; at the top of the file.
Concretely:
- Add
using System.Linq;alongside the otherusingdirectives. - Change the
foreach (var expr in childExpressions)loop toforeach (var expr in childExpressions.Where(e => e is InvocationExpressionSyntax)). - Inside the loop, replace the pattern
if (expr is InvocationExpressionSyntax childInvocation)with a direct castvar childInvocation = (InvocationExpressionSyntax)expr;(or equivalent), leaving the rest of the logic unchanged.
| @@ -1,5 +1,6 @@ | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Linq; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| @@ -106,15 +107,13 @@ | ||
| var secondArg = args[1].Expression; | ||
| var childExpressions = GetCollectionElements(secondArg); | ||
|
|
||
| foreach (var expr in childExpressions) | ||
| foreach (var expr in childExpressions.Where(e => e is InvocationExpressionSyntax)) | ||
| { | ||
| if (expr is InvocationExpressionSyntax childInvocation) | ||
| var childInvocation = (InvocationExpressionSyntax)expr; | ||
| var childName = GetElementName(childInvocation, semanticModel); | ||
| if (childName != null) | ||
| { | ||
| var childName = GetElementName(childInvocation, semanticModel); | ||
| if (childName != null) | ||
| { | ||
| builder.Add((childName, childInvocation.GetLocation())); | ||
| } | ||
| builder.Add((childName, childInvocation.GetLocation())); | ||
| } | ||
| } | ||
|
|
| foreach (var req in requiredAttrs) | ||
| { | ||
| if (!presentAttrs.Contains(req.AttributeName) && | ||
| _descriptorMap.TryGetValue(req.DiagnosticId, out var descriptor)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, the fix is to replace the foreach loops that perform an implicit filter via an if (with continue or by wrapping the body) with foreach loops that iterate over requiredAttrs.Where(...) / recommendedAttrs.Where(...), moving the filtering predicate into the Where call. This expresses the intent explicitly and removes the inner conditional that only exists to skip elements.
For MissingAttributeAnalyzer.cs, the best minimal change is:
- Add
using System.Linq;so thatWhereis available as an extension method. - Change the loop
foreach (var req in requiredAttrs)intoforeach (var req in requiredAttrs.Where(...)), where the predicate is the conjunction of the two existing conditions: the attribute is not present, and its diagnostic descriptor exists in_descriptorMap. - Inside the loop, we can then assume the conditions already hold, so we only need to call
context.ReportDiagnostic(...)using the already-resolveddescriptor. That means we change the loop variable type to a tuple-like pattern(var req, var descriptor)by using LINQ’sSelectto carry the descriptor through, or we keep theTryGetValueinside the loop but remove the presence check. To minimize logic changes, we can keep_descriptorMap.TryGetValueinside theWherecall and project(req, descriptor)for use in the loop. - Apply the same transformation to the
foreach (var rec in recommendedAttrs)loop.
Concretely, within AnalyzeInvocation, replace:
- Lines 70–78 with a
foreachoverrequiredAttrs.Where(...)that produces(req, descriptor)and callsReportDiagnosticwithdescriptor. - Lines 86–94 similarly for
recommendedAttrs.
No new methods or types are required; only System.Linq as an import.
| @@ -3,6 +3,7 @@ | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using System.Linq; | ||
|
|
||
| namespace Abies.Analyzers; | ||
|
|
||
| @@ -67,14 +68,13 @@ | ||
| { | ||
| var presentAttrs = AnalysisHelpers.GetAttributeNames(invocation, context.SemanticModel); | ||
|
|
||
| foreach (var req in requiredAttrs) | ||
| foreach (var (req, descriptor) in requiredAttrs | ||
| .Where(req => !presentAttrs.Contains(req.AttributeName) && | ||
| _descriptorMap.TryGetValue(req.DiagnosticId, out _)) | ||
| .Select(req => (req, _descriptorMap[req.DiagnosticId]))) | ||
| { | ||
| if (!presentAttrs.Contains(req.AttributeName) && | ||
| _descriptorMap.TryGetValue(req.DiagnosticId, out var descriptor)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); | ||
| } | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); | ||
| } | ||
| } | ||
|
|
||
| @@ -83,14 +82,13 @@ | ||
| { | ||
| var presentAttrs = AnalysisHelpers.GetAttributeNames(invocation, context.SemanticModel); | ||
|
|
||
| foreach (var rec in recommendedAttrs) | ||
| foreach (var (rec, descriptor) in recommendedAttrs | ||
| .Where(rec => !presentAttrs.Contains(rec.AttributeName) && | ||
| _descriptorMap.TryGetValue(rec.DiagnosticId, out _)) | ||
| .Select(rec => (rec, _descriptorMap[rec.DiagnosticId]))) | ||
| { | ||
| if (!presentAttrs.Contains(rec.AttributeName) && | ||
| _descriptorMap.TryGetValue(rec.DiagnosticId, out var descriptor)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); | ||
| } | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); | ||
| } | ||
| } | ||
| } |
| foreach (var rec in recommendedAttrs) | ||
| { | ||
| if (!presentAttrs.Contains(rec.AttributeName) && | ||
| _descriptorMap.TryGetValue(rec.DiagnosticId, out var descriptor)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix this pattern you move the filtering logic out of the loop body into a LINQ .Where(...) call on the source sequence, so the foreach iterates only those elements that pass the predicate. This removes the initial if/continue pattern and makes the code’s intent explicit.
Here, we should update the foreach (var rec in recommendedAttrs) loop so it iterates only over those rec for which presentAttrs does not already contain rec.AttributeName. The _descriptorMap.TryGetValue part is not a pure filter on recommendedAttrs—it’s a lookup into another dictionary—so it should remain inside the loop body. Concretely, in Abies.Analyzers/MissingAttributeAnalyzer.cs, within the AnalyzeInvocation method, replace:
foreach (var rec in recommendedAttrs)
{
if (!presentAttrs.Contains(rec.AttributeName) &&
_descriptorMap.TryGetValue(rec.DiagnosticId, out var descriptor))
{
context.ReportDiagnostic(
Diagnostic.Create(descriptor, invocation.GetLocation()));
}
}with:
foreach (var rec in recommendedAttrs.Where(rec => !presentAttrs.Contains(rec.AttributeName)))
{
if (_descriptorMap.TryGetValue(rec.DiagnosticId, out var descriptor))
{
context.ReportDiagnostic(
Diagnostic.Create(descriptor, invocation.GetLocation()));
}
}This preserves behavior: we still report only when the attribute is missing and the diagnostic descriptor exists, but the loop now explicitly filters on the missing-attribute condition. No new methods or imports are needed; System.Linq must already be available for other LINQ uses, and if not, it is a standard BCL namespace we can safely import.
| @@ -83,10 +83,9 @@ | ||
| { | ||
| var presentAttrs = AnalysisHelpers.GetAttributeNames(invocation, context.SemanticModel); | ||
|
|
||
| foreach (var rec in recommendedAttrs) | ||
| foreach (var rec in recommendedAttrs.Where(rec => !presentAttrs.Contains(rec.AttributeName))) | ||
| { | ||
| if (!presentAttrs.Contains(rec.AttributeName) && | ||
| _descriptorMap.TryGetValue(rec.DiagnosticId, out var descriptor)) | ||
| if (_descriptorMap.TryGetValue(rec.DiagnosticId, out var descriptor)) | ||
| { | ||
| context.ReportDiagnostic( | ||
| Diagnostic.Create(descriptor, invocation.GetLocation())); |
Replace the abandoned type-safe HTML DSL approach with lightweight Roslyn analyzers that validate HTML correctness at compile time while keeping the existing stringly-typed DSL unchanged. Analyzers: - ABIES001: img() missing alt attribute (Warning) - ABIES002: Flow content inside phrasing-only parents (Warning) - ABIES003: a() missing href attribute (Info) - ABIES004: button() missing type attribute (Info) - ABIES005: input() missing type attribute (Info) Distribution: - Bundled in Abies NuGet package (analyzers/dotnet/cs/) - Automatic for all PackageReference consumers - Explicit ProjectReference needed for solution consumers Includes: - Abies.Analyzers project (netstandard2.0, Roslyn 4.8.0) - Abies.Analyzers.Tests project (17 tests) - ADR-021 documenting the decision Resolves #86
📝 Description
What
Add Roslyn analyzers that validate HTML correctness at compile time for the Abies HTML DSL.
Why
The Abies HTML DSL is stringly typed — all elements return
Node, all attributes returnDOM.Attribute, and all values arestring. This means the type system allows illegal HTML states that compile but produce invalid markup (e.g.,img()withoutalt,divnested insidespan).A full type-safe HTML DSL prototype was built (~9,300 lines, 90+ files) but was rejected due to massive API surface explosion, broken composability, and high migration cost. Roslyn analyzers achieve the same compile-time validation with zero breaking changes. See ADR-021 for the full decision record.
How
Two analyzer classes inspect
InvocationExpressionSyntaxnodes via the Roslyn semantic model:The analyzer DLL is bundled into the Abies NuGet package at
analyzers/dotnet/cs/, so all consumers get validation automatically.🔗 Related Issues
Resolves #86
✅ Type of Change
🧪 Testing
Test Coverage
Testing Details
dotnet packproduces correctanalyzers/dotnet/cs/layout)✨ Changes Made
Abies.Analyzersproject (netstandard2.0, Roslyn 4.8.0) with 5 diagnostic rules:img()missingalt()attributedivinspan)a()missinghref()attributebutton()missingtype()attributeinput()missingtype()attributeAbies.Analyzers.Testsproject with 17 testsAbies.csprojfor NuGet packaging (analyzers/dotnet/cs/convention)ProjectReferencetoAbies.Conduit.csproj(needed for solution-level consumers)Abies.slnaltattributes to profile/avatar images inProfile.cs,Article.cs,Home.cs)🔍 Code Review Checklist
🚀 Deployment Notes
None — analyzers are compile-time only and have no runtime impact.
📋 Additional Context
MSBuild Discovery
OutputItemType="Analyzer"does not propagate transitively throughProjectReferencechains. This means solution-level consumers (likeAbies.Conduit) need an explicitProjectReferencetoAbies.Analyzers, while NuGet package consumers get the analyzer automatically via theanalyzers/dotnet/cs/convention.Architecture