Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ivy): validate the NgModule declarations field #34404

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Dec 13, 2019

This commit adds three previously missing validations to
NgModule.declarations:

  1. It checks that declared classes are actually within the current
    compilation.

  2. It checks that declared classes are directives, components, or pipes.

  3. It checks that classes are declared in at most one NgModule.

@alxhub alxhub requested review from angular/fw-compiler as code owners Dec 13, 2019
@googlebot googlebot added the cla: yes label Dec 13, 2019
@alxhub alxhub requested a review from JoostK Dec 13, 2019
// This is an error: declaring something outside of the current compilation.
// If no better position for the error can be found, the `declarations` array from the
// NgModule is used.
let errorNode: ts.Expression = rawDeclarations;

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 14, 2019

Member

I find the current way of assuming the whole array upfront and then narrowing down to a more specific identifier a bit confusing, as compared to the other way around:

let errorNode: ts.Expression|null = ref.getIdentityInExpression(rawDeclarations);
if (errorNode === null) {
  errorNode = rawDeclarations;
}

Additionally, consider extracting this out into a function?

export function determineReferenceOrigin(ref: Reference, container: ts.Expression): ts.Expression {
  const identity = ref.getIdentityInExpression(container);
  if (identity === null) {
    return container;
  }
  return identity;
}

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 17, 2019

Author Contributor

Good idea, done :)

duplicateDeclMap.set(firstDeclData.ngModule, firstDeclData);
this.duplicateDeclarations.set(decl.node, duplicateDeclMap);

// Remove the directive/pipe from `declarationToModule`.

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 14, 2019

Member

I would include in the comment that we're removing it from declarationToModule because the declaration is invalid.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 17, 2019

Author Contributor

Good point - done!

packages/compiler-cli/src/ngtsc/scope/src/local.ts Outdated Show resolved Hide resolved
contextNode = id;
}
context.push({

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 14, 2019

Member

This must be one block higher.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 16, 2019

Author Contributor

Actually no, our intention is to skip declarations which don't have rawDeclarations entirely. This is mainly .d.ts files, for which showing an error would be pointless.

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 16, 2019

Member

In that case, the code is misleading :-) Moving contextNode within the if (decl.rawDeclarations) block should make this far more clear.

export class Module {}
`);

env.driveMain();

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 14, 2019

Member

I don't think this test is currently passing? This should produce a diagnostic, so driveMain throws causing the test to fail.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 17, 2019

Author Contributor

Good point :) yes, it's currently failing. Fixed.

@@ -192,3 +261,14 @@ runInEachFileSystem(() => {
return node as T;
}
});

function nodeToClass(node: ts.Node): ts.ClassDeclaration {

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 14, 2019

Member

nit:

Suggested change
function nodeToClass(node: ts.Node): ts.ClassDeclaration {
function findContainingClass(node: ts.Node): ts.ClassDeclaration {

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 17, 2019

Author Contributor

👍

expect(diags[0].messageText).toContain(`not a part of the current compilation`);
});

it('should detect when a declaration is shared between two modules', () => {

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 14, 2019

Member

Can you add a test for these two cases:

  1. a declaration occurs twice in a single NgModule.
  2. a declaration occurs twice in two NgModules, which should then have only two related infos for the two NgModules (not for each declaration, which would have resulted in either 4—or possibly 3—related info messages)

The code is specifically set up to deal with those cases and a test would help to show that.

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 17, 2019

Author Contributor

Good call! Done.

@alxhub alxhub force-pushed the alxhub:ngtsc/declaration-diagnostics branch from 9035151 to 703ab4e Dec 16, 2019
@JoostK
JoostK approved these changes Dec 16, 2019
Copy link
Member

JoostK left a comment

Thanks for the updates. CI is unhappy but otherwise LGTM!

This commit adds three previously missing validations to
NgModule.declarations:

1. It checks that declared classes are actually within the current
   compilation.

2. It checks that declared classes are directives, components, or pipes.

3. It checks that classes are declared in at most one NgModule.
@alxhub alxhub force-pushed the alxhub:ngtsc/declaration-diagnostics branch from 703ab4e to ce74f66 Dec 16, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 17, 2019
@kara kara closed this in 763f8d4 Dec 17, 2019
kara added a commit that referenced this pull request Dec 17, 2019
This commit adds three previously missing validations to
NgModule.declarations:

1. It checks that declared classes are actually within the current
   compilation.

2. It checks that declared classes are directives, components, or pipes.

3. It checks that classes are declared in at most one NgModule.

PR Close #34404
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 17, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.