refactor(core): assert TNode is not a container when setting attribute on element (#37111)

This PR provides a more helpful error than the one currently present:
`el.setAttribute is not a function`. It is not valid to have directives with host bindings
on `ng-template` or `ng-container` nodes. VE would silently ignore this, while Ivy
attempts to set the attribute and throws an error because these are comment nodes
and do not have `setAttribute` functionality.

It is better to throw a helpful error than to silently ignore this because
putting a directive with host binding on an `ng-template` or `ng-container` is most often a mistake.
Developers should be made aware that the host binding will have no effect in these cases.

Note that an error is already thrown in Ivy, as mentioned above, so this
is not a breaking change and can be merged to both master and patch.

Resolves #35994

PR Close #37111

goldens/size-tracking/integration-payloads.json

@@ -62,7 +62,7 @@
         "bundle": "TODO(i): we should define ngDevMode to false in Closure, but --define only works in the global scope.",
         "bundle": "TODO(i): (FW-2164) TS 3.9 new class shape seems to have broken Closure in big ways. The size went from 169991 to 252338",
         "bundle": "TODO(i): after removal of tsickle from ngc-wrapped / ng_package, we had to switch to SIMPLE optimizations which increased the size from 252338 to 1197869, see PR#37221 and PR#37317 for more info",
-        "bundle": 1197869
+        "bundle": 1198831
       }
     }
   }

packages/core/src/render3/instructions/shared.ts

@@ -29,7 +29,7 @@ import {isProceduralRenderer, RComment, RElement, Renderer3, RendererFactory3, R
 import {SanitizerFn} from '../interfaces/sanitization';
 import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks';
 import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TVIEW, TView, TViewType} from '../interfaces/view';
-import {assertNodeOfPossibleTypes} from '../node_assert';
+import {assertNodeNotOfTypes, assertNodeOfPossibleTypes} from '../node_assert';
 import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher';
 import {enterView, getBindingsEnabled, getCheckNoChangesMode, getCurrentDirectiveIndex, getIsParent, getPreviousOrParentTNode, getSelectedIndex, leaveView, setBindingIndex, setBindingRootForHostBindings, setCheckNoChangesMode, setCurrentDirectiveIndex, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state';
 import {NO_CHANGE} from '../tokens';
@@ -1484,8 +1484,14 @@ function addComponentLogic<T>(lView: LView, hostTNode: TElementNode, def: Compon
 export function elementAttributeInternal(
     tNode: TNode, lView: LView, name: string, value: any, sanitizer: SanitizerFn|null|undefined,
     namespace: string|null|undefined) {
-  ngDevMode && assertNotSame(value, NO_CHANGE as any, 'Incoming value should never be NO_CHANGE.');
-  ngDevMode && validateAgainstEventAttributes(name);
+  if (ngDevMode) {
+    assertNotSame(value, NO_CHANGE as any, 'Incoming value should never be NO_CHANGE.');
+    validateAgainstEventAttributes(name);
+    assertNodeNotOfTypes(
+        tNode, [TNodeType.Container, TNodeType.ElementContainer],
+        `Attempted to set attribute \`${name}\` on a container node. ` +
+            `Host bindings are not valid on ng-container or ng-template.`);
+  }
   const element = getNativeByTNode(tNode, lView) as RElement;
   const renderer = lView[RENDERER];
   if (value == null) {

packages/core/src/render3/node_assert.ts

@@ -34,6 +34,15 @@ export function assertNodeOfPossibleTypes(tNode: TNode|null, ...types: TNodeType
       `Should be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`);
 }
 
+export function assertNodeNotOfTypes(tNode: TNode, types: TNodeType[], message?: string): void {
+  assertDefined(tNode, 'should be called with a TNode');
+  const found = types.some(type => tNode.type === type);
+  assertEqual(
+      found, false,
+      message ??
+          `Should not be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`);
+}
+
 function typeName(type: TNodeType): string {
   if (type == TNodeType.Projection) return 'Projection';
   if (type == TNodeType.Container) return 'Container';

packages/core/test/acceptance/host_binding_spec.ts

@@ -1427,4 +1427,53 @@ describe('host bindings', () => {
         '<img src="unsafe:javascript:alert(3)">', bypassSanitizationTrustHtml,
         /* isAttribute */ false);
   });
+
+  onlyInIvy('VE would silently ignore this').describe('host binding on containers', () => {
+    @Directive({selector: '[staticHostAtt]', host: {'static': 'attr'}})
+    class StaticHostAttr {
+      constructor() {}
+    }
+
+    @Directive({selector: '[dynamicHostAtt]', host: {'[attr.dynamic]': '"dynamic"'}})
+    class DynamicHostAttr {
+      constructor() {}
+    }
+
+    it('should fail with expected error with ng-container', () => {
+      @Component({
+        selector: 'my-app',
+        template: `
+          <ng-template #ref></ng-template>
+          <ng-container [ngTemplateOutlet]="ref" staticHostAtt dynamicHostAtt></ng-container>
+        `
+      })
+      class App {
+      }
+
+      const comp =
+          TestBed.configureTestingModule({declarations: [App, StaticHostAttr, DynamicHostAttr]})
+              .createComponent(App);
+      // TODO(FW-2202): binding static attrs won't throw an error. We should be more consistent.
+      expect(() => comp.detectChanges())
+          .toThrowError(
+              /Attempted to set attribute `dynamic` on a container node. Host bindings are not valid on ng-container or ng-template./);
+    });
+
+    it('should fail with expected error with ng-template', () => {
+      @Component({
+        selector: 'my-app',
+        template: ` <ng-template staticHostAtt dynamicHostAtt></ng-template> `
+      })
+      class App {
+      }
+
+      const comp =
+          TestBed.configureTestingModule({declarations: [App, StaticHostAttr, DynamicHostAttr]})
+              .createComponent(App);
+      // TODO(FW-2202): binding static attrs won't throw an error. We should be more consistent.
+      expect(() => comp.detectChanges())
+          .toThrowError(
+              /Attempted to set attribute `dynamic` on a container node. Host bindings are not valid on ng-container or ng-template./);
+    });
+  });
 });