two bugs of angular-whitespace

[sagittarius-rev]

1. improper position of the errors with angular-whitespace

For example, the template is:

<div *ngFor="let item of list;trackBy item.id;let $index=index">{{ item.title }}</div>

When lint this Angular template, it would locate an error before trackBy (Missing white space after semicolon) as expected. But actually, it will locate the error before item.id .

This bug caused by angularWhitespaceRule.ts of line 86, line 97 and line 105:

directive.split('=')[1].trim()

It supposes that the right side code of the directive would equal to directive.split('=')[1], but the microsyntax of the Angular directives allow equality sign existed within the expression! In this example, the rawExpression would be let item of list;trackBy item.id;let $inde wrongly, instead of let item of list;trackBy item.id;let $index=index .

How to fix this bug?

first, replace the code of line 86

      const rawExpression = directive.split('=')[1].trim();

with

      const match = /^([^=]+=\s*)([^]*?)\s*$/.exec(directive);
      const rawExpression = match[2];
      const positionFix = match[1].length + 1;

second, replace the code of line 97 and line 105

const start = prop.sourceSpan.start.offset + internalStart + directive.length - directive.split('=')[1].trim().length + 1;

with

const start = prop.sourceSpan.start.offset + internalStart + positionFix;

2. mistake of the auto fix with angular-whitespace

We can use tslint --fix to auto fix the errors of Angular template, for example:

<h4>{{title|translate    }}</h4>

As expected, the fix result would be:

<h4>{{ title | translate }}</h4>

But actually, the result is:

<h4>{{ titl e |translate }}</h4>

This bug caused by angularWhitespaceRule.ts of the code between line 48 and line 56:

          context.addFailure(
            context.createFailure(
            start, length, failure, [
            new Lint.Replacement(
              absolutePosition,
              length,
              `${InterpolationOpen} ${match[0].replace(InterpolationOpen, '').replace(InterpolationClose, '').trim()} ${InterpolationClose}`
            )
          ]));

Normally, when locating and fixing the errors, it must minimize the changes of the text, to avoid disturbing other fix actions. But the codes above changes the whole text of Angular interpolation, including {{ and }}.

There are two errors of the template, at first, it will be replaced to <h4>{{ title|translate }}</h4>, after that, two space will be inserted before and after the char at position 12: the char at this position is | formerly, but it has changed to e after the previous replacing. So the auto fixing caused a serious error.

How to fix this bug?

first, replace the codes between line 12 and line 15

const InterpolationNoWhitespaceRe = new RegExp(`${InterpolationOpen}\\S(.*?)\\S${InterpolationClose}|${InterpolationOpen}` +
  `\\s(.*?)\\S${InterpolationClose}|${InterpolationOpen}\\S(.*?)\\s${InterpolationClose}`, 'g');
const InterpolationExtraWhitespaceRe =
  new RegExp(`${InterpolationOpen}\\s\\s(.*?)\\s${InterpolationClose}|${InterpolationOpen}\\s(.*?)\\s\\s${InterpolationClose}`, 'g');

with

const InterpolationWhitespaceRe = new RegExp(`${InterpolationOpen}(\\s*)(.*?)(\\s*)${InterpolationClose}`, 'g');

second, replace the codes between line 42 and line 68

      const applyRegex = (regex: RegExp, failure: string) => {
        let match: RegExpExecArray | null;
        while (match = regex.exec(expr)) {
          const start = text.sourceSpan.start.offset + match.index;
          const absolutePosition = context.getSourcePosition(start);
          const length = match[0].length;
          context.addFailure(
            context.createFailure(
            start, length, failure, [
            new Lint.Replacement(
              absolutePosition,
              length,
              `${InterpolationOpen} ${match[0].replace(InterpolationOpen, '').replace(InterpolationClose, '').trim()} ${InterpolationClose}`
            )
          ]));
        }
      };
      InterpolationNoWhitespaceRe.lastIndex = 0;
      applyRegex(
        InterpolationNoWhitespaceRe,
        `Missing whitespace in interpolation; expecting ${InterpolationOpen} expr ${InterpolationClose}`
      );
      InterpolationExtraWhitespaceRe.lastIndex = 0;
      applyRegex(
        InterpolationExtraWhitespaceRe,
        `Extra whitespace in interpolation; expecting ${InterpolationOpen} expr ${InterpolationClose}`
      );

with

      const checkWhiteSpace = (subMatch: string, location: 'start' | 'end', position: number, absolutePosition: number) => {
          const { length } = subMatch;
          if (length === 1) {
              return;
          }
          const errorText = length === 0 ? 'Missing' : 'Extra';
          context.addFailure(context.createFailure(position, length,
            `${errorText} whitespace in interpolation ${location}; expecting ${InterpolationOpen} expr ${InterpolationClose}`, [
              new Lint.Replacement(absolutePosition, length, ' ')
          ]));
      };

      InterpolationWhitespaceRe.lastIndex = 0;
      let match: RegExpExecArray | null;
      while (match = InterpolationWhitespaceRe.exec(expr)) {
          const start = text.sourceSpan.start.offset + match.index;
          const absolutePosition = context.getSourcePosition(start);
          let positionFix = InterpolationOpen.length;

          checkWhiteSpace(match[1], 'start', start + positionFix, absolutePosition + positionFix);
          positionFix += match[1].length + match[2].length;
          checkWhiteSpace(match[3], 'end', start + positionFix, absolutePosition + positionFix);
      }

It splits the lint error Missing(Extra) whitespace in interpolation to Missing(Extra) whitespace in interpolation start and Missing(Extra) whitespace in interpolation end.

[sagittarius-rev]

For testing spec, rewrite the codes between line 42 and line 68 to:

      const checkWhiteSpace = (subMatch: string, location: 'start' | 'end', fixTo: string,
        position: number, absolutePosition: number, lengthFix: number
      ) => {
        const { length } = subMatch;
        if (length === 1) {
            return;
        }
        const errorText = length === 0 ? 'Missing' : 'Extra';
        context.addFailure(context.createFailure(position, length + lengthFix,
          `${errorText} whitespace in interpolation ${location}; expecting ${InterpolationOpen} expr ${InterpolationClose}`, [
            new Lint.Replacement(absolutePosition, length + lengthFix, fixTo)
        ]));
      };

      InterpolationWhitespaceRe.lastIndex = 0;
      let match: RegExpExecArray | null;
      while (match = InterpolationWhitespaceRe.exec(expr)) {
        const start = text.sourceSpan.start.offset + match.index;
        const absolutePosition = context.getSourcePosition(start);

        checkWhiteSpace(match[1], 'start', `${InterpolationOpen} `, start, absolutePosition, InterpolationOpen.length);
        const positionFix = InterpolationOpen.length + match[1].length + match[2].length;
        checkWhiteSpace(match[3], 'end', ` ${InterpolationClose}`, start + positionFix, absolutePosition + positionFix,
          InterpolationClose.length);
      }

After then, update the angularWhitespaceRule.ts code file also.

[wKoza]

Hi @sagittarius-rev,
Thanks for your analysis ! Do you want open a PR with this change ?

[mgechev]

He already did #470 ✨