Skip to content

feat(PC0039): report malformed translation files - #589

Merged
Arthurvdv merged 4 commits into
ALCops:mainfrom
tkroes:feat/lc0100-translation-files-must-be-well-formed
Oct 27, 2026
Merged

Arthurvdv merged 4 commits into
ALCops:mainfrom
tkroes:feat/lc0100-translation-files-must-be-well-formed

Conversation

@tkroes

@tkroes tkroes commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds LinterCop rule PC0039 TranslationFilesMustBeWellFormed to report malformed or unreadable translation files for the current app.

  • Checks XML well-formedness for compiler-discovered XLIFF files (no XLIFF schema/semantic validation)
  • Reports every malformed/unreadable file, with its path and details at Location.None; XML parse errors include line and column
  • Remains silent when translation generation is disabled or the translation directory is absent
  • Category Style, severity Error, enabled by default; no CodeFix and no setting
  • Supports all three installed SDK target assemblies (netstandard2.1, net8.0, net10.0); no version gate

Validation

  • Targeted tests: 4 passed
  • Full LinterCop suite: 400 passed
  • Release CI build across all target frameworks: succeeded
  • dotnet format ALCops.sln --verify-no-changes: clean
  • .claude rule validation: passed
  • /code-review: no significant issues found

Docs-site page in the sibling alcops.dev repo remains to be written.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Arthurvdv

Copy link
Copy Markdown
Member

Thanks for this one, @tkroes. A malformed .xlf is a nasty thing to track down, so a rule for it is more then welcome 🤗

A few points I would like to discuss with you before we merge.

PlatformCop vs LinterCop

I tend to place rules by what happens if they're ignored (see how the analyzers differ):

  • LinterCop covers code that is correct but could be shorter, clearer or faster. Ignoring a LinterCop rule breaks nothing; the cost falls on whoever maintains the code next.
  • PlatformCop covers constructs the platform rejects, ignores, or runs differently from what the code says. The tie-breaker is: "If ignoring the rule produces a runtime error or a silently wrong result, it is a PlatformCop rule."

The rule doc justifies with "failed translation loading affects the whole extension". That is the PlatformCop consequence: the platform doesn't apply the translations and users see the base-language captions. It doesn't depend on any Base Application convention, so it isn't ApplicationCop either. It also isn't about procedure bodies, which is what LinterCop inspects.

Could you move the rule to PlatformCop with the next free PC ID? LC0100 hasn't been released, so nothing is lost by leaving it unused. And while moving it, don't forget to change the category from Style to something like Usage: a broken translation file isn't a style issue.

Severity: Warning rather than Error

The rule is on by default at Error, so anyone who upgrades ALCops with a malformed .xlf in their project gets a new build error straight away. That can break CI/CD pipelines that update the analyzers automatically. Could you ship it as a Warning? Teams that want it to block the build can raise it to Error in their ruleset.

Code Review

Some remarks on the implementaion of the code itself

1. Stream the file instead of building a DOM. XmlDocument.Load builds the whole document in memory just to check well-formedness. Translation files can be several MB per language, with 20+ languages. It also keeps the default DTD handling, so a DOCTYPE with entity declarations gets expanded. An XmlReader read to the end does the same check at streaming cost, and lets us turn DTDs off:

private static readonly XmlReaderSettings WellFormedOnly = new()
{
    DtdProcessing = DtdProcessing.Prohibit,
    XmlResolver = null,
    IgnoreComments = true,
    IgnoreWhitespace = true,
};

using Stream stream = fileSystem.OpenRead(translationFile);
using var reader = XmlReader.Create(stream, WellFormedOnly);
while (reader.Read())
{
}

XmlException keeps its LineNumber/LinePosition, so the message stays the same.

2. Discovery only catches DirectoryNotFoundException. Each file read handles IOException and UnauthorizedAccessException, but listing the files doesn't. An unreadable Translations folder or a path-too-long error then escapes the compilation action, and the user gets an analyzer crash instead of a translation-file diagnostic. For example:

catch (DirectoryNotFoundException)
{
    return;
}
catch (Exception exception) when (exception is IOException or UnauthorizedAccessException)
{
    ReportFileError(ctx, "Translations", $"Unable to enumerate translation files: {exception.Message}");
    return;
}

... or return silently if you'd rather not report this case. Either way it shouldn't crash.

3. Missing manifest: this rule and LC0091 behave differently. When the app's manifest (app.json) can't be resolved, this rule returns, while TranslatableTextShouldBeTranslated carries on: it only skips when a manifest is present and turns translation-file generation off and otherwise uses an empty app name. What do you think is the best choice here? Should this rule follow the same pattern, so both translation rules agree on when they run?

NavAppManifest? manifest;
try
{
    manifest = ManifestHelper.GetManifest(ctx.Compilation);
}
catch (FileNotFoundException)
{
    manifest = null;
}

if (manifest is not null && !manifest.CompilerFeatures.ShouldGenerateTranslationFile())
    return;

string appName = manifest?.AppName ?? string.Empty;

Tests

I see some errors in src/ALCops.LinterCop.Test/Rules/TranslationFilesMustBeWellFormed/TranslationFilesMustBeWellFormed.cs. Could you bring in #592 (88f251a) from main? That will show whether they're still errors or just came from the test-build issue #592 fixed.

Arthurvdv and others added 2 commits October 26, 2026 15:03
…o 1.5.1 (ALCops#592)

* fix(ci): fail the test job when a test project does not compile

Every test step in build-test.yml was continue-on-error, so a compile
error in a test project (CS1705 from RoslynTestKit referencing a newer
SDK than the leg's floor) produced no trx and was silently skipped; the
daily build stayed green for weeks.

Each test project is now built in its own step without continue-on-error,
so a compile error fails the job at that step. `dotnet test --no-build`
(same NavTargetFramework and Configuration, so it runs the exact build
output) stays continue-on-error, so test failures still do not stop the
remaining cops and AL versions from reporting together.

The rules docs now describe the gate and the RoslynTestKit floor
contract: the per-TFM floor moves down when Microsoft publishes a
net10.0 build for an older AL major, and the remedy for CS1705 is
re-pinning and releasing RoslynTestKit.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* chore: bump RoslynTestKit to 1.5.1 and pin the local net10.0 SDK to the 16.10 floor

RoslynTestKit 1.5.1 compiles its net10.0 lib against SDK 16.10.44.19458,
the lowest marketplace net10.0 release, so it binds on every leg at or
above the CI compile floor.

Setup-BCDevTools.ps1 now pins net10.0 to the same 16.10 VSIX so the
local compile floor equals the CI floor.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* chore(ci): name the build steps in the no-trx error and keep the floor doc to current state

The no-trx error in Merge Test Results now points at the Build steps as
well as the Setup and Run steps. The floor row in get-bc-devtools.md
drops its dated history, states the mechanism and current values, and
explains why RoslynTestKit must be compiled against an SDK at or below
the floor (CS1705).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tkroes

tkroes commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

@Arthurvdv

  • The rule has been moved from LC0100 to PC0039
  • The diagnostic is now a warning instead of an error by default
  • The file is now streamed rather than building a DOM
  • File discovery now also correctly handles all of the exceptions that file reading does
  • This rule now behaves the same as LC0091 when a manifest is missing
  • fix(ci): fail the test job on compile errors and bump RoslynTestKit to 1.5.1 #592 has been brought in from main

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Arthurvdv Arthurvdv changed the title feat(LC0100): report malformed translation files feat(PC0039): report malformed translation files Oct 27, 2026
@Arthurvdv

Copy link
Copy Markdown
Member

Look great, thank you! Let's get this merged! 🎉

@Arthurvdv
Arthurvdv merged commit ae0bf28 into ALCops:main Oct 27, 2026
52 checks passed
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.

2 participants