templates-no-negated-async not reporting some failures

[rafaelss95]

Reproduction:

{
  "rules": {
    "templates-no-negated-async": true
  }
}

@Component({
  selector: 'app-test',
  template: `
    <div *ngFor="let a of collection; trackBy: trackByFn"></div>
    {{ (foo | async) == false }} // not reporting failure here
    {{ !(bar | async) }} // not reporting failure here
  `,
})
export class TestComponent {}

@Component({
  selector: 'app-test',
  template: `
    <div *ngIf="!(a | async)"></div> // idk if it's intentional, but it's not reporting failure
    {{ (foo | async) == false }} // not reporting failure here
    {{ !(bar | async) }} // not reporting failure here
    {{ !(isHandset | async) ? 'dialog' : 'navigation' }} // not reporting failure here
  `,
})
export class TestComponent {}

[rafaelss95]

I've started to looking at this issue and I found what's the problem:

codelyzer/src/templatesNoNegatedAsyncRule.ts

Line 28 in 5a84041

const operator = this.codeWithMap.code.slice(endLeftSpan, startRightSpan);

For the first template above the results are:

endLeftSpan = 28
startRightSpan = 33
this.codeWithMap.code.slice(endLeftSpan, startRightSpan); returns "t a o" (from *ngFor line).

It seems that there is a real problem with multiline templates. It's only getting the code from the first line, I really don't know why.

---

Question:

Why do we always assume that the expression is being compared to false? What if the user put ! trying to represent a nil check (null/undefined)?

@Component({
  selector: 'app-test',
  template: '<div *ngIf="!(data$ | async)">Some content</div>'
})
class TestComponent implements OnInit {
  data$: Observable<User | undefined>;

  ngOnInit() {
    this.data$ = someServiceThatCanReturnUserOrUndefined;
  }
}

In this case, an user is trying to use ! to check if Observable emits a undefined or even a null value, so the expression could be changed to:

<div *ngIf="(data$ | async) == null">Some content</div>

<!-- or -->

<div *ngIf="(data$ | async) === undefined">Some content</div>

... instead, we replace the expression with === false, what brings an unexpected behavior, a completely unsafe replace.

---

Also, there are real failures related to the current replacement:

<div *ngIf="!(foo | async)">Something</div>

... After running --fix:

<div *ngIf="(foo | async) === false >Something</div>
<!-- final quote removed and extra whitespace added -->

---

IMHO, we should remove the "fix" option for this rule, since we can't assume what the "expression" is trying to do... or maybe leave only the replace for strict equality (== false -> === false).

@wKoza @mgechev let me know your thoughts.

[mgechev]

IMHO, we should remove the "fix" option for this rule, since we can't assume what the "expression" is trying to do... or maybe leave only the replace for strict equality (== false -> === false).

Yes, we should remove the fix because it's not safe.

[rafaelss95]

@mgechev I'm still trying to find a valid use case for this rule, do you have one?

Here...

codelyzer/src/templatesNoNegatedAsyncRule.ts

Line 50 in 5a84041

this.addFailureFromStartToEnd(spanStart, spanEnd, 'Async pipes can not be negated, use (observable | async) === false instead', [

... we say "Async pipes can not be negated". Why not?

Take a look at this sample:

@Component({
  selector: 'app-root',
  template: `
    <div *ngIf="!(obs$ | async)">
      Loading... <!-- this will be displayed until the observable emits a value -->
    </div>
  `
})
export class AppComponent  {
  obs$ = observableOf([1, 2, 3]).pipe(
    delay(2000) // simulates an http request
  );
}

What would be the benefits of an explicit check like this <div *ngIf="(obs$ | async) === null" instead of what I did above?

Source sample

[rafaelss95]

@mgechev just to complement: unless there's a valid case that this rule could be useful, I'm voting in favor of deprecate this rule.

[mgechev]

I'll look at the story behind it over the weekend.

[rafaelss95]

@mgechev Friendly bump, have you looked?

[mgechev]

Yes, here's the motivation. I'm not sure if the value it brings justifies the corner cases you pointed. I'd vote to drop the rule. Let's keep the issue open for a few more days and collect opinions.

[rafaelss95]

@mgechev better close this and create a new issue about deprecate this rule or keep the discussion here?

[mgechev]

Sounds good!

[mgechev]

@rafaelss95 since we had this conversation, we reevaluated the pros and cons of this rule and introduced it in Google. Let's keep it as part of codelyzer.

[rafaelss95]

@mgechev Okay, can we just update the following failure message?

codelyzer/src/templatesNoNegatedAsyncRule.ts

Line 68 in bc5a7ec

static readonly FAILURE_STRING_NEGATED_PIPE = 'Async pipes can not be negated, use (observable | async) === false instead';

[mgechev]

Sure. How do you suggest to update it?