Ensure parameters are in scope when converting parameter/return types to type nodes #49627
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
tests/baselines/reference/jsDeclarationsParameterTagReusesInputNodeInEmit1.js
Outdated
Show resolved
Hide resolved
tests/baselines/reference/jsDeclarationsImportAliasExposedWithinNamespaceCjs.js
Outdated
Show resolved
Hide resolved
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 70dba24. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 70dba24. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 70dba24. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 70dba24. You can monitor the build here. |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey |
|
@jakebailey Here they are:Comparison Report - main..49627
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tests/baselines/reference/blockScopedVariablesUseBeforeDef.types
Outdated
Show resolved
Hide resolved
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 6714b2b. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
I haven't reviewed the code yet, but I am weirded out by the whitespace changes -- which are admittedly the same as in #37444. But why insert 3 more spaces in, say, module A {
class Point {
constructor(public x: number, public y: number) { }
}
export var UnitSquare : {
top: { left: Point, right: Point },
bottom: { left: Point, right: Point }
} = null;
} |
|
I think before it was creating an all new type node and in doing so put the code onto one line. But after, it's reusing the node, and so it has the spacing as the original source. The baselines just remove the newlines so it looks awkwardly spaced. I'd test on the above pack-this, but it looks like typescript-staging is down :( |
tests/baselines/reference/declarationEmitDistributiveConditionalWithInfer.js
Outdated
Show resolved
Hide resolved
| @@ -32,6 +32,4 @@ declare global { | |||
| } | |||
| } | |||
| //// [c.d.ts] | |||
| declare type Foo = teams.calling.Foo; | |||
| export declare const bar: (p?: Foo) => void; | |||
There was a problem hiding this comment.
This is a case that I'm pretty sure only worked because of a fluke. It's desirable, but when we get the type from the annotation, we get the interface type Foo in another file (the symbol is correctly resolved), but then this gets handed down and the info about it being an alias is completely lost. I think this only worked due to assumptions about the enclosing declaration that now no longer hold.
|
I've gotten this PR pretty far but I'm going to stop working on it for a while because it's getting too frustrating. It seems like this part of the code is really fragile when it comes to what it considers to be reusable in ways that I can't seem to keep in my head at once. If fix one thing and break something I fixed earlier, because I forgot what I did and ended up undoing it, etc. The whole enclosing declaration thing in general weirds me out; it doesn't seem consistent at all which enclosing declaration is used in what scenario, and yet the types baseline code just always uses the parent node, which would lead me to believe that it doesn't need to even exist at all, either, but I'm sure I'm missing something. |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at c215aa8. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at c215aa8. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at c215aa8. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at c215aa8. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at c215aa8. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
jakebailey
force-pushed
the
fix-48783
branch
from
bda8cd5 to
fafd048
Compare
|
Forgive the force push; this had so many commits that I wanted to still be able to see the test diffs. I've updated the logic to share the same node within the same chain instead of repeating it multiple times; I think it's about as fast as it can get. I also dropped the added property on the context; it turns out that wasn't safe because it's technically possible that another part of the code temporarily changes the enclosing declaration, but we could then accidentally inject new variables into a different chain. Now, we just walk up the ancestors to find an existing one, creating one if needed. |
| @@ -168,125 +168,3 @@ export declare class AsClassProperty { | |||
| isNaN?: typeof globalThis.isNaN; | |||
| } | |||
| export type AsFunctionType = (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN; | |||
|
|
|||
|
|
|||
| //// [DtsFileErrors] | |||
There was a problem hiding this comment.
Note the the errors going away here; I constructed these tests such that they will emit a d.ts error when they're wrong, rather than us having to figure out if the output looks right or not.
|
@typescript-bot perf test faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 27c0c92. You can monitor the build here. Update: The results are in! |
| && signature.declaration | ||
| && signature.declaration !== context.enclosingDeclaration | ||
| && !isInJSFile(signature.declaration) | ||
| && some(expandedParams) |
There was a problem hiding this comment.
getExpandedParameters looks like it always returns an array of length 1, at least unless the rest type is somehow a 0-constituent union, but I think those are always impossible.
If the check is there just because the types could allow an empty array, maybe it should be an assert in the body of the if instead.
There was a problem hiding this comment.
Check above, and you'll see:
const expandedParams = getExpandedParameters(signature, /*skipUnionExpanding*/ true)[0];There was a problem hiding this comment.
(Later I might see if we can avoid constructing a bunch of random arrays we don't need...)
|
@jakebailey Here they are:Comparison Report - main..49627
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Fixes #48783