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

feat(core): add undecorated classes migration schematic #31650

Closed
wants to merge 2 commits into from

Conversation

@devversion
Copy link
Member

commented Jul 19, 2019

  • See individual commits.

Regarding google3 tslint driver. I'm still worried about performance here. TSLint is generally pretty slow, doesn't have a global analysis phase etc. read more why tslint is bad for replacements here (bullet points).

It's especially bad now because the rule for this migration re-creates another program for each source file! (significant memory pressure; so much overhead).. We need to create another program because we need the AngularCompilerProgram to read/parse and simplify AOT metadata and summaries.

We could play with caching the program to reduce memory pressure.. but ideally we don't run the migration with tslint in google3.. or we just wait and hope that it goes through.. It might be worth exploring the upgrade tool we built for the Angular CDK migrations (which solves most of the issues above).

@ngbot ngbot bot added this to the needsTriage milestone Jul 19, 2019

@googlebot googlebot added the cla: yes label Jul 19, 2019

@devversion devversion changed the title feat(core): add undecorated-base-class migration schematic feat(core): add undecorated classes migration schematic Jul 19, 2019

@devversion devversion force-pushed the devversion:wip-base-classes branch 2 times, most recently from 99acb68 to 9631211 Jul 19, 2019

@devversion devversion marked this pull request as ready for review Jul 19, 2019

@devversion devversion requested a review from angular/fw-core as a code owner Jul 19, 2019

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

We now have a MapReduce over g3 that brings up an ngc ng.Program for every compilation. Just used it to list all Directives in g3. I'll patch this over on Monday to find all violations and count them.

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

I wired it up directly in tsunami, so you can remove any google3-specific or tslint bits from this PR.

An ngTsunami analyzer is a program which gets a tsconfig path as its only argument.
It can import a helper function createCompilationUnit either returns an ng.Program or null (if it's not an Angular compilation unit for example)
It is expected to pass a protocol buffer result to an emitAnalyzerResults function (it happens to write them to stdout for the wrapping mapreduce binary to get them)

To start with I just want to find all the violations in google3, like tslint without --fix, so I just encode the map of relativeFilePath -> {line:column} in the appropriate protocol buffer.

It's not working for all inputs though, there's a crash:

 stderr: failed to load main  Error: Unexpected value in spread expression: [object Object]
E0720 14:57:20.751639 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitSpreadElement (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:526:19)
E0720 14:57:20.751816 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitArrayLiteralExpression (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:136:63)
E0720 14:57:20.751944 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitExpression (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:106:27)
E0720 14:57:20.752108 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitVariableDeclaration (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:256:25)
E0720 14:57:20.752249 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitDeclaration (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:235:25)
E0720 14:57:20.752387 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitIdentifier (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:213:27)
E0720 14:57:20.752516 105917 wavelet.go:97 stderr:     at StaticInterpreter.visitExpression (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:88:27)
E0720 14:57:20.752613 105917 wavelet.go:97 stderr:     at StaticInterpreter.visit (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.js:59:21)
E0720 14:57:20.752839 105917 wavelet.go:97 stderr:     at PartialEvaluator.evaluate (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interface.js:19:28)
E0720 14:57:20.753044 105917 wavelet.go:97 stderr:     at NgDirectiveCollector._visitNgModuleDecorator (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/core/schematics/migrations/undecorated-classes/directive_collector.js:75:44)
E0720 14:57:20.753188 105917 wavelet.go:97 stderr:     at NgDirectiveCollector._visitClassDeclaration (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/core/schematics/migrations/undecorated-classes/directive_collector.js:48:18)
E0720 14:57:20.753380 105917 wavelet.go:97 stderr:     at NgDirectiveCollector.visitNode (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/core/schematics/migrations/undecorated-classes/directive_collector.js:31:18)
E0720 14:57:20.753563 105917 wavelet.go:97 stderr:     at /export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/core/schematics/migrations/undecorated-classes/directive_collector.js:33:59
E0720 14:57:20.753692 105917 wavelet.go:97 stderr:     at visitNodes (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/node_modules/typescript/stable/lib/typescript.js:16521:30)
E0720 14:57:20.753834 105917 wavelet.go:97 stderr:     at Object.forEachChild (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/node_modules/typescript/stable/lib/typescript.js:16749:24)
E0720 14:57:20.754027 105917 wavelet.go:97 stderr:     at NgDirectiveCollector.visitNode (/export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/third_party/javascript/angular2/rc/packages/core/schematics/migrations/undecorated-classes/directive_collector.js:33:12)
E0720 14:57:20.754174 105917 wavelet.go:97 stderr:     at /export/hda3/borglet/local_ram_fs_dirs/794.mapreduce-tsunami-mapper-0.typescript.16432097079.28790bf39cfcc7a3/analyzer_pkg/google3/javascript/typescript/tsunami/analyzers/angular/undecorated.js:43:79

The spread operator appears in this code, hope this is enough to repro:

import {allComponents as sharedComponents} from 'google3/.../ng2/shared';
export type AngularComponent = Type<any>;
export const COMPONENTS: AngularComponent[] = [
  ConditionalLink,
  ExampleNg2Component,
  MatIcon,
  ...sharedComponents,
];

the .../ng2/shared is another ng_module compilation unit.

There's a second case of the same error, code looks like

import * as Dash2Util from 'google3/.../ng1_util';
@NgModule({
  imports: [
    BrowserAnimationsModule,
    Dash2Util.Dash2Module,
  ],
  entryComponents: [
    ...Dash2Util.DASH2_ENTRY_COMPONENTS,
  ],
})
export class Ng2AppModule {
  ngDoBootstrap() {}
}

In both cases I think ngUpgrade is in use.

I'll re-run the MR with some extra error-handling so we can find out if these are all the crashes.

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

Also FWIW, 3 out of 3337 shards of g3 experienced the crash, so we're not losing much data due to this currently.

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

okay with those three shards excluded, I have the code locations violating the check
https://alexeagle.users.x20web.corp.google.com/ngc_tsunami.html
(googlers only sorry)
which lists a lot of violations in .d.ts files, not in the .ts file where the source lives. Maybe we need to make use of the sourcemapping to emit .ts locations instead?

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

It triggers in this case which seems wrong

import {Pipe, PipeTransform} from '@angular/core';
@Pipe({name: 'timestamp'})
export class Timestamp implements PipeTransform {
}
@devversion

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

@alexeagle Thanks for spending time on this! Finding violations first sounds reasonable to me.

so I just encode the map of relativeFilePath -> {line:column} in the appropriate protocol buffer.

The returned transform failures are only for instances where the migration could not be performed automatically. Successful replacements are stored in the update recorders. So right now I think that your MR test-run just showed cases where the migration failed to perform.

which lists a lot of violations in .d.ts files, not in the .ts file where the source lives.

Hmm. The transform should actually never create failures for TS nodes outside of program source files. What are the violation messages for such instances?.

@devversion devversion force-pushed the devversion:wip-base-classes branch from 9631211 to ba0c418 Jul 21, 2019

@devversion

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

@alexeagle Removed the tslint driver bits in favor of the tsunami driver. Also I fixed the @Pipe case you mentioned. It looks like this one wasn't part of the migration plan but it seems reasonable to also migrate undecorated pipes that inherit metadata (case 2).

For case 1 where the pipe is decorated but inherits the constructor from undecorated pipes, we'd need to figure out what to do. cc. @alxhub. do we need an abstract @Pipe() as well?

@devversion devversion force-pushed the devversion:wip-base-classes branch from ba0c418 to 9116aeb Jul 21, 2019

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Probably shouldn't add a class comment if there is already one:
image

@devversion

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

I think we should always omit the leading/trailing comment for the copied decorator. Comments that are associated with a decorator are most of the time not intended to target the decorator, but rather the class.

@devversion devversion force-pushed the devversion:wip-base-classes branch 4 times, most recently from cf2a823 to 7e8391f Jul 22, 2019

@devversion

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

The PR is currently blocked on a components build issue that has been unveiled by these changes. I'll rebase this PR on top of angular/components#16758 once it landed.

@devversion devversion force-pushed the devversion:wip-base-classes branch from bb1371d to 9277c82 Aug 12, 2019

@devversion devversion force-pushed the devversion:wip-base-classes branch 2 times, most recently from 3ad8430 to e3c686a Aug 13, 2019

@ngbot

This comment was marked as resolved.

Copy link

commented Aug 13, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: lint" is failing
    pending status "ci/angular: size" is pending
    pending status "ci/circleci: integration_test" is pending
    pending status "ci/circleci: legacy-misc-tests" is pending
    pending status "ci/circleci: material-unit-tests" is pending
    pending status "ci/circleci: test" is pending
    pending status "ci/circleci: test_aio" is pending
    pending status "ci/circleci: test_aio_local" is pending
    pending status "ci/circleci: test_aio_local_ivy" is pending
    pending status "ci/circleci: test_aio_tools" is pending
    pending status "ci/circleci: test_docs_examples" is pending
    pending status "ci/circleci: test_docs_examples_ivy" is pending
    pending status "ci/circleci: test_ivy_aot" is pending
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"
    pending missing required status "cla/google"

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.

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Caretaker note (to self): needs a G3 patch

devversion added 2 commits Jul 24, 2019
feat(core): add undecorated classes migration schematic
Introduces a new migration schematic that follows the given
migration plan: https://hackmd.io/@alx/S1XKqMZeS.

First case: The schematic detects decorated directives which
inherit a constructor. The migration ensures that all base
classes until the class with the explicit constructor are
properly decorated with "@directive()" or "@component". In
case one of these classes is not decorated, the schematic
adds the abstract "@directive()" decorator automatically.

Second case: The schematic detects undecorated declarations
and copies the inherited "@directive()", "@component" or
"@pipe" decorator to the undecorated derived class. This
involves non-trivial import rewriting, identifier aliasing
and AOT metadata serializing
(as decorators are not always part of source files)
ci: update material-unit-tests job to latest commit
Updates the `material-unit-tests` job to the latest commit
on the components repository. 097f4335a4e0b6e6b579829ae3a9cffce6292d2b.

This commit ensures that the postinstall script does not run NGC
on schematic code from `@angular/core`. Running NGC on the
generated schematic code can cause unexpected issues as some
migrations import types directly from `@angular/compiler-cli`
while the entry-point is not usable in all cases.

See: #29220.

@devversion devversion force-pushed the devversion:wip-base-classes branch from e3c686a to 7166d1e Aug 13, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

merge-assistance: global

@kara kara closed this in 024c31d Aug 13, 2019

kara added a commit that referenced this pull request Aug 13, 2019
ci: update material-unit-tests job to latest commit (#31650)
Updates the `material-unit-tests` job to the latest commit
on the components repository. 097f4335a4e0b6e6b579829ae3a9cffce6292d2b.

This commit ensures that the postinstall script does not run NGC
on schematic code from `@angular/core`. Running NGC on the
generated schematic code can cause unexpected issues as some
migrations import types directly from `@angular/compiler-cli`
while the entry-point is not usable in all cases.

See: #29220.

PR Close #31650
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(core): add undecorated classes migration schematic (angular#31650)
Introduces a new migration schematic that follows the given
migration plan: https://hackmd.io/@alx/S1XKqMZeS.

First case: The schematic detects decorated directives which
inherit a constructor. The migration ensures that all base
classes until the class with the explicit constructor are
properly decorated with "@directive()" or "@component". In
case one of these classes is not decorated, the schematic
adds the abstract "@directive()" decorator automatically.

Second case: The schematic detects undecorated declarations
and copies the inherited "@directive()", "@component" or
"@pipe" decorator to the undecorated derived class. This
involves non-trivial import rewriting, identifier aliasing
and AOT metadata serializing
(as decorators are not always part of source files)

PR Close angular#31650
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
ci: update material-unit-tests job to latest commit (angular#31650)
Updates the `material-unit-tests` job to the latest commit
on the components repository. 097f4335a4e0b6e6b579829ae3a9cffce6292d2b.

This commit ensures that the postinstall script does not run NGC
on schematic code from `@angular/core`. Running NGC on the
generated schematic code can cause unexpected issues as some
migrations import types directly from `@angular/compiler-cli`
while the entry-point is not usable in all cases.

See: angular#29220.

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

This comment has been minimized.

Copy link

commented Sep 15, 2019

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 Sep 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.