refactor(ivy): correctly type class declarations#29209
refactor(ivy): correctly type class declarations#29209gkalpak wants to merge 11 commits intoangular:masterfrom
Conversation
0d88d10 to
dc19808
Compare
petebacondarwin
left a comment
There was a problem hiding this comment.
I know this is still draft but as it might solve my problem I started to look at it.
I like the idea of a new ClassDeclaration type. This kind of abstraction fits well with our general approach to hiding the internals of the TS AST. I believe we should go a bit further and use it wherever we can to hide the use of ts.ClassDeclaration.
I also think that the code would be easier to decipher if we added new alias types for InnerClassDeclaration and OuterClassDeclaration (or perhaps just let the latter be ClassDeclaration).
There was a problem hiding this comment.
We ought to be able to move all such typings from clazz: ts.Declaration to the new ClassDeclaration type, I believe.
There was a problem hiding this comment.
Yeah, I was thinking about that, but (a) wasn't sure if these methods are only called with ClassDeclaration (and didn't bother finding out 😁) and (b) I didn't want to make unnecessary changes (i.e. I only fixed the necessary signatures and then made necessary changes for typechecking to be happy).
Happy to go "all in" 😁
There was a problem hiding this comment.
Making functions like these into protected methods on the host would have two benefits:
- we can avoid having to pass the
typecheckeras a parameter since it is a member of the host. - we can override it in a future
UmdReflectionHostthat might need slightly different behaviour.
There was a problem hiding this comment.
It didn't use the TypeChecker in an earlier implementation, but totally makes sense now.
(Personally, I would turn all standalone functions into methods (or move them to a utils file if they are generic and re-usable).)
dc19808 to
5fba061
Compare
a4721f6 to
9db9df5
Compare
I briefly looked into it, but I didn't where how it would be useful. If it's an ES5 thing only, we can't use it outside of |
|
BTW, one optimization that can be done is memoizing [outer variable declaration <--> inner function declaration] pairs, so that we don't have to walk the AST in order to get from one to the other. I was going to do that, but maybe not worth it if this code is going to be refactored as part of #29119 anyway. |
9db9df5 to
cf9abf9
Compare
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
merge-assistance: The |
…duleScopeResolver`
Previously, several `ngtsc` and `ngcc` APIs dealing with class
declaration nodes used inconsistent types. For example, some methods of
the `DecoratorHandler` interface expected a `ts.Declaration` argument,
but actual `DecoratorHandler` implementations specified a stricter
`ts.ClassDeclaration` type.
As a result, the stricter methods would operate under the incorrect
assumption that their arguments were of type `ts.ClassDeclaration`,
while the actual arguments might be of different types (e.g. `ngcc`
would call them with `ts.FunctionDeclaration` or
`ts.VariableDeclaration` arguments, when compiling ES5 code).
Additionally, since we need those class declarations to be referenced in
other parts of the program, `ngtsc`/`ngcc` had to either repeatedly
check for `ts.isIdentifier(node.name)` or assume there was a `name`
identifier and use `node.name!`. While this assumption happens to be
true in the current implementation, working around type-checking is
error-prone (e.g. the assumption might stop being true in the future).
This commit fixes this by introducing a new type to be used for such
class declarations (`ts.Declaration & {name: ts.Identifier}`) and using
it consistently throughput the code.
322e6aa to
7ba59e1
Compare
|
You can preview 7ba59e1 at https://pr29209-7ba59e1.ngbuilds.io/. |
…29209) Previously, several `ngtsc` and `ngcc` APIs dealing with class declaration nodes used inconsistent types. For example, some methods of the `DecoratorHandler` interface expected a `ts.Declaration` argument, but actual `DecoratorHandler` implementations specified a stricter `ts.ClassDeclaration` type. As a result, the stricter methods would operate under the incorrect assumption that their arguments were of type `ts.ClassDeclaration`, while the actual arguments might be of different types (e.g. `ngcc` would call them with `ts.FunctionDeclaration` or `ts.VariableDeclaration` arguments, when compiling ES5 code). Additionally, since we need those class declarations to be referenced in other parts of the program, `ngtsc`/`ngcc` had to either repeatedly check for `ts.isIdentifier(node.name)` or assume there was a `name` identifier and use `node.name!`. While this assumption happens to be true in the current implementation, working around type-checking is error-prone (e.g. the assumption might stop being true in the future). This commit fixes this by introducing a new type to be used for such class declarations (`ts.Declaration & {name: ts.Identifier}`) and using it consistently throughput the code. PR Close #29209
…duleScopeResolver` (angular#29209) PR Close angular#29209
…ngular#29209) Previously, several `ngtsc` and `ngcc` APIs dealing with class declaration nodes used inconsistent types. For example, some methods of the `DecoratorHandler` interface expected a `ts.Declaration` argument, but actual `DecoratorHandler` implementations specified a stricter `ts.ClassDeclaration` type. As a result, the stricter methods would operate under the incorrect assumption that their arguments were of type `ts.ClassDeclaration`, while the actual arguments might be of different types (e.g. `ngcc` would call them with `ts.FunctionDeclaration` or `ts.VariableDeclaration` arguments, when compiling ES5 code). Additionally, since we need those class declarations to be referenced in other parts of the program, `ngtsc`/`ngcc` had to either repeatedly check for `ts.isIdentifier(node.name)` or assume there was a `name` identifier and use `node.name!`. While this assumption happens to be true in the current implementation, working around type-checking is error-prone (e.g. the assumption might stop being true in the future). This commit fixes this by introducing a new type to be used for such class declarations (`ts.Declaration & {name: ts.Identifier}`) and using it consistently throughput the code. PR Close angular#29209
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Previously, several
ngtscandngccAPIs dealing with class declaration nodes used inconsistent types. For example, some methods of theDecoratorHandlerinterface expected ats.Declarationargument, but actualDecoratorHandlerimplementations specified a stricterts.ClassDeclarationtype.As a result, the stricter methods would operate under the incorrect assumption that their arguments were of type
ts.ClassDeclaration, while the actual arguments might be of different types (e.g.ngccwould call them withts.FunctionDeclarationorts.VariableDeclarationarguments, when compiling ES5 code).Additionally, since we need those class declarations to be referenced in other parts of the program,
ngtsc/ngcchad to either repeatedly check forts.isIdentifier(node.name)or assume there was anameidentifier and usenode.name!. While this assumption happens to be true in the current implementation, working around type-checking is error-prone (e.g. the assumption might stop being true in the future).This commit fixes this by introducing a new type to be used for such class declarations (
ts.Declaration & {name: ts.Identifier}) and using it consistently throughput the code.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Jira issue: FW-1151