fix(localize): render context of translation file parse errors (#38673)

Previously the position of the error in a translation file when parsing it was not displayed. Just the error message. Now the position (line and column) and some context is displayed along with the error messages. Fixes #38377 PR Close #38673
angular · Sep 2, 2020 · 32f33f0 · 32f33f0
1 parent b0bd777
commit 32f33f0
Show file tree

Hide file tree

Showing 10 changed files with 140 additions and 115 deletions.
diff --git a/...ize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts b/...ize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts
@@ -13,7 +13,7 @@ import {getAttribute, getAttrOrThrow} from '../translation_parsers/translation_u
 
 import {MessageRenderer} from './message_renderer';
 
-interface MessageSerializerConfig {
+export interface MessageSerializerConfig {
   inlineElements: string[];
   placeholder?: {elementName: string; nameAttribute: string; bodyAttribute?: string;};
   placeholderContainer?: {elementName: string; startAttribute: string; endAttribute: string;};

diff --git a/...ools/src/translate/translation_files/translation_parsers/serialize_translation_message.ts b/...ools/src/translate/translation_files/translation_parsers/serialize_translation_message.ts
@@ -0,0 +1,32 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+import {Element, ParseError} from '@angular/compiler';
+import {ɵParsedTranslation} from '@angular/localize';
+
+import {MessageSerializer, MessageSerializerConfig} from '../message_serialization/message_serializer';
+import {TargetMessageRenderer} from '../message_serialization/target_message_renderer';
+
+import {parseInnerRange} from './translation_utils';
+
+/**
+ * Serialize the given `element` into a parsed translation using the given `serializer`.
+ */
+export function serializeTranslationMessage(element: Element, config: MessageSerializerConfig): {
+  translation: ɵParsedTranslation|null,
+  parseErrors: ParseError[],
+  serializeErrors: ParseError[]
+} {
+  const {rootNodes, errors: parseErrors} = parseInnerRange(element);
+  try {
+    const serializer = new MessageSerializer(new TargetMessageRenderer(), config);
+    const translation = serializer.serialize(rootNodes);
+    return {translation, parseErrors, serializeErrors: []};
+  } catch (e) {
+    return {translation: null, parseErrors, serializeErrors: [e]};
+  }
+}
diff --git a/.../src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts b/.../src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts
@@ -14,17 +14,15 @@ export class TranslationParseError extends Error {
   constructor(
       public span: ParseSourceSpan, public msg: string,
       public level: ParseErrorLevel = ParseErrorLevel.ERROR) {
-    super(msg);
-  }
-
-  contextualMessage(): string {
-    const ctx = this.span.start.getContext(100, 3);
-    return ctx ? `${this.msg} ("${ctx.before}[${ParseErrorLevel[this.level]} ->]${ctx.after}")` :
-                 this.msg;
+    super(contextualMessage(span, msg, level));
   }
+}
 
-  toString(): string {
-    const details = this.span.details ? `, ${this.span.details}` : '';
-    return `${this.contextualMessage()}: ${this.span.start}${details}`;
+function contextualMessage(span: ParseSourceSpan, msg: string, level: ParseErrorLevel): string {
+  const ctx = span.start.getContext(100, 2);
+  msg += `\nAt ${span.start}${span.details ? `, ${span.details}` : ''}:\n`;
+  if (ctx) {
+    msg += `...${ctx.before}[${ParseErrorLevel[level]} ->]${ctx.after}...\n`;
   }
+  return msg;
 }
diff --git a/...calize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts b/...calize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts
@@ -5,10 +5,12 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {Element, LexerRange, Node, ParseError, ParseErrorLevel, ParseSourceSpan, XmlParser} from '@angular/compiler';
+import {Element, LexerRange, Node, ParseError, ParseErrorLevel, ParseSourceSpan, ParseTreeResult, XmlParser} from '@angular/compiler';
+
 import {Diagnostics} from '../../../diagnostics';
+
 import {TranslationParseError} from './translation_parse_error';
-import {ParseAnalysis} from './translation_parser';
+import {ParseAnalysis, ParsedTranslationBundle} from './translation_parser';
 
 export function getAttrOrThrow(element: Element, attrName: string): string {
   const attrValue = getAttribute(element, attrName);
@@ -30,17 +32,15 @@ export function getAttribute(element: Element, attrName: string): string|undefin
  * This would be equivalent to parsing the `innerHTML` string of an HTML document.
  *
  * @param element The element whose inner range we want to parse.
- * @returns a collection of XML `Node` objects that were parsed from the element's contents.
+ * @returns a collection of XML `Node` objects and any errors that were parsed from the element's
+ *     contents.
  */
-export function parseInnerRange(element: Element): Node[] {
+export function parseInnerRange(element: Element): ParseTreeResult {
   const xmlParser = new XmlParser();
   const xml = xmlParser.parse(
       element.sourceSpan.start.file.content, element.sourceSpan.start.file.url,
       {tokenizeExpansionForms: true, range: getInnerRange(element)});
-  if (xml.errors.length) {
-    throw xml.errors.map(e => new TranslationParseError(e.span, e.msg).toString()).join('\n');
-  }
-  return xml.rootNodes;
+  return xml;
 }
 
 /**
@@ -155,3 +155,12 @@ export function addParseError(diagnostics: Diagnostics, parseError: ParseError):
     diagnostics.warn(parseError.toString());
   }
 }
+
+/**
+ * Add the provided `errors` to the `bundle` diagnostics.
+ */
+export function addErrorsToBundle(bundle: ParsedTranslationBundle, errors: ParseError[]): void {
+  for (const error of errors) {
+    addParseError(bundle.diagnostics, error);
+  }
+}
diff --git a/...rc/tools/src/translate/translation_files/translation_parsers/xliff1_translation_parser.ts b/...rc/tools/src/translate/translation_files/translation_parsers/xliff1_translation_parser.ts
@@ -6,15 +6,13 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {Element, ParseErrorLevel, visitAll} from '@angular/compiler';
-import {ɵParsedTranslation} from '@angular/localize';
 
 import {Diagnostics} from '../../../diagnostics';
 import {BaseVisitor} from '../base_visitor';
-import {MessageSerializer} from '../message_serialization/message_serializer';
-import {TargetMessageRenderer} from '../message_serialization/target_message_renderer';
 
+import {serializeTranslationMessage} from './serialize_translation_message';
 import {ParseAnalysis, ParsedTranslationBundle, TranslationParser} from './translation_parser';
-import {addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, parseInnerRange, XmlTranslationParserHint} from './translation_utils';
+import {addErrorsToBundle, addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, XmlTranslationParserHint} from './translation_utils';
 
 /**
  * A translation parser that can load XLIFF 1.2 files.
@@ -150,23 +148,14 @@ class XliffTranslationVisitor extends BaseVisitor {
       return;
     }
 
-    try {
-      bundle.translations[id] = serializeTargetMessage(targetMessage);
-    } catch (e) {
-      // Capture any errors from serialize the target message
-      if (e.span && e.msg && e.level) {
-        addParseDiagnostic(bundle.diagnostics, e.span, e.msg, e.level);
-      } else {
-        throw e;
-      }
+    const {translation, parseErrors, serializeErrors} = serializeTranslationMessage(targetMessage, {
+      inlineElements: ['g', 'bx', 'ex', 'bpt', 'ept', 'ph', 'it', 'mrk'],
+      placeholder: {elementName: 'x', nameAttribute: 'id'}
+    });
+    if (translation !== null) {
+      bundle.translations[id] = translation;
     }
+    addErrorsToBundle(bundle, parseErrors);
+    addErrorsToBundle(bundle, serializeErrors);
   }
 }
-
-function serializeTargetMessage(source: Element): ɵParsedTranslation {
-  const serializer = new MessageSerializer(new TargetMessageRenderer(), {
-    inlineElements: ['g', 'bx', 'ex', 'bpt', 'ept', 'ph', 'it', 'mrk'],
-    placeholder: {elementName: 'x', nameAttribute: 'id'}
-  });
-  return serializer.serialize(parseInnerRange(source));
-}
diff --git a/...rc/tools/src/translate/translation_files/translation_parsers/xliff2_translation_parser.ts b/...rc/tools/src/translate/translation_files/translation_parsers/xliff2_translation_parser.ts
@@ -6,15 +6,13 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {Element, Node, ParseErrorLevel, visitAll} from '@angular/compiler';
-import {ɵParsedTranslation} from '@angular/localize';
 
 import {Diagnostics} from '../../../diagnostics';
 import {BaseVisitor} from '../base_visitor';
-import {MessageSerializer} from '../message_serialization/message_serializer';
-import {TargetMessageRenderer} from '../message_serialization/target_message_renderer';
 
+import {serializeTranslationMessage} from './serialize_translation_message';
 import {ParseAnalysis, ParsedTranslationBundle, TranslationParser} from './translation_parser';
-import {addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, parseInnerRange, XmlTranslationParserHint} from './translation_utils';
+import {addErrorsToBundle, addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, XmlTranslationParserHint} from './translation_utils';
 
 /**
  * A translation parser that can load translations from XLIFF 2 files.
@@ -141,29 +139,20 @@ class Xliff2TranslationVisitor extends BaseVisitor {
       return;
     }
 
-    try {
-      bundle.translations[unit] = serializeTargetMessage(targetMessage);
-    } catch (e) {
-      // Capture any errors from serialize the target message
-      if (e.span && e.msg && e.level) {
-        addParseDiagnostic(bundle.diagnostics, e.span, e.msg, e.level);
-      } else {
-        throw e;
-      }
+    const {translation, parseErrors, serializeErrors} = serializeTranslationMessage(targetMessage, {
+      inlineElements: ['cp', 'sc', 'ec', 'mrk', 'sm', 'em'],
+      placeholder: {elementName: 'ph', nameAttribute: 'equiv', bodyAttribute: 'disp'},
+      placeholderContainer:
+          {elementName: 'pc', startAttribute: 'equivStart', endAttribute: 'equivEnd'}
+    });
+    if (translation !== null) {
+      bundle.translations[unit] = translation;
     }
+    addErrorsToBundle(bundle, parseErrors);
+    addErrorsToBundle(bundle, serializeErrors);
   }
 }
 
-function serializeTargetMessage(source: Element): ɵParsedTranslation {
-  const serializer = new MessageSerializer(new TargetMessageRenderer(), {
-    inlineElements: ['cp', 'sc', 'ec', 'mrk', 'sm', 'em'],
-    placeholder: {elementName: 'ph', nameAttribute: 'equiv', bodyAttribute: 'disp'},
-    placeholderContainer:
-        {elementName: 'pc', startAttribute: 'equivStart', endAttribute: 'equivEnd'}
-  });
-  return serializer.serialize(parseInnerRange(source));
-}
-
 function isFileElement(node: Node): node is Element {
   return node instanceof Element && node.name === 'file';
 }
diff --git a/...e/src/tools/src/translate/translation_files/translation_parsers/xtb_translation_parser.ts b/...e/src/tools/src/translate/translation_files/translation_parsers/xtb_translation_parser.ts
@@ -5,17 +5,15 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import {Element, ParseErrorLevel, visitAll} from '@angular/compiler';
-import {ɵParsedTranslation} from '@angular/localize';
+import {Element, ParseError, ParseErrorLevel, visitAll} from '@angular/compiler';
 import {extname} from 'path';
 
 import {Diagnostics} from '../../../diagnostics';
 import {BaseVisitor} from '../base_visitor';
-import {MessageSerializer} from '../message_serialization/message_serializer';
-import {TargetMessageRenderer} from '../message_serialization/target_message_renderer';
 
+import {serializeTranslationMessage} from './serialize_translation_message';
 import {ParseAnalysis, ParsedTranslationBundle, TranslationParser} from './translation_parser';
-import {addParseDiagnostic, addParseError, canParseXml, getAttribute, parseInnerRange, XmlTranslationParserHint} from './translation_utils';
+import {addErrorsToBundle, addParseDiagnostic, addParseError, canParseXml, getAttribute, XmlTranslationParserHint} from './translation_utils';
 
 
 /**
@@ -103,20 +101,17 @@ class XtbVisitor extends BaseVisitor {
           return;
         }
 
-        try {
-          bundle.translations[id] = serializeTargetMessage(element);
-        } catch (error) {
-          if (typeof error === 'string') {
-            bundle.diagnostics.warn(
-                `Could not parse message with id "${
-                    id}" - perhaps it has an unrecognised ICU format?\n` +
-                error);
-          } else if (error.span && error.msg && error.level) {
-            addParseDiagnostic(bundle.diagnostics, error.span, error.msg, error.level);
-          } else {
-            throw error;
-          }
+        const {translation, parseErrors, serializeErrors} = serializeTranslationMessage(
+            element, {inlineElements: [], placeholder: {elementName: 'ph', nameAttribute: 'name'}});
+        if (parseErrors.length) {
+          // We only want to warn (not error) if there were problems parsing the translation for
+          // XTB formatted files. See https://github.com/angular/angular/issues/14046.
+          bundle.diagnostics.warn(computeParseWarning(id, parseErrors));
+        } else if (translation !== null) {
+          // Only store the translation if there were no parse errors
+          bundle.translations[id] = translation;
         }
+        addErrorsToBundle(bundle, serializeErrors);
         break;
 
       default:
@@ -127,9 +122,8 @@ class XtbVisitor extends BaseVisitor {
   }
 }
 
-function serializeTargetMessage(source: Element): ɵParsedTranslation {
-  const serializer = new MessageSerializer(
-      new TargetMessageRenderer(),
-      {inlineElements: [], placeholder: {elementName: 'ph', nameAttribute: 'name'}});
-  return serializer.serialize(parseInnerRange(source));
+function computeParseWarning(id: string, errors: ParseError[]): string {
+  const msg = errors.map(e => e.toString()).join('\n');
+  return `Could not parse message with id "${id}" - perhaps it has an unrecognised ICU format?\n` +
+      msg;
 }
diff --git a/...ls/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts b/...ls/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts
@@ -621,7 +621,7 @@ describe('Xliff1TranslationParser', () => {
       });
 
       describe('[structure errors]', () => {
-        it('should throw when a trans-unit has no translation', () => {
+        it('should fail when a trans-unit has no translation', () => {
           const XLIFF = [
             `<?xml version="1.0" encoding="UTF-8" ?>`,
             `<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">`,
@@ -646,7 +646,7 @@ describe('Xliff1TranslationParser', () => {
         });
 
 
-        it('should throw when a trans-unit has no id attribute', () => {
+        it('should fail when a trans-unit has no id attribute', () => {
           const XLIFF = [
             `<?xml version="1.0" encoding="UTF-8" ?>`,
             `<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">`,
@@ -671,7 +671,7 @@ describe('Xliff1TranslationParser', () => {
           ].join('\n'));
         });
 
-        it('should throw on duplicate trans-unit id', () => {
+        it('should fail on duplicate trans-unit id', () => {
           const XLIFF = [
             `<?xml version="1.0" encoding="UTF-8" ?>`,
             `<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">`,
@@ -703,7 +703,7 @@ describe('Xliff1TranslationParser', () => {
       });
 
       describe('[message errors]', () => {
-        it('should throw on unknown message tags', () => {
+        it('should fail on unknown message tags', () => {
           const XLIFF = [
             `<?xml version="1.0" encoding="UTF-8" ?>`,
             `<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">`,
@@ -719,17 +719,18 @@ describe('Xliff1TranslationParser', () => {
           ].join('\n');
 
           expectToFail('/some/file.xlf', XLIFF, /Invalid element found in message/, [
-            `Invalid element found in message. ("`,
-            `      <trans-unit id="deadbeef" datatype="html">`,
+            `Error: Invalid element found in message.`,
+            `At /some/file.xlf@6:16:`,
+            `...`,
             `        <source/>`,
             `        <target>[ERROR ->]<b>msg should contain only ph tags</b></target>`,
             `      </trans-unit>`,
-            `    </body>`,
-            `"): /some/file.xlf@6:16`,
+            `...`,
+            ``,
           ].join('\n'));
         });
 
-        it('should throw when a placeholder misses an id attribute', () => {
+        it('should fail when a placeholder misses an id attribute', () => {
           const XLIFF = [
             `<?xml version="1.0" encoding="UTF-8" ?>`,
             `<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">`,
@@ -745,13 +746,14 @@ describe('Xliff1TranslationParser', () => {
           ].join('\n');
 
           expectToFail('/some/file.xlf', XLIFF, /required "id" attribute/gi, [
-            `Missing required "id" attribute: ("`,
-            `      <trans-unit id="deadbeef" datatype="html">`,
+            `Error: Missing required "id" attribute:`,
+            `At /some/file.xlf@6:16:`,
+            `...`,
             `        <source/>`,
             `        <target>[ERROR ->]<x/></target>`,
             `      </trans-unit>`,
-            `    </body>`,
-            `"): /some/file.xlf@6:16`,
+            `...`,
+            ``,
           ].join('\n'));
         });
       });