Skip to content

fix(compiler): don't report parse error for interpolation inside string in property binding#40267

Closed
crisbeto wants to merge 4 commits intoangular:masterfrom
crisbeto:39601-quoted-interpolation-binding
Closed

fix(compiler): don't report parse error for interpolation inside string in property binding#40267
crisbeto wants to merge 4 commits intoangular:masterfrom
crisbeto:39601-quoted-interpolation-binding

Conversation

@crisbeto
Copy link
Member

Currently we check whether a property binding contains an interpolation using a regex so that we can throw an error. The problem is that the regex doesn't account for quotes which means that something like [prop]="'{{ foo }}'" will be considered an error, even though it's not actually an interpolation.

These changes build on top of the logic from #39826 to account for interpolation characters inside quotes.

Fixes #39601.

…ng in property binding

Currently we check whether a property binding contains an interpolation using a regex so
that we can throw an error. The problem is that the regex doesn't account for quotes
which means that something like `[prop]="'{{ foo }}'"` will be considered an error, even
though it's not actually an interpolation.

These changes build on top of the logic from angular#39826 to account for interpolation
characters inside quotes.

Fixes angular#39601.
@google-cla google-cla bot added the cla: yes label Dec 26, 2020
@crisbeto crisbeto marked this pull request as ready for review December 26, 2020 11:05
@pullapprove pullapprove bot requested a review from alxhub December 26, 2020 11:05
@pullapprove pullapprove bot added area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime labels Dec 26, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 26, 2020
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix and removed area: core Issues related to the framework runtime labels Dec 26, 2020
@pullapprove pullapprove bot added area: core Issues related to the framework runtime labels Dec 26, 2020
Comment on lines 308 to 316
this._forEachUnquotedChar(input, 0, charIndex => {
if (startIndex === -1) {
startIndex = input.indexOf(start, charIndex);
return false;
} else {
endIndex = this._getInterpolationEndIndex(input, end, charIndex);
return endIndex > -1;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is a bit inefficient, since it will continue to run through each unquoted character even indexOf found the start.

Also I am concerned that the following string would find a false positive:

abc '{{' }} def

since the {{ is in a string, it should be ignored but would be picked up by the indexOf(), when the charIndex is 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would also be clearer if you created a new function _getInterpolationStartIndex(input, expressionStart, start):

  private _getInterpolationStartIndex(input: string, expressionStart: string, start: number): number {
    let result = -1;
    this._forEachUnquotedChar(input, start, charIndex => {
      if (input.startsWith(expressionStart, charIndex)) {
        result = charIndex;
        return true;
      } else {
        return false;
      }
    });
   return result;

Then you could do:

const startIndex = this._getInterpolationStartIndex(input, start, 0);
if (startIndex === -1) {
  return;
}
const endIndex = this._getInterpolationEndIndex(input, end, startIndex+start.length);
if (endIndex === -1) {
  return;
}

this._reportError(...);

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to add a _getInterpolationStartIndex, because there's some other logic that looks for an interpolation start in splitInterpolation, but the requirements there are slightly different. As for the abc '{{' }} def case, I think I tried something similar and it was reported as a different (syntax?) error further down the compilation process.

@crisbeto
Copy link
Member Author

crisbeto commented Dec 29, 2020

I've added the extra test cases and addressed the feedback. @petebacondarwin

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @crisbeto. One last thing.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the quick turnaround.

@crisbeto crisbeto added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 29, 2020
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Dec 29, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir
Copy link
Contributor

FYI, presubmit is successful for the changes in this PR.

@AndrewKushnir AndrewKushnir removed the request for review from alxhub January 5, 2021 20:22
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jan 5, 2021
@AndrewKushnir AndrewKushnir removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jan 5, 2021
josephperrott pushed a commit that referenced this pull request Jan 5, 2021
…ng in property binding (#40267)

Currently we check whether a property binding contains an interpolation using a regex so
that we can throw an error. The problem is that the regex doesn't account for quotes
which means that something like `[prop]="'{{ foo }}'"` will be considered an error, even
though it's not actually an interpolation.

These changes build on top of the logic from #39826 to account for interpolation
characters inside quotes.

Fixes #39601.

PR Close #40267
@angular-automatic-lock-bot
Copy link

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 Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strings in bindings are not allowed to contain interpolation syntax

3 participants