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

Allow `allowJs` and `declaration` to be used together #32372

Merged
merged 60 commits into from Sep 26, 2019

Conversation

@weswigham
Copy link
Member

commented Jul 12, 2019

Which, in turn, should enable incremental builds and project-based builds for projects containing .js and .json files.

This introduces a new symbol-based declaration emitter (meaning it's highly coupled to the checker and is indeed part of the node builder) - currently this is only used for JSON and JavaScript, as the output is likely worse than what the other declaration emitter is capable of (minimally, it'll likely never respect input declaration order). In addition, it is still incomplete - it does not yet support serializing namespaces.

Things yet to do:

  • Support for serializing namespaces (even though they can't appear in a non-TS file without error, I'd like this emitter to be complete - I've already added enum and interface support)
  • Related - test javascript @enum tag declaration emit - it's not a real enum, but sorta is - I have no idea what it's symbol looks like (@sandersn I'm more than willing to take some suggestions for esoteric jsdoc type/symbol structure edge cases).
  • More/better tests computed names and various import/export aliases
  • More/better tests for Object.defineProperty and nested exports.whatever.whatever = declarations
  • More/better tests for interface/class/namespaces merges (really only applies once namespace support is in)
  • Call setOriginalNode where possible (ie, when an original declaration exists) throughout the serializer on the names/declarations of the manufactured nodes, in order to make declaration maps (at all) accurate
  • Do a secondary "cleanup" pass on the produced declaration file that just does some make-the-file-look-pretty transforms, like merging export declarations and import declarations.
    • Flatten and merge exports
    • Sort statements (imports > privates > exports > export assignments, alphabetical within each, where possible)
    • Hoist out import types & merge imports
  • Relevant incremental/project reference tests (cc @sheetalkamat what do we want to see here?)
  • Add visibility errors when unserializable private names are used, rather than silently printing them
  • Reuse existing type annotations where possible to avoid using unserializable private names
    • Refuse to reuse node trees containing references which do not resolve under TS (not JS) resolution rules, ie const Cls = require('./class'); /** @type {Cls} */const x = .... (We can't use Cls here because it was fetched thru a value and not an import alias)

Still, unfinished as it is, this is probably able to be used in most scenarios already, so getting some implementation feedback (and output feedback) would be welcome.

Fixes #7546

cc @DanielRosenwasser you dumped this into the 3.6 release, but never assigned it to anyone - so I started on it at the start of the week. Think we could squeeze it into the beta?

@sandersn sandersn self-assigned this Jul 12, 2019
@weswigham weswigham force-pushed the weswigham:symbolic-declaration-files branch from c003de7 to be85d75 Jul 12, 2019
@brendankenny

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

very exciting!

so getting some implementation feedback (and output feedback) would be welcome.

hopefully you meant this :)

Running it against lighthouse (adding declaration, removing noEmit, and adding emitDeclarationOnly to our tsconfig), the output seems reasonable, but I only get three d.ts files before it hits an Error: Debug Failure. Unhandled class member kind! 32.

I might be holding it wrong, though, I've never actually used --declaration.

JSDoc comments are also not preserved on the signatures. Will that be possible?

Copy link
Member

left a comment

Some initial comments. Nothing too useful yet, just observations.

Still need to look at the tests and the meat of the code.

src/compiler/utilities.ts Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@weswigham

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

JSDoc comments are also not preserved on the signatures. Will that be possible?

Should be - the work item in the OP to do with "using setOriginalNode where possible" should cover that.

Running it against lighthouse (adding declaration, removing noEmit, and adding emitDeclarationOnly to our tsconfig), the output seems reasonable, but I only get three d.ts files before it hits an Error: Debug Failure. Unhandled class member kind! 32.

Hoho, thanks for the already setup real-world codebase. I had some bugs with base class serialization which are now fixed, so one of those assertions is gone, and the other shouldn't be possible to trigger so long as you can't write namespace Cls { interface X {} } class Cls {} in JS (plus a small bug exposed in the normal declaration emitter to do with confusion of a type as a type parameter).

In any case, lighthouse no longer crashes with this PR and those settings, which is good, however we do produce some invalid declaration files 😦 . I'll need to look into which symbol patterns produce the invalid output and patch up their emit. Looks like @typedef's in a file with a module.exports = {...} is a big offender, which makes sense - the way this works at all is very strange within the checker.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Part of the idea was to have infrastructure in place to fuzz-test and ensure that we don't just crash on random .js files. It seems like we're already running into some and producing incorrect results, so I think if anything we can put this into typescript@experimental`.

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2019

I think if anything we can put this into typescript@experimental

To what end? Do we have partners willing to provide the same kind of and quality of feedback we'd get from the month long beta period we now have? I though the entire point of a beta is that it can be semistable and have features that may not make the final release cut (because they didn't stabilize enough over the beta timeframe) - it's not an RC, after all; a beta's not meant to be in a state where it's promotable to a full release.

Copy link
Member

left a comment

More comments, up through getinternalSymbolName

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

@typescript-bot pack this

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

@typescript-bot pack this pls - where you at

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

Heya @weswigham, I've started to run the tarball bundle task on this PR at fd9648a. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/36910/artifacts?artifactName=tgz&fileId=BF842F98B0BE069E93F283BACE70B6730B39DC4117B82B30B115803058C7555202&fileName=/typescript-3.6.0-insiders.20190718.tgz"
    }
}

and then running npm install.

@matthewmueller

This comment has been minimized.

Copy link

commented Jul 24, 2019

Works pretty well! I'm seeing a couple issues:

1) error in tsconfig.json that I just want to bring up to make sure it's covered:

Screen Shot 2019-07-24 at 1 11 18 PM

This might just be due to my vscode's typescript being different than the project's typescript

2) It doesn't seem to understand the following type definitions

/**
 * Imports
 *
 * @typedef {import("../timer")} Timer
 * @typedef {import("../hook")} Hook
 * @typedef {import("../hook").HookHandler} HookHandler
 */

/**
 * Input type definition
 *
 * @typedef {Object} Input
 * @prop {Timer} timer
 * @prop {Hook} hook
 */

/**
 * New `Context`
 *
 * @class
 * @param {Input} input
 */

function Context(input) {
  if (!(this instanceof Context)) {
    return new Context(input)
  }
  this.state = this.construct(input)
}

Where I'm importing one typedef from another file into this file. In this case I see:

Screen Shot 2019-07-24 at 1 22 33 PM

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

What's the shape of Hook (input and output) And what TS version is your IDE using? I likewise had to make declaration parsing a taaaad more permissive in some ways to represent some JS patterns, so there's the possibility that something isn't an error with this PR but is without it.

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

Oooo, and that's a JS-style psuedoclass - I haven't even tested those yet, but it definitely seems like their output is a bit wonky. Will have to work on that. Afaik the only way to emit them into a declaration file is with a type alias declaring both call and construct signatures.

@matthewmueller

This comment has been minimized.

Copy link

commented Jul 24, 2019

Sorry for an incomplete example! Hook contains the following:

../hook.js

/**
 * Imports
 *
 * @typedef {import("../reporters").Reporter} Reporter
 * @typedef {import("../context")} Context
 * @typedef {import("../timer")} Timer
 */

/**
 * Hook handler
 *
 * @typedef {(t: Context) => void|Promise<void>} HookHandler
 */

/**
 * Input type definition
 *
 * @typedef {Object} Input
 * @prop {Reporter} reporter
 * @prop {string} filename
 * @prop {string} title
 * @prop {string} cwd
 * @prop {Timer} timer
 */

/**
 * New `Hook`
 *
 * @class
 * @param {Input} input
 */

function Hook(input) {
  if (!(this instanceof Hook)) {
    return new Hook(input)
  }
}

/**
 * Exports
 */

module.exports = Hook

I should mention, everything works great from a type-checking perspective. Just when you generate typedefs, I'm seeing errors.

And I'm using Typescript 3.5.2 in VScode. I've started using the workspace version 3.6.0-insiders.20190708 and the tsconfig.json seems to work :-)

Lastly, I should mention that this unfinished PR alone is already super helpful. I wasn't really sure how to write typedefs and just having example outputs allows me to fix things by hand. Thanks for your hard work so far!


FWIW, I just needed to make 3 fixes to make to the example above work (with the red squiggles):

  1. import(...) with a old-school constructor and @class needs to be typeof import(...)
  2. Types (State, Input, etc.) need to be placed inside namespace test { }
  3. References to these types need to be changed from State to test.State

Update

Change (1.) turned out to be wrong, I needed to turn it into a class declare class Hook in the definition file, then it does work with import("./hook")

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

OK, yup, definitely looks like it comes down to not serializing the types of JS functions-as-classes yet. It'll be one of the next things I look into - how they work internally is all a bit ad-hoc, so recognizing them and serializing them in a compatible way will be... interesting.

@matthewmueller

This comment has been minimized.

Copy link

commented Jul 24, 2019

Awesome! Let me know if I can help in any other way. One last thing I ran into and it might just be the odd way I'm doing things. This is not a bug, but more of a possible UX improvement.

I use the following to make a local reference to typedefs so I don't have to constantly import this type over and over in the code.

/**
 * @typedef {import("./context")} Context
 */

Currently for the JS functions-as-classes exported with module.exports = Context correctly uses imports. The generated index.d.ts has the following:

type Context = import('./context')

But for other typedefs, it actually inlines the whole type. Sometimes these types are huge so it's probably not what you want.

reporter.js

/**
 * Reporter type definition
 *
 * @typedef {Object} Reporter
 * @prop {(event: Event) => Promise<void>} report
 * @prop {() => Promise<void>} flush
 * @prop {() => Promise<void>} close
 */

context.js

/**
 * @typedef {import("./reporter").Reporter} Reporter
 */

Results in context.d.ts with a full reporter type definition:

type Reporter = {
  report: (event: Event) => Promise<void>
  flush: () => Promise<void>
  close: () => Promise<void>
}

Where I'd expect:

type Reporter = import("./report").Reporter

Once again, really impressed with how far along this is already.

@sandersn

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@weswigham I think it might be worthwhile to first make constructor functions into real classes. Assuming that they fit into the class mold neatly enough, it should cut the amount of work for this PR.

@matthewmueller

This comment has been minimized.

Copy link

commented Jul 26, 2019

Agreed if that's possible. I'm finding declare class X to be the cleanest way to handwrite definitions for @class.

@daKmoR

This comment has been minimized.

Copy link

commented Aug 1, 2019

@typescript-bot the above-mentioned tgz seem to be no longer available? Could you provide a new version? pretty please 🤗

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

I can @typescript-bot pack this for your testing yes, but this is currently blocked on #32584 to fix the reported issue with ES5-y class/functions.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

Heya @weswigham, I've started to run the tarball bundle task on this PR at eb4a036. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/38490/artifacts?artifactName=tgz&fileId=B44B1B9D6D3E1494E9E4D61354B7758F8C6EE6DD0CEA7A4304ABDBD5DF6F7FF002&fileName=/typescript-3.6.0-insiders.20190801.tgz"
    }
}

and then running npm install.

@daKmoR

This comment has been minimized.

Copy link

commented Aug 1, 2019

@weswigham awesome thx 🤗

needed to try it immediately 💪
you can see the result here open-wc/open-wc#679 👍

really promising 🥇 and almost good enough for us... if not be for 1 thing

  • exports are invalid 😭 (missing the from '../path/to/file.js' part)

a nice to have:

  • jsDoc gets lost 😢 (e.g. docu would be nicer with it)
@weswigham weswigham force-pushed the weswigham:symbolic-declaration-files branch 2 times, most recently from 3898322 to cb1e433 Aug 2, 2019
@weswigham

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

@daKmoR just 4 u: @typescript-bot pack this again 😉

weswigham added 2 commits Sep 25, 2019
Copy link
Member

left a comment

Another batch of comments because the page is getting suuuuuper flaky about adding comments.


function cloneNodeBuilderContext(context: NodeBuilderContext): NodeBuilderContext {
const initial: NodeBuilderContext = { ...context };
// Make type parameters created within this context not consume the name outside this context

This comment has been minimized.

Copy link
@sandersn

sandersn Sep 25, 2019

Member

'consume' isn't defined here, but it would be easier to read with a definition.

This comment has been minimized.

Copy link
@weswigham

weswigham Sep 25, 2019

Author Member

There's an exact example of the scenario written just below in the same comment block?

src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
}

function mergeRedundantStatements(statements: Statement[]) {
const exportAssignment = find(statements, isExportAssignment);

This comment has been minimized.

Copy link
@sandersn

sandersn Sep 25, 2019

Member

this function iterates and re-allocates statements lots and lots of times. Probably fine for an initial version, but I think we'll need a smarter structure when we want large files to be efficient.

This comment has been minimized.

Copy link
@weswigham

weswigham Sep 25, 2019

Author Member

This function is only called once per file/namespace, so it's probably fine (given that the alternative - tracking masks on top of an immutable buffer - is super complicated, and if we were going to do that, I imagine we'd do it in other transformers first...).

src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
weswigham added 4 commits Sep 25, 2019
Copy link
Member

left a comment

OK, I've looked at everything so far.
Overall, I think we should take this change for the beta. I hand a bunch of small questions but I'm pretty sure they are all covered by some test case.

Beforehand, I'd like to see a perf run on some large code base -- basically just run tsc --diagnostics 20 times and average the emit time (if this counts toward emit time).
Afterward, I hope we can get to the TODOs, clean up the performance and style of the code, and address any more problems people run into.

src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
/*decorators*/ undefined,
createModifiersFromModifierFlags((isReadonlySymbol(p) ? ModifierFlags.Readonly : 0) | staticFlag),
name,

This comment has been minimized.

Copy link
@sandersn

sandersn Sep 25, 2019

Member

why do type parameters get special treatment?

This comment has been minimized.

Copy link
@weswigham

weswigham Sep 26, 2019

Author Member

again, without context, no idea what this is referring to (but, broadly: type params in the node builder usually get special treatment because they usually need a generated name and are short-lived, scope-wise, only existing within a signature declaration of some kind)

src/compiler/checker.ts Outdated Show resolved Hide resolved
…ctors, and test showing bug in export assigned class expression name shadowing
@weswigham

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

Beforehand, I'd like to see a perf run on some large code base -- basically just run tsc --diagnostics 20 times and average the emit time (if this counts toward emit time).

It (and declaration emit in general) does, iirc. Finding a large project that actually can emit declarations without error is a bit hard - the larger projects usually have some visibility errors in them (private classes from other scopes fetched via functions referenced in exported variables or somesuch).

But! If the presence of errors does not deter you, the lighthouse project linked waaaaaay upthread has diagnostic data like this:

Files:           620
Lines:        217946
Nodes:        729137
Identifiers:  206153
Symbols:      460655
Types:        217832
Memory used: 477279K
I/O read:      0.11s
I/O write:     0.23s
Parse time:    1.88s
Bind time:     0.88s
Check time:    6.51s
Emit time:    13.02s
Total time:   22.30s

and that's fairly representative - over a handful of runs, the emit time hovers between 13 and 14 seconds. Previously there was no emit time (emitOnlyDeclarations is set), so that's all new declaration emit time. A quick trace shows 13% of that compilation time is spent on logic in getAccessibleSymbolChainFromSymbolTable (and it's transitively responsible for 28% of total time) - which is something I've witnessed in other projects which are heavy in inferred types that use normal declaration emit. That's not a problem with the new machinery in this PR, however - just an effect of generating declarations from all inferred types in a file, rather than just some - we now use that inefficient codepath much more. On the plus side, it means improvements to that are magnified for the js declaration emitter. The serializers themselves are comparably a tiny sliver of time spent.

@weswigham weswigham requested a review from sandersn Sep 26, 2019
@weswigham

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

For posterity: The reason getAccessibleSymbolChainFromSymbolTable is so bad is because its' fail case is searching every externally visible symbol to see if any refer to the target symbol. Every. Single. One. This is then repeated for every [symbol, enclosingDeclaration] pair (and potentially repeated for the same pair - it's uncached). TBH, the whole algorithm for that process needs to be redesigned for efficiency and hierarchical cachability - I'm thinking something along the lines of maps of which symbols are reachable from which scopes which can just be union'd as scopes nest. A kind of transitiveExports map that isn't keyed by name, but by symbol id, while also storing a bit for merged symbols and alias targets (so a set, really).

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

I'd like to not fix that in this PR, as that's also a few hundred lines of change that don't directly relate to this feature~

@sandersn

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

Thanks, the perf analysis sounds like a good starting point for post-beta work. I'll look over your recent changes+replies and sign off.

@weswigham weswigham merged commit 61cb06c into microsoft:master Sep 26, 2019
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
Details
node10 Build #45992 succeeded
Details
node12 Build #45990 succeeded
Details
node8 Build #45991 succeeded
Details
@ahocevar

This comment has been minimized.

Copy link

commented Sep 28, 2019

Thanks for the great work on this!

Regarding output feedback: with https://github.com/openlayers/openlayers (master), I do not get any output, and this exception is thrown:

/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:77718
                throw e;
                ^

TypeError: Cannot read property 'escapedText' of undefined
    at getIsContextSensitiveAssignmentOrContextType (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:43163:123)
    at getContextualTypeForBinaryOperand (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:43119:44)
    at getContextualType (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:43401:28)
    at getApparentTypeOfContextualType (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:43328:17)
    at checkObjectLiteral (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:43746:34)
    at checkExpressionWorker (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:48054:28)
    at checkExpression (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:47983:38)
    at checkBinaryLikeExpression (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:47366:29)
    at checkBinaryExpression (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:47352:20)
    at checkExpressionWorker (/Users/ahocevar/projects/openlayers/node_modules/typescript/lib/tsc.js:48096:28)

typescript package version: typescript@3.7.0-dev.20190928

Let me know if you need more details to investigate this.

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

Could you open a new issue with some details (ideally some exact repro steps)? That callstack indicates it's probably unrelated to this PR, and, like 10 things got merged in Friday. :D

@Bnaya

This comment has been minimized.

Copy link

commented Oct 3, 2019

possible related issue:
#33735

@vipcxj

This comment has been minimized.

Copy link

commented Oct 15, 2019

which version this pull affect? it seems that the latest version still not allow allowJs and declaration to be used together

@weswigham

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2019

The nightly and 3.7 beta.

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