From 81dce5c6649ddc56903a37ef6a26dcd3ee37df96 Mon Sep 17 00:00:00 2001
From: Daniel Trevino <23410540+danieltre23@users.noreply.github.com>
Date: Tue, 22 Jun 2021 10:48:27 -0500
Subject: [PATCH] fix(compiler-cli): check split two way binding (#42601)

Check for split two way binding when output is not declared to make error message clearer.

PR Close #42601
---
 goldens/public-api/compiler-cli/error_code.md |  1 +
 .../src/ngtsc/diagnostics/src/error_code.ts   |  6 ++
 .../typecheck/diagnostics/src/diagnostic.ts   | 49 +++++++-----
 .../src/ngtsc/typecheck/src/oob.ts            | 71 +++++++++++++++--
 .../ngtsc/typecheck/src/type_check_block.ts   | 38 +++++++++
 .../src/ngtsc/typecheck/test/test_utils.ts    |  1 +
 .../test/ngtsc/template_typecheck_spec.ts     | 79 +++++++++++++++++++
 7 files changed, 217 insertions(+), 28 deletions(-)

diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md
index cdccfd018d..84a4a59516 100644
--- a/goldens/public-api/compiler-cli/error_code.md
+++ b/goldens/public-api/compiler-cli/error_code.md
@@ -50,6 +50,7 @@ export enum ErrorCode {
     PIPE_MISSING_NAME = 2002,
     SCHEMA_INVALID_ATTRIBUTE = 8002,
     SCHEMA_INVALID_ELEMENT = 8001,
+    SPLIT_TWO_WAY_BINDING = 8007,
     SUGGEST_STRICT_TEMPLATES = 10001,
     SUGGEST_SUBOPTIMAL_TYPE_INFERENCE = 10002,
     // (undocumented)
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
index a0dc5e3f5a..e6dba314c2 100644
--- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
+++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -166,6 +166,12 @@ export enum ErrorCode {
    */
   DUPLICATE_VARIABLE_DECLARATION = 8006,
 
+  /**
+   * A template has a two way binding (two bindings created by a single syntactial element)
+   * in which the input and output are going to different places.
+   */
+  SPLIT_TWO_WAY_BINDING = 8007,
+
   /**
    * The template type-checking engine would need to generate an inline type check block for a
    * component, but the current type-checking environment doesn't support it.
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts b/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts
index 4070a47a7a..61d2f143aa 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/diagnostics/src/diagnostic.ts
@@ -33,21 +33,26 @@ export interface TemplateDiagnostic extends ts.Diagnostic {
 export function makeTemplateDiagnostic(
     templateId: TemplateId, mapping: TemplateSourceMapping, span: ParseSourceSpan,
     category: ts.DiagnosticCategory, code: number, messageText: string|ts.DiagnosticMessageChain,
-    relatedMessage?: {
+    relatedMessages?: {
       text: string,
-      span: ParseSourceSpan,
-    }): TemplateDiagnostic {
+      start: number,
+      end: number,
+      sourceFile: ts.SourceFile,
+    }[]): TemplateDiagnostic {
   if (mapping.type === 'direct') {
     let relatedInformation: ts.DiagnosticRelatedInformation[]|undefined = undefined;
-    if (relatedMessage !== undefined) {
-      relatedInformation = [{
-        category: ts.DiagnosticCategory.Message,
-        code: 0,
-        file: mapping.node.getSourceFile(),
-        start: relatedMessage.span.start.offset,
-        length: relatedMessage.span.end.offset - relatedMessage.span.start.offset,
-        messageText: relatedMessage.text,
-      }];
+    if (relatedMessages !== undefined) {
+      relatedInformation = [];
+      for (const relatedMessage of relatedMessages) {
+        relatedInformation.push({
+          category: ts.DiagnosticCategory.Message,
+          code: 0,
+          file: relatedMessage.sourceFile,
+          start: relatedMessage.start,
+          length: relatedMessage.end - relatedMessage.start,
+          messageText: relatedMessage.text,
+        });
+      }
     }
     // For direct mappings, the error is shown inline as ngtsc was able to pinpoint a string
     // constant within the `@Component` decorator for the template. This allows us to map the error
@@ -82,15 +87,17 @@ export function makeTemplateDiagnostic(
         fileName, mapping.template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX);
 
     let relatedInformation: ts.DiagnosticRelatedInformation[] = [];
-    if (relatedMessage !== undefined) {
-      relatedInformation.push({
-        category: ts.DiagnosticCategory.Message,
-        code: 0,
-        file: sf,
-        start: relatedMessage.span.start.offset,
-        length: relatedMessage.span.end.offset - relatedMessage.span.start.offset,
-        messageText: relatedMessage.text,
-      });
+    if (relatedMessages !== undefined) {
+      for (const relatedMessage of relatedMessages) {
+        relatedInformation.push({
+          category: ts.DiagnosticCategory.Message,
+          code: 0,
+          file: relatedMessage.sourceFile,
+          start: relatedMessage.start,
+          length: relatedMessage.end - relatedMessage.start,
+          messageText: relatedMessage.text,
+        });
+      }
     }
 
     relatedInformation.push({
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
index 446debb4ee..ec841fcd69 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
@@ -6,7 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler';
+import {BindingPipe, PropertyWrite, TmplAstBoundEvent, TmplAstElement, TmplAstReference, TmplAstVariable} from '@angular/compiler';
+import {BoundAttribute} from '@angular/compiler/src/render3/r3_ast';
 import * as ts from 'typescript';
 
 import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics';
@@ -74,6 +75,13 @@ export interface OutOfBandDiagnosticRecorder {
    * type-checking configuration prohibits their usage.
    */
   suboptimalTypeInference(templateId: TemplateId, variables: TmplAstVariable[]): void;
+
+  /**
+   * Reports a split two way binding error message.
+   */
+  splitTwoWayBinding(
+      templateId: TemplateId, input: BoundAttribute, output: TmplAstBoundEvent,
+      inputConsumer: ClassDeclaration, outputConsumer: ClassDeclaration|TmplAstElement): void;
 }
 
 export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -133,10 +141,12 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
     }
     this._diagnostics.push(makeTemplateDiagnostic(
         templateId, mapping, sourceSpan, ts.DiagnosticCategory.Error,
-        ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, {
+        ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, [{
           text: `The variable ${assignment.name} is declared here.`,
-          span: target.valueSpan || target.sourceSpan,
-        }));
+          start: target.valueSpan?.start.offset || target.sourceSpan.start.offset,
+          end: target.valueSpan?.end.offset || target.sourceSpan.end.offset,
+          sourceFile: mapping.node.getSourceFile(),
+        }]));
   }
 
   duplicateTemplateVar(
@@ -152,10 +162,12 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
     // TODO(alxhub): allocate to a tighter span once one is available.
     this._diagnostics.push(makeTemplateDiagnostic(
         templateId, mapping, variable.sourceSpan, ts.DiagnosticCategory.Error,
-        ngErrorCode(ErrorCode.DUPLICATE_VARIABLE_DECLARATION), errorMsg, {
+        ngErrorCode(ErrorCode.DUPLICATE_VARIABLE_DECLARATION), errorMsg, [{
           text: `The variable '${firstDecl.name}' was first declared here.`,
-          span: firstDecl.sourceSpan,
-        }));
+          start: firstDecl.sourceSpan.start.offset,
+          end: firstDecl.sourceSpan.end.offset,
+          sourceFile: mapping.node.getSourceFile(),
+        }]));
   }
 
   requiresInlineTcb(templateId: TemplateId, node: ClassDeclaration): void {
@@ -211,6 +223,51 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
         templateId, mapping, diagnosticVar.keySpan, ts.DiagnosticCategory.Suggestion,
         ngErrorCode(ErrorCode.SUGGEST_SUBOPTIMAL_TYPE_INFERENCE), message));
   }
+
+  splitTwoWayBinding(
+      templateId: TemplateId, input: BoundAttribute, output: TmplAstBoundEvent,
+      inputConsumer: ClassDeclaration, outputConsumer: ClassDeclaration|TmplAstElement): void {
+    const mapping = this.resolver.getSourceMapping(templateId);
+    const errorMsg = `The property and event halves of the two-way binding '${
+        input.name}' are not bound to the same target.
+            Find more at https://angular.io/guide/two-way-binding#how-two-way-binding-works`;
+
+    const relatedMessages: {text: string; start: number; end: number;
+                            sourceFile: ts.SourceFile;}[] = [];
+
+    relatedMessages.push({
+      text: `The property half of the binding is to the '${inputConsumer.name.text}' component.`,
+      start: inputConsumer.name.getStart(),
+      end: inputConsumer.name.getEnd(),
+      sourceFile: inputConsumer.name.getSourceFile(),
+    });
+
+    if (outputConsumer instanceof TmplAstElement) {
+      let message = `The event half of the binding is to a native event called '${
+          input.name}' on the <${outputConsumer.name}> DOM element.`;
+      if (!mapping.node.getSourceFile().isDeclarationFile) {
+        message += `\n \n Are you missing an output declaration called '${output.name}'?`;
+      }
+      relatedMessages.push({
+        text: message,
+        start: outputConsumer.sourceSpan.start.offset + 1,
+        end: outputConsumer.sourceSpan.start.offset + outputConsumer.name.length + 1,
+        sourceFile: mapping.node.getSourceFile(),
+      });
+    } else {
+      relatedMessages.push({
+        text: `The event half of the binding is to the '${outputConsumer.name.text}' component.`,
+        start: outputConsumer.name.getStart(),
+        end: outputConsumer.name.getEnd(),
+        sourceFile: outputConsumer.name.getSourceFile(),
+      });
+    }
+
+
+    this._diagnostics.push(makeTemplateDiagnostic(
+        templateId, mapping, input.keySpan, ts.DiagnosticCategory.Error,
+        ngErrorCode(ErrorCode.SPLIT_TWO_WAY_BINDING), errorMsg, relatedMessages));
+  }
 }
 
 function makeInlineDiagnostic(
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
index f905a4915d..566f772734 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
@@ -984,6 +984,11 @@ export class TcbDirectiveOutputsOp extends TcbOp {
       if (output.type !== ParsedEventType.Regular || !outputs.hasBindingPropertyName(output.name)) {
         continue;
       }
+
+      if (this.tcb.env.config.checkTypeOfOutputEvents && output.name.endsWith('Change')) {
+        const inputName = output.name.slice(0, -6);
+        isSplitTwoWayBinding(inputName, output, this.node.inputs, this.tcb);
+      }
       // TODO(alxhub): consider supporting multiple fields with the same property name for outputs.
       const field = outputs.getByBindingPropertyName(output.name)![0].classPropertyName;
 
@@ -1049,6 +1054,14 @@ class TcbUnclaimedOutputsOp extends TcbOp {
         continue;
       }
 
+      if (this.tcb.env.config.checkTypeOfOutputEvents && output.name.endsWith('Change')) {
+        const inputName = output.name.slice(0, -6);
+        if (isSplitTwoWayBinding(inputName, output, this.element.inputs, this.tcb)) {
+          // Skip this event handler as the error was already handled.
+          continue;
+        }
+      }
+
       if (output.type === ParsedEventType.Animation) {
         // Animation output bindings always have an `$event` parameter of type `AnimationEvent`.
         const eventType = this.tcb.env.config.checkTypeOfAnimationEvents ?
@@ -1979,6 +1992,31 @@ function tcbEventHandlerExpression(ast: AST, tcb: Context, scope: Scope): ts.Exp
   return translator.translate(ast);
 }
 
+function isSplitTwoWayBinding(
+    inputName: string, output: TmplAstBoundEvent, inputs: TmplAstBoundAttribute[], tcb: Context) {
+  const input = inputs.find(input => input.name === inputName);
+  if (input === undefined || input.sourceSpan !== output.sourceSpan) {
+    return false;
+  }
+  // Input consumer should be a directive because it's claimed
+  const inputConsumer = tcb.boundTarget.getConsumerOfBinding(input) as TypeCheckableDirectiveMeta;
+  const outputConsumer = tcb.boundTarget.getConsumerOfBinding(output);
+  if (outputConsumer === null || inputConsumer.ref === undefined ||
+      outputConsumer instanceof TmplAstTemplate) {
+    return false;
+  }
+  if (outputConsumer instanceof TmplAstElement) {
+    tcb.oobRecorder.splitTwoWayBinding(
+        tcb.id, input, output, inputConsumer.ref.node, outputConsumer);
+    return true;
+  } else if (outputConsumer.ref !== inputConsumer.ref) {
+    tcb.oobRecorder.splitTwoWayBinding(
+        tcb.id, input, output, inputConsumer.ref.node, outputConsumer.ref.node);
+    return true;
+  }
+  return false;
+}
+
 class TcbEventHandlerTranslator extends TcbExpressionTranslator {
   protected override resolve(ast: AST): ts.Expression|null {
     // Recognize a property read on the implicit receiver corresponding with the event parameter
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
index 9130af7095..860b0aafad 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
@@ -694,4 +694,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
   requiresInlineTcb(): void {}
   requiresInlineTypeConstructors(): void {}
   suboptimalTypeInference(): void {}
+  splitTwoWayBinding(): void {}
 }
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
index 3d28fb5841..9543acfcc9 100644
--- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
+++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -338,6 +338,85 @@ export declare class AnimationEvent {
       expect(diags.length).toBe(0);
     });
 
+    it('should check split two way binding', () => {
+      env.tsconfig({strictTemplates: true});
+      env.write('test.ts', `
+        import {Component, Input, NgModule} from '@angular/core';
+
+        @Component({
+          selector: 'test',
+          template: '<child-cmp [(value)]="counterValue"></child-cmp>',
+        })
+
+        export class TestCmp {
+          counterValue = 0;
+        }
+
+        @Component({
+          selector: 'child-cmp',
+          template: '',
+        })
+
+        export class ChildCmp {
+          @Input() value = 0;
+        }
+
+        @NgModule({
+          declarations: [TestCmp, ChildCmp],
+        })
+        export class Module {}
+      `);
+      const diags = env.driveDiagnostics();
+      expect(diags.length).toBe(1);
+      expect(diags[0].code).toBe(ngErrorCode(ErrorCode.SPLIT_TWO_WAY_BINDING));
+      expect(getSourceCodeForDiagnostic(diags[0])).toBe('value');
+      expect(diags[0].relatedInformation!.length).toBe(2);
+      expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('ChildCmp');
+      expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![1])).toBe('child-cmp');
+    });
+
+    it('when input and output go to different directives', () => {
+      env.tsconfig({strictTemplates: true});
+      env.write('test.ts', `
+        import {Component, Input, NgModule, Output, Directive} from '@angular/core';
+
+        @Component({
+          selector: 'test',
+          template: '<child-cmp [(value)]="counterValue"></child-cmp>',
+        })
+        export class TestCmp {
+          counterValue = 0;
+        }
+
+        @Directive({
+          selector: 'child-cmp'
+        })
+        export class ChildCmpDir {
+          @Output() valueChange: any;
+        }
+
+        @Component({
+          selector: 'child-cmp',
+          template: '',
+        })
+        export class ChildCmp {
+          @Input() value = 0;
+        }
+
+        @NgModule({
+          declarations: [TestCmp, ChildCmp, ChildCmpDir],
+        })
+        export class Module {}
+      `);
+      const diags = env.driveDiagnostics();
+      expect(diags.length).toBe(1);
+      expect(diags[0].code).toBe(ngErrorCode(ErrorCode.SPLIT_TWO_WAY_BINDING));
+      expect(getSourceCodeForDiagnostic(diags[0])).toBe('value');
+      expect(diags[0].relatedInformation!.length).toBe(2);
+      expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![0])).toBe('ChildCmp');
+      expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![1])).toBe('ChildCmpDir');
+    });
+
     describe('strictInputTypes', () => {
       beforeEach(() => {
         env.write('test.ts', `