fix(compiler): process imports first and declarations second while calculating scopes (#35850)

Prior to this commit, while calculating the scope for a module, Ivy compiler processed `declarations` field first and `imports` after that. That results in a couple issues:

* for Pipes with the same `name` and present in `declarations` and in an imported module, Pipe from imported module was selected. In View Engine the logic is opposite: Pipes from `declarations` field receive higher priority.
* for Directives with the same selector and present in `declarations` and in an imported module, we first invoked the logic of a Directive from `declarations` field and after that - imported Directive logic. In View Engine, it was the opposite and the logic of a Directive from the `declarations` field was invoked last.

In order to align Ivy and View Engine behavior, this commit updates the logic in which we populate module scope: we first process all imports and after that handle `declarations` field. As a result, in Ivy both use-cases listed above work similar to View Engine.

Resolves #35502.

PR Close #35850

packages/compiler-cli/src/ngtsc/scope/src/local.ts

@@ -296,9 +296,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
     const exportPipes = new Map<ts.Declaration, PipeMeta>();
 
     // The algorithm is as follows:
-    // 1) Add directives/pipes declared in the NgModule to the compilation scope.
+    // 1) Add all of the directives/pipes from each NgModule imported into the current one to the
-    // 2) Add all of the directives/pipes from each NgModule imported into the current one to the
+    //    compilation scope.
-    //    compilation scope. At this point, the compilation scope is complete.
+    // 2) Add directives/pipes declared in the NgModule to the compilation scope. At this point, the
+    //    compilation scope is complete.
     // 3) For each entry in the NgModule's exports:
     //    a) Attempt to resolve it as an NgModule with its own exported directives/pipes. If it is
     //       one, add them to the export scope of this NgModule.
@@ -307,31 +308,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
     //    c) If it's neither an NgModule nor a directive/pipe in the compilation scope, then this
     //       is an error.
 
-    // 1) add declarations.
+    // 1) process imports.
-    for (const decl of ngModule.declarations) {
-      const directive = this.localReader.getDirectiveMetadata(decl);
-      const pipe = this.localReader.getPipeMetadata(decl);
-      if (directive !== null) {
-        compilationDirectives.set(decl.node, {...directive, ref: decl});
-      } else if (pipe !== null) {
-        compilationPipes.set(decl.node, {...pipe, ref: decl});
-      } else {
-        this.taintedModules.add(ngModule.ref.node);
-
-        const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !);
-        diagnostics.push(makeDiagnostic(
-            ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode,
-            `The class '${decl.node.name.text}' is listed in the declarations of the NgModule '${ngModule.ref.node.name.text}', but is not a directive, a component, or a pipe.
-
-Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`,
-            [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}]));
-        continue;
-      }
-
-      declared.add(decl.node);
-    }
-
-    // 2) process imports.
     for (const decl of ngModule.imports) {
       const importScope = this.getExportedScope(decl, diagnostics, ref.node, 'import');
       if (importScope === null) {
@@ -353,6 +330,30 @@ Either remove it from the NgModule's declarations, or add an appropriate Angular
       }
     }
 
+    // 2) add declarations.
+    for (const decl of ngModule.declarations) {
+      const directive = this.localReader.getDirectiveMetadata(decl);
+      const pipe = this.localReader.getPipeMetadata(decl);
+      if (directive !== null) {
+        compilationDirectives.set(decl.node, {...directive, ref: decl});
+      } else if (pipe !== null) {
+        compilationPipes.set(decl.node, {...pipe, ref: decl});
+      } else {
+        this.taintedModules.add(ngModule.ref.node);
+
+        const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !);
+        diagnostics.push(makeDiagnostic(
+            ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode,
+            `The class '${decl.node.name.text}' is listed in the declarations ` +
+                `of the NgModule '${ngModule.ref.node.name.text}', but is not a directive, a component, or a pipe. ` +
+                `Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`,
+            [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}]));
+        continue;
+      }
+
+      declared.add(decl.node);
+    }
+
     // 3) process exports.
     // Exports can contain modules, components, or directives. They're processed differently.
     // Modules are straightforward. Directives and pipes from exported modules are added to the

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -598,6 +598,213 @@ runInEachFileSystem(os => {
       expect(jsContents).toContain('outputs: { output: "output" }');
     });
 
+    it('should pick a Pipe defined in `declarations` over imported Pipes', () => {
+      env.tsconfig({});
+      env.write('test.ts', `
+        import {Component, Pipe, NgModule} from '@angular/core';
+
+        // ModuleA classes
+
+        @Pipe({name: 'number'})
+        class PipeA {}
+
+        @NgModule({
+          declarations: [PipeA],
+          exports: [PipeA]
+        })
+        class ModuleA {}
+
+        // ModuleB classes
+
+        @Pipe({name: 'number'})
+        class PipeB {}
+
+        @Component({
+          selector: 'app',
+          template: '{{ count | number }}'
+        })
+        export class App {}
+
+        @NgModule({
+          imports: [ModuleA],
+          declarations: [PipeB, App],
+        })
+        class ModuleB {}
+      `);
+
+      env.driveMain();
+
+      const jsContents = trim(env.getContents('test.js'));
+      expect(jsContents).toContain('pipes: [PipeB]');
+    });
+
+    it('should respect imported module order when selecting Pipe (last imported Pipe is used)',
+       () => {
+         env.tsconfig({});
+         env.write('test.ts', `
+            import {Component, Pipe, NgModule} from '@angular/core';
+
+            // ModuleA classes
+
+            @Pipe({name: 'number'})
+            class PipeA {}
+
+            @NgModule({
+              declarations: [PipeA],
+              exports: [PipeA]
+            })
+            class ModuleA {}
+
+            // ModuleB classes
+
+            @Pipe({name: 'number'})
+            class PipeB {}
+
+            @NgModule({
+              declarations: [PipeB],
+              exports: [PipeB]
+            })
+            class ModuleB {}
+
+            // ModuleC classes
+
+            @Component({
+              selector: 'app',
+              template: '{{ count | number }}'
+            })
+            export class App {}
+
+            @NgModule({
+              imports: [ModuleA, ModuleB],
+              declarations: [App],
+            })
+            class ModuleC {}
+          `);
+
+         env.driveMain();
+
+         const jsContents = trim(env.getContents('test.js'));
+         expect(jsContents).toContain('pipes: [PipeB]');
+       });
+
+    it('should add Directives and Components from `declarations` at the end of the list', () => {
+      env.tsconfig({});
+      env.write('test.ts', `
+        import {Component, Directive, NgModule} from '@angular/core';
+
+        // ModuleA classes
+
+        @Directive({selector: '[dir]'})
+        class DirectiveA {}
+
+        @Component({
+          selector: 'comp',
+          template: '...'
+        })
+        class ComponentA {}
+
+        @NgModule({
+          declarations: [DirectiveA, ComponentA],
+          exports: [DirectiveA, ComponentA]
+        })
+        class ModuleA {}
+
+        // ModuleB classes
+
+        @Directive({selector: '[dir]'})
+        class DirectiveB {}
+
+        @Component({
+          selector: 'comp',
+          template: '...',
+        })
+        export class ComponentB {}
+
+        @Component({
+          selector: 'app',
+          template: \`
+            <div dir></div>
+            <comp></comp>
+          \`,
+        })
+        export class App {}
+
+        @NgModule({
+          imports: [ModuleA],
+          declarations: [DirectiveB, ComponentB, App],
+        })
+        class ModuleB {}
+      `);
+
+      env.driveMain();
+
+      const jsContents = trim(env.getContents('test.js'));
+      expect(jsContents).toContain('directives: [DirectiveA, DirectiveB, ComponentA, ComponentB]');
+    });
+
+    it('should respect imported module order while processing Directives and Components', () => {
+      env.tsconfig({});
+      env.write('test.ts', `
+        import {Component, Directive, NgModule} from '@angular/core';
+
+        // ModuleA classes
+
+        @Directive({selector: '[dir]'})
+        class DirectiveA {}
+
+        @Component({
+          selector: 'comp',
+          template: '...'
+        })
+        class ComponentA {}
+
+        @NgModule({
+          declarations: [DirectiveA, ComponentA],
+          exports: [DirectiveA, ComponentA]
+        })
+        class ModuleA {}
+
+        // ModuleB classes
+
+        @Directive({selector: '[dir]'})
+        class DirectiveB {}
+
+        @Component({
+          selector: 'comp',
+          template: '...'
+        })
+        class ComponentB {}
+
+        @NgModule({
+          declarations: [DirectiveB, ComponentB],
+          exports: [DirectiveB, ComponentB]
+        })
+        class ModuleB {}
+
+        // ModuleC classes
+
+        @Component({
+          selector: 'app',
+          template: \`
+            <div dir></div>
+            <comp></comp>
+          \`,
+        })
+        export class App {}
+
+        @NgModule({
+          imports: [ModuleA, ModuleB],
+          declarations: [App],
+        })
+        class ModuleC {}
+      `);
+
+      env.driveMain();
+
+      const jsContents = trim(env.getContents('test.js'));
+      expect(jsContents).toContain('directives: [DirectiveA, DirectiveB, ComponentA, ComponentB]');
+    });
+
     it('should compile Components with a templateUrl in a different rootDir', () => {
       env.tsconfig({}, ['./extraRootDir']);
       env.write('extraRootDir/test.html', '<p>Hello World</p>');

packages/core/src/render3/jit/module.ts

@@ -455,19 +455,6 @@ export function transitiveScopesFor<T>(moduleType: Type<T>): NgModuleTransitiveS
     },
   };
 
-  maybeUnwrapFn(def.declarations).forEach(declared => {
-    const declaredWithDefs = declared as Type<any>& { ɵpipe?: any; };
-
-    if (getPipeDef(declaredWithDefs)) {
-      scopes.compilation.pipes.add(declared);
-    } else {
-      // Either declared has a ɵcmp or ɵdir, or it's a component which hasn't
-      // had its template compiled yet. In either case, it gets added to the compilation's
-      // directives.
-      scopes.compilation.directives.add(declared);
-    }
-  });
-
   maybeUnwrapFn(def.imports).forEach(<I>(imported: Type<I>) => {
     const importedType = imported as Type<I>& {
       // If imported is an @NgModule:
@@ -485,6 +472,19 @@ export function transitiveScopesFor<T>(moduleType: Type<T>): NgModuleTransitiveS
     importedScope.exported.pipes.forEach(entry => scopes.compilation.pipes.add(entry));
   });
 
+  maybeUnwrapFn(def.declarations).forEach(declared => {
+    const declaredWithDefs = declared as Type<any>& { ɵpipe?: any; };
+
+    if (getPipeDef(declaredWithDefs)) {
+      scopes.compilation.pipes.add(declared);
+    } else {
+      // Either declared has a ɵcmp or ɵdir, or it's a component which hasn't
+      // had its template compiled yet. In either case, it gets added to the compilation's
+      // directives.
+      scopes.compilation.directives.add(declared);
+    }
+  });
+
   maybeUnwrapFn(def.exports).forEach(<E>(exported: Type<E>) => {
     const exportedType = exported as Type<E>& {
       // Components, Directives, NgModules, and Pipes can all be exported.

packages/core/test/acceptance/directive_spec.ts

@@ -7,7 +7,7 @@
  */
 
 import {CommonModule} from '@angular/common';
-import {Component, Directive, ElementRef, EventEmitter, Output, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core';
+import {Component, Directive, ElementRef, EventEmitter, NgModule, Output, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core';
 import {Input} from '@angular/core/src/metadata';
 import {TestBed} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
@@ -633,4 +633,96 @@ describe('directives', () => {
          expect(div.getAttribute('title')).toBe('a');
        });
   });
+
+  describe('directives with the same selector', () => {
+    it('should process Directives from `declarations` list after imported ones', () => {
+      const log: string[] = [];
+      @Directive({selector: '[dir]'})
+      class DirectiveA {
+        constructor() { log.push('DirectiveA.constructor'); }
+        ngOnInit() { log.push('DirectiveA.ngOnInit'); }
+      }
+
+      @NgModule({
+        declarations: [DirectiveA],
+        exports: [DirectiveA],
+      })
+      class ModuleA {
+      }
+
+      @Directive({selector: '[dir]'})
+      class DirectiveB {
+        constructor() { log.push('DirectiveB.constructor'); }
+        ngOnInit() { log.push('DirectiveB.ngOnInit'); }
+      }
+
+      @Component({
+        selector: 'app',
+        template: '<div dir></div>',
+      })
+      class App {
+      }
+
+      TestBed.configureTestingModule({
+        imports: [ModuleA],
+        declarations: [DirectiveB, App],
+      });
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+
+      expect(log).toEqual([
+        'DirectiveA.constructor', 'DirectiveB.constructor', 'DirectiveA.ngOnInit',
+        'DirectiveB.ngOnInit'
+      ]);
+    });
+
+    it('should respect imported module order', () => {
+      const log: string[] = [];
+      @Directive({selector: '[dir]'})
+      class DirectiveA {
+        constructor() { log.push('DirectiveA.constructor'); }
+        ngOnInit() { log.push('DirectiveA.ngOnInit'); }
+      }
+
+      @NgModule({
+        declarations: [DirectiveA],
+        exports: [DirectiveA],
+      })
+      class ModuleA {
+      }
+
+      @Directive({selector: '[dir]'})
+      class DirectiveB {
+        constructor() { log.push('DirectiveB.constructor'); }
+        ngOnInit() { log.push('DirectiveB.ngOnInit'); }
+      }
+
+      @NgModule({
+        declarations: [DirectiveB],
+        exports: [DirectiveB],
+      })
+      class ModuleB {
+      }
+
+      @Component({
+        selector: 'app',
+        template: '<div dir></div>',
+      })
+      class App {
+      }
+
+      TestBed.configureTestingModule({
+        imports: [ModuleA, ModuleB],
+        declarations: [App],
+      });
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+
+      expect(log).toEqual([
+        'DirectiveA.constructor', 'DirectiveB.constructor', 'DirectiveA.ngOnInit',
+        'DirectiveB.ngOnInit'
+      ]);
+    });
+
+  });
 });

packages/core/test/acceptance/pipe_spec.ts

@@ -110,6 +110,86 @@ describe('pipe', () => {
     expect(fixture.nativeElement.textContent).toEqual('value a b default 0 1 2 3');
   });
 
+  it('should pick a Pipe defined in `declarations` over imported Pipes', () => {
+    @Pipe({name: 'number'})
+    class PipeA implements PipeTransform {
+      transform(value: any) { return `PipeA: ${value}`; }
+    }
+
+    @NgModule({
+      declarations: [PipeA],
+      exports: [PipeA],
+    })
+    class ModuleA {
+    }
+
+    @Pipe({name: 'number'})
+    class PipeB implements PipeTransform {
+      transform(value: any) { return `PipeB: ${value}`; }
+    }
+
+    @Component({
+      selector: 'app',
+      template: '{{ count | number }}',
+    })
+    class App {
+      count = 10;
+    }
+
+    TestBed.configureTestingModule({
+      imports: [ModuleA],
+      declarations: [PipeB, App],
+    });
+    const fixture = TestBed.createComponent(App);
+    fixture.detectChanges();
+
+    expect(fixture.nativeElement.textContent).toBe('PipeB: 10');
+  });
+
+  it('should respect imported module order when selecting Pipe (last imported Pipe is used)',
+     () => {
+       @Pipe({name: 'number'})
+       class PipeA implements PipeTransform {
+         transform(value: any) { return `PipeA: ${value}`; }
+       }
+
+       @NgModule({
+         declarations: [PipeA],
+         exports: [PipeA],
+       })
+       class ModuleA {
+       }
+
+       @Pipe({name: 'number'})
+       class PipeB implements PipeTransform {
+         transform(value: any) { return `PipeB: ${value}`; }
+       }
+
+       @NgModule({
+         declarations: [PipeB],
+         exports: [PipeB],
+       })
+       class ModuleB {
+       }
+
+       @Component({
+         selector: 'app',
+         template: '{{ count | number }}',
+       })
+       class App {
+         count = 10;
+       }
+
+       TestBed.configureTestingModule({
+         imports: [ModuleA, ModuleB],
+         declarations: [App],
+       });
+       const fixture = TestBed.createComponent(App);
+       fixture.detectChanges();
+
+       expect(fixture.nativeElement.textContent).toBe('PipeB: 10');
+     });
+
   it('should do nothing when no change', () => {
     let calls: any[] = [];