-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Try emitting an error when export would be emitted with different unwidened type #55860
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
Conversation
@typescript-bot test top200 |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at bf925fb. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at bf925fb. You can monitor the build here. |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
Reading live logs, this seems to be firing on |
@typescript-bot test top100 |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 0ca3a1f. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 0ca3a1f. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
A bunch of the above declarations are still ones that shouldn't be visible, so I've clearly still done something wrong. |
@typescript-bot test top100 |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 5560b81. 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 5560b81. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
@typescript-bot test top200 |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 4ef662f. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 4ef662f. You can monitor the build here. Update: The results are in! |
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 4ef662f. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
@jakebailey you mentioned examples where this doesn't just solve a divergence of declaration emit, and maybe I missed the specifics in the design meeting. Can you write up/show what this solves more explicitly? |
Yes, I am building an example repo now! |
Example: https://github.com/jakebailey/dts-emit-widened-unwidened-examples Just to summarize @weswigham, this is arguably a limitation of declaration emit, and we could alternatively fix this by introducing syntax into |
Just guessing based on the title of this PR, but is #55832 related? (it's a case where declaration emit produces a different type--in particular |
@typescript-bot pack this |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 9d8809c. 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 |
This is a very very bad attempt at making the problems we've found in #55445 into check errors. I guarantee that this PR is wrong, you can see very strange things happening because observing
isDeclarationVisible
of each declaration passed tocheckVariableLikeDeclaration
seems to cause a side effect (???), and my check is poorly copied fromdeclarations.ts
so has some improper edge cases.In any case, it still catches everything we mentioned in #55445 so it's somewhat interesting.