Skip to content

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

Closed
P4 wants to merge 1 commit intoangular:mainfrom
P4:comment-span
Closed

fix(compiler): return full spans for Comment nodes#50855
P4 wants to merge 1 commit intoangular:mainfrom
P4:comment-span

Conversation

@P4
Copy link
Copy Markdown
Contributor

@P4 P4 commented Jun 26, 2023 •
edited
Loading

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 Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The HTML parser creates Comment nodes with ParseSourceSpan taken directly from the opening COMMENT_START token, causing every comment to claim to be 4 characters long - sourceSpan.toString() always returns <!--.

What is the new behavior?

The parser computes a new ParseSourceSpan from the start and end tokens, covering the full comment.
sourceSpan.toString() returns the whole comment starting with <!-- and ending with -->.

Does this PR introduce a breaking change?

  • Yes
  • No

Not sure honestly. Several tests for @angular/compiler had to be updated because they expected a stringified version of the AST node to be <!--, similar issues might occur in any third-party code that uses the AST directly.

Other information

Example of what the @angular-eslint interaction mentioned above looks like:

<!-- eslint-disable @angular-eslint/template/no-call-expression -->
<p>no lint errors in this file</p>

Since there are no call expressions, linting this file reports an unused directive:

src/app/app.component.html
  1:1  warning  Unused eslint-disable directive (no problems were reported from '@angular-eslint/template/no-call-expression')

✖ 1 problem (0 errors, 1 warning)
  0 errors and 1 warning potentially fixable with the `--fix` option.

Running with --fix should remove the comment entirely, but instead produces the following change:

-<!-- eslint-disable @angular-eslint/template/no-call-expression -->
+  eslint-disable @angular-eslint/template/no-call-expression -->
<p>no lint errors in this file</p>

The eslint directive ends up as text inside the component, which is not ideal.

@google-cla
Copy link
Copy Markdown

google-cla bot commented Jun 26, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@pullapprove pullapprove bot requested a review from AndrewKushnir June 26, 2023 14:42
@P4 P4 force-pushed the comment-span branch 2 times, most recently from 6f190ce to c62801f Compare June 26, 2023 14:52
JoostK
JoostK approved these changes Jun 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@JoostK JoostK added target: patch This PR is targeted for the next patch release action: presubmit The PR is in need of a google3 presubmit area: compiler Issues related to `ngc`, Angular's template compiler compiler: parser labels Jun 26, 2023
@ngbot ngbot bot modified the milestone: Backlog Jun 26, 2023
@JoostK JoostK removed the request for review from AndrewKushnir June 26, 2023 15:53
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Jun 27, 2023

Presubmit.

@P4
Copy link
Copy Markdown
Contributor Author

P4 commented Jul 27, 2023

@AndrewKushnir It's been a month, is this still blocked on something internally?

@P4 @AndrewKushnir
fix(compiler): return full spans for Comment nodes
2ca6d2f 
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.
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Jul 28, 2023

@P4 sorry for the delay. I've rebased the PR on top of the most recent main and restarted tests in Google's codebase (internal-only link). Will let you know if there are any issues identified by those tests.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Jul 28, 2023
@AndrewKushnir
Copy link
Copy Markdown
Contributor

AndrewKushnir commented Jul 28, 2023 •
edited
Loading

Caretaker note: presubmit is "green" (only pre-existing failing tests). This PR is ready for merge.

@alxhub
Copy link
Copy Markdown
Member

alxhub commented Jul 28, 2023

This PR was merged into the repository by commit 6755f53.

alxhub pushed a commit that referenced this pull request Jul 28, 2023
@P4 @alxhub
fix(compiler): return full spans for Comment nodes (#50855)
cc722ea 
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
@alxhub alxhub closed this in 6755f53 Jul 28, 2023
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Aug 29, 2023

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 29, 2023
@P4 P4 deleted the comment-span branch November 22, 2023 15:28
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@P4 @ChellappanRajan
fix(compiler): return full spans for Comment nodes (angular#50855)
7e4e195 
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 angular#50855
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler compiler: parser merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@P4 @AndrewKushnir @alxhub @JoostK