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(ngcc): properly detect origin of constructor param types #33901

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Nov 18, 2019

The ReflectionHost supports enumeration of constructor parameters, and one
piece of information it returns describes the origin of the parameter's
type. Parameter types come in two flavors: local (the type is not imported
from anywhere) or non-local (the type comes via an import).

ngcc incorrectly classified all type parameters as 'local', because in the
source files that ngcc processes the type parameter is a real ts.Identifer.
However, that identifier may still have come from an import and thus might
be non-local.

This commit changes ngcc's ReflectionHost(s) to properly recognize and
report these non-local type references.

Fixes #33677

@alxhub alxhub requested a review from angular/fw-ngcc as a code owner Nov 18, 2019
@googlebot googlebot added the cla: yes label Nov 18, 2019
@alxhub alxhub force-pushed the alxhub:ngcc/pipe-cdr branch from 652ce12 to 4096b96 Nov 18, 2019
if (typeExpression !== null) {
// `typeExpression` is an expression in a "type" context. Resolve it to a declared value.
// Either it's a reference to an imported type, or a type declared locally. Distinguish the
// to cases with `getDeclarationOfExpression`.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 18, 2019

Member

typo: to => two

This comment has been minimized.

Copy link
@alxhub

alxhub Nov 18, 2019

Author Contributor

Nice catch!

// to cases with `getDeclarationOfExpression`.
const decl = this.getDeclarationOfExpression(typeExpression);
if (decl !== null && decl.node !== null && decl.viaModule !== null &&
isNamedDeclaration(decl.node)) {

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 18, 2019

Member

Can it be possible that viaModule !== null && !isNamedDeclaration(decl.node)?
If so, what does that mean? As this stands, we will return a "local" reference instead.

This comment has been minimized.

Copy link
@alxhub

alxhub Nov 18, 2019

Author Contributor

I don't think it's generally possible, since a declaration has to have a name to be exported in the first place. You could probably contrive some example where it's true (export default 42?) but in general I think a fallback option of "this could not be understood as a sane import, so just use the local identifier instead" is sane.

packages/compiler-cli/ngcc/test/host/umd_host_spec.ts Outdated Show resolved Hide resolved
defaultImportStatement: null
};
}
}

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 18, 2019

Member

NIT: this block of code is a candidate for a new private method, to keep the size of this method lean.

return {
  name: getNameText(nameNode),
  nameNode,
  typeValueReference: this.getTypeValueReference(typeExpression),
  typeNode: null,
  decorators,
};

This comment has been minimized.

Copy link
@alxhub

alxhub Nov 18, 2019

Author Contributor

Yeah, in general we should consider "constructor" methods for more complex types like this.

The ReflectionHost supports enumeration of constructor parameters, and one
piece of information it returns describes the origin of the parameter's
type. Parameter types come in two flavors: local (the type is not imported
from anywhere) or non-local (the type comes via an import).

ngcc incorrectly classified all type parameters as 'local', because in the
source files that ngcc processes the type parameter is a real ts.Identifer.
However, that identifier may still have come from an import and thus might
be non-local.

This commit changes ngcc's ReflectionHost(s) to properly recognize and
report these non-local type references.

Fixes #33677
@alxhub alxhub force-pushed the alxhub:ngcc/pipe-cdr branch from 4096b96 to 8e30ba8 Nov 18, 2019
@gkalpak gkalpak added the comp: ngcc label Nov 19, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 19, 2019
@alxhub alxhub closed this in 29b8666 Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 19, 2019
The ReflectionHost supports enumeration of constructor parameters, and one
piece of information it returns describes the origin of the parameter's
type. Parameter types come in two flavors: local (the type is not imported
from anywhere) or non-local (the type comes via an import).

ngcc incorrectly classified all type parameters as 'local', because in the
source files that ngcc processes the type parameter is a real ts.Identifer.
However, that identifier may still have come from an import and thus might
be non-local.

This commit changes ngcc's ReflectionHost(s) to properly recognize and
report these non-local type references.

Fixes #33677

PR Close #33901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.