Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Isolated declarations #53463
Isolated declarations #53463
Changes from 250 commits
0a1f101
15b7e60
3a3e7e5
220fb0d
6e866ca
8ab3fcb
7f574a8
b97e79f
c509a6c
e4cf7a0
4a66f33
adb25b1
a837746
5d88b4e
902f5ee
d653203
cc478de
643d33c
40f9a58
82dfd3b
49025f5
580be4c
cd8feab
7781968
cb280cc
3d9503d
63c219d
b81cdc0
2af866d
3ec90b0
7800241
fe34c87
9942551
2f04c7a
0dd34fe
1a1d0bd
7ca6435
2d309c6
9767aa9
00d340d
faa3cf1
e772096
f1f82f9
dde8cb6
aeeb920
b0d064d
35d400e
aac2f67
5904193
570f403
c6b60fc
86d5751
276f9ed
9416f38
24a5f53
9c274fd
60960f8
c474481
192982e
bee8fa8
c0b63bf
62fd7be
57d243a
0dfdee1
9b2fb26
363e9d1
430bedc
035429c
1f85796
7abf074
303b991
61b1ea7
3b8e896
da9b9c2
1e14150
32d02cc
72451bb
ae1646e
b50c877
10724ec
98fb92a
3077592
fb324e5
6c2a8d3
e9ca3d4
5c82583
a7c15af
5e2e6d0
e878ebd
981e948
ef231d9
6faac4f
852c2a8
2c8eaf5
24fd810
04cedfe
46e9453
a4bec21
c20406f
1c7972f
87ad092
d390052
83ae26a
011f142
883cc69
ae61dd6
d6f9d2d
9c80f76
ea7b82a
36e5505
4a4312f
5a7784c
9856f97
e1a2304
3936d4a
a406046
ffe4b1c
3023b9f
9a3db9c
9dc09ca
fca6fb1
35ccc67
767f0fe
57f21b1
91c8b76
e45e9b8
c236425
2862991
93912bb
f086f35
672a57a
1074f9a
eeb8a9f
b7d062a
6f3a0ec
23840f4
96c9c15
2dfc9d1
867e100
3199057
9ca6342
d4e8654
1d882e1
cf214ae
9c19913
da557b0
24d8a3c
a8c29dc
c0d6726
6ca0b26
0f12363
cf1ca98
92b3dcb
26e9574
7ab0f0d
7067307
4e01096
c0831c2
d9328ea
d89c2ae
500d2a3
af1e6fa
8dcb272
7bffe7e
31370b0
f7a65aa
142ff2b
62318bf
69d991a
4554796
1b4c121
898eb0f
fb08ff8
261eba5
2c0bf81
d91c282
f1cfac4
a0f31ee
901c7e3
e18d2e8
c5ddc9b
afaec96
211b056
97161e0
a6fe9fc
f49ebd9
5531cef
970c397
c45ffaf
fcfb698
66b6ac1
03135a7
7133c47
d1f598a
20c5351
9ecd15f
3f0ca54
4164b9b
d99cdf5
857f90b
a3a5049
202d522
be73e33
c223e05
b7d936c
7776b34
c3c863d
a97dfbc
04097f6
71131db
8e9f0f5
52158b5
999d03e
5a1db4f
d097266
8b4221e
868f250
4097ae1
c0b4867
f0fad1b
7cf4447
ef6a868
0ad2847
3b6033b
d27c784
7c4dbed
c3245bb
b2706e0
27ff8b2
2164696
e2e6adc
6987970
b2b946c
2b27f43
47374f3
6803a00
edc4d32
f65c3b6
5ff13fb
8347b23
373aaf4
163e4ba
571e43f
e7ad116
a81b583
418f975
3188a82
e1bb300
dc1ffe9
a9d06d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel weird about this backdoor for testing. Is there any way to test the same thing without it? Was this just helpful once during testing and doesn't need to stick around? Reading 6803a00 it's only called from with isolated declaration emit; is it the case that the injected check could just always be present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that both
transpileDeclaration
and TSC emit use thesymbol
property on the node. TSC will put a full symbol in it, whiletranspileDeclaration
will put a minimal symbol that has enough information to make declaration emit work.During tests we reuse the ASTs between running the same test. As long as both emit methods go though their binding phase and set the symbol, this is not a problem. However a bug in the binder used by
transpileDeclaration
meant that a node had a TSC symbol instead of atranspileDeclaration
one. The problem is that everything worked fine, while callingtranspileDeclaration
, the declaration transform happily used the TSC symbol (which it should be able to do, declaration emit with--isolatedDeclarations
in TSC will not use the binder intranspileDeclaration
).This masked the bug, until I tried to actually use
transpileDeclaration
in context where the AST was not a hand me down from regular TSC, and so the symbol was missing.The
assertSymbolValid
will assert the symbol is not null while in TSC, and in tests that calltranspileDeclaration
it will assert that teh symbol is not missing AND that it is a minimal symbol that comes from thetranspileDeclaration
declaration binder, ensuring thattranspileDeclaration
doesn't accidentally use TSC symbols.The reason this needs to be injected is that the same code paths are used both in TSC and in
transpileDeclaration
always asserting it's a TSC symbol or atranspileDeclaration
minimal symbol is not an option.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I guess I thought that it would?
I'm still not happy with this backdoor, but I also can't think of a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these are true. it does allow one file at a time declaration emit, but it also ensures that not types need to be created by the type checker. I feel the latter constraint is the stricter one since it forbids most inference even if the information is available in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what’s going on in
localInferenceResolver.ts
is “type information,” but I take your point that needing to annotate function return types is probably more visible to users than the lack of cross-file information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
is the typical style. My ear also slightly prefers
in '--isolatedDeclarations'
overwith
, while the wordierwhen '--isolatedDeclarations' is enabled
is more established... I don’t have a very strong opinion here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: “cannot” is typical