fix(compiler): return full spans for Comment nodes (#50855)

P4 · alxhub · commit cc722ea1f5b1 · 2023-07-28T14:39:18.000-07:00
Change sourceSpan for Comment nodes to cover the whole comment instead of just the opening token. The primary motivation for this is the interaction between ESLint and `@angular-eslint`. ESLint can detect unused `eslint-disable` directives in comments and automatically remove them when running with `--fix`. This is based on ranges computed from AST spans, and as a result does not work inside Angular templates - right now all comments claim to be 4 characters long so only the opening `<!--` is removed. PR Close #50855
diff --git a/packages/compiler/src/ml_parser/parser.ts b/packages/compiler/src/ml_parser/parser.ts
@@ -107,9 +107,13 @@ class _TreeBuilder {
 
   private _consumeComment(token: CommentStartToken) {
     const text = this._advanceIf(TokenType.RAW_TEXT);
-    this._advanceIf(TokenType.COMMENT_END);
+    const endToken = this._advanceIf(TokenType.COMMENT_END);
     const value = text != null ? text.parts[0].trim() : null;
-    this._addToParent(new html.Comment(value, token.sourceSpan));
+    const sourceSpan = endToken == null ?
+        token.sourceSpan :
+        new ParseSourceSpan(
+            token.sourceSpan.start, endToken.sourceSpan.end, token.sourceSpan.fullStart);
+    this._addToParent(new html.Comment(value, sourceSpan));
   }
 
   private _consumeExpansion(token: ExpansionFormStartToken) {
diff --git a/packages/compiler/test/i18n/extractor_merger_spec.ts b/packages/compiler/test/i18n/extractor_merger_spec.ts
@@ -351,40 +351,40 @@ import {serializeNodes as serializeHtmlNodes} from '../ml_parser/util/util';
       describe('blocks', () => {
         it('should report nested blocks', () => {
           expect(extractErrors(`<!-- i18n --><!-- i18n --><!-- /i18n --><!-- /i18n -->`)).toEqual([
-            ['Could not start a block inside a translatable section', '<!--'],
-            ['Trying to close an unopened block', '<!--'],
+            ['Could not start a block inside a translatable section', '<!-- i18n -->'],
+            ['Trying to close an unopened block', '<!-- /i18n -->'],
           ]);
         });
 
         it('should report unclosed blocks', () => {
           expect(extractErrors(`<!-- i18n -->`)).toEqual([
-            ['Unclosed block', '<!--'],
+            ['Unclosed block', '<!-- i18n -->'],
           ]);
         });
 
         it('should report translatable blocks in translatable elements', () => {
           expect(extractErrors(`<p i18n><!-- i18n --><!-- /i18n --></p>`)).toEqual([
-            ['Could not start a block inside a translatable section', '<!--'],
-            ['Trying to close an unopened block', '<!--'],
+            ['Could not start a block inside a translatable section', '<!-- i18n -->'],
+            ['Trying to close an unopened block', '<!-- /i18n -->'],
           ]);
         });
 
         it('should report translatable blocks in implicit elements', () => {
           expect(extractErrors(`<p><!-- i18n --><!-- /i18n --></p>`, ['p'])).toEqual([
-            ['Could not start a block inside a translatable section', '<!--'],
-            ['Trying to close an unopened block', '<!--'],
+            ['Could not start a block inside a translatable section', '<!-- i18n -->'],
+            ['Trying to close an unopened block', '<!-- /i18n -->'],
           ]);
         });
 
         it('should report when start and end of a block are not at the same level', () => {
           expect(extractErrors(`<!-- i18n --><p><!-- /i18n --></p>`)).toEqual([
-            ['I18N blocks should not cross element boundaries', '<!--'],
+            ['I18N blocks should not cross element boundaries', '<!-- /i18n -->'],
             ['Unclosed block', '<p><!-- /i18n --></p>'],
           ]);
 
           expect(extractErrors(`<p><!-- i18n --></p><!-- /i18n -->`)).toEqual([
-            ['I18N blocks should not cross element boundaries', '<!--'],
-            ['Unclosed block', '<!--'],
+            ['I18N blocks should not cross element boundaries', '<!-- /i18n -->'],
+            ['Unclosed block', '<!-- /i18n -->'],
           ]);
         });
       });
diff --git a/packages/compiler/test/ml_parser/html_parser_spec.ts b/packages/compiler/test/ml_parser/html_parser_spec.ts
@@ -251,7 +251,7 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn, humanizeNodes}
             ],
             [html.Element, 'span', 1, '<span>x {{ expr }<!---->} y</span>', '<span>', '</span>'],
             [html.Text, 'x {{ expr }', 2, ['x '], ['{{', ' expr }'], [''], 'x {{ expr }'],
-            [html.Comment, '', 2, '<!--'],
+            [html.Comment, '', 2, '<!---->'],
             [html.Text, '} y', 2, ['} y'], '} y'],
             [html.Element, 'div', 1, '<div></div>', '<div>', '</div>'],
           ]);
diff --git a/packages/compiler/test/render3/view/parse_template_options_spec.ts b/packages/compiler/test/render3/view/parse_template_options_spec.ts
@@ -39,9 +39,13 @@ describe('collectCommentNodes', () => {
         .toEqual('eslint-disable-next-line');
     expect(templateCommentsOptionEnabled.commentNodes![0].sourceSpan)
         .toBeInstanceOf(ParseSourceSpan);
+    expect(templateCommentsOptionEnabled.commentNodes![0].sourceSpan.toString())
+        .toEqual('<!-- eslint-disable-next-line -->');
     expect(templateCommentsOptionEnabled.commentNodes![1]).toBeInstanceOf(Comment);
     expect(templateCommentsOptionEnabled.commentNodes![1].value).toEqual('some nested comment');
     expect(templateCommentsOptionEnabled.commentNodes![1].sourceSpan)
         .toBeInstanceOf(ParseSourceSpan);
+    expect(templateCommentsOptionEnabled.commentNodes![1].sourceSpan.toString())
+        .toEqual('<!-- some nested comment -->');
   });
 });
