Make the implicitly exported rule consistent between checker and binder - #54659
Merged
Conversation
Member
Author
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I'm starting to run the parallelized Definitely Typed test suite on this PR at 777e7be. Hold tight - I'll update this comment with the log link once the build has been queued. |
TypeScript Bot (typescript-bot)
added
the
For Backlog Bug
Member
Author
|
Well, I guess the bot still isn't working, so we'll have to wait to see what this PR breaks in DT. |
Member
Author
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the parallelized Definitely Typed test suite on this PR at 777e7be. You can monitor the build here. Update: The results are in! |
Contributor
|
Hey Gabriela Araujo Britto (@gabritto), the results of running the DT tests are ready. Branch only errors:Package: react-relay Package: react-relay/v7 Package: relay-runtime Package: jsonld Package: relay-test-utils |
Member
Author
|
TypeScript Bot (@typescript-bot) test this pretty please |
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the diff-based user code test suite on this PR at 777e7be. You can monitor the build here. Update: The results are in! |
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the diff-based top-repos suite (tsserver) on this PR at 777e7be. You can monitor the build here. Update: The results are in! |
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the extended test suite on this PR at 777e7be. You can monitor the build here. |
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the perf test suite on this PR at 777e7be. You can monitor the build here. Update: The results are in! |
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the diff-based user code test suite (tsserver) on this PR at 777e7be. You can monitor the build here. Update: The results are in! |
Contributor
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the diff-based top-repos suite on this PR at 777e7be. You can monitor the build here. Update: The results are in! |
Contributor
|
Gabriela Araujo Britto (@gabritto) Here are the results of running the user test suite comparing Everything looks good! |
Contributor
|
Gabriela Araujo Britto (@gabritto) 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
|
Member
|
I am moderately suspicious that the user tests didn't net anything, but maybe that's to be expected if this only happened in ambient contexts. (I guess top100 is still running) |
Contributor
|
Gabriela Araujo Britto (@gabritto) Here they are:
CompilerComparison Report - main..54659
System
Hosts
Scenarios
TSServerComparison Report - main..54659
System
Hosts
Scenarios
StartupComparison Report - main..54659
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
|
Gabriela Araujo Britto (@gabritto) Here are the results of running the top-repos suite comparing Everything looks good! |
1 similar comment
Contributor
|
Gabriela Araujo Britto (@gabritto) Here are the results of running the top-repos suite comparing Everything looks good! |
Gabriela Araujo Britto (gabritto)
requested review from
Jake Bailey (jakebailey),
Ron Buckton (rbuckton) and
Wesley Wigham (weswigham)
June 16, 2023 17:52
|
|
||
| const enum ContainerFlags { | ||
| /** @internal */ | ||
| export const enum ContainerFlags { |
There was a problem hiding this comment.
I'm not experienced enough to know the right way to do this, but it does feel a little suspicious to export... I feel like I've seen a function that does something similar in the checker but I can't recall the name.
Member
Author
There was a problem hiding this comment.
I wanted to make sure the container used to check the implicit export rules is precisely the same the binder uses, and the binder uses those flags for that... I saw a getContainerNode in services/utilities, and getEnclosingBlockScopeContainer in compiler/utilities, but those are different.
Andrew Branch (andrewbranch)
approved these changes
Jun 22, 2023
Andrew Branch (andrewbranch)
added
the
Breaking Change
Member
|
For me or anyone else in the future: the declare module 'replace-in-file' {
export function replaceInFile(config: unknown): Promise<unknown[]>;
export {};
namespace replaceInFile {
export function sync(config: unknown): unknown[];
}
}introduces the same issue. In an ambient module, adding an TypeScript now recognizes these unfortunately confusing semantics more consistently, and issues an error on the fact that all declarations of In the case where there is no declare module 'replace-in-file' {
export function replaceInFile(config: unknown): Promise<unknown[]>;
namespace replaceInFile {
export function sync(config: unknown): unknown[];
}
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Fixes #54342.
In the binder, when deciding if a declaration should be treated as exported or not, we consult the declaration container's flags to see if it is an "export context", meaning an ambient context where declarations are implicitly exported. In the checker, however, we treat everything in an ambient context as exported, regardless of whether we're in an ambient context where declarations are implicitly exported, resulting in us incorrectly not erroring on the
.d.tsfile present in #54342.