Make the implicitly exported rule consistent between checker and binder #54659
Conversation
This file contains 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
|
@typescript-bot run DT |
|
Well, I guess the bot still isn't working, so we'll have to wait to see what this PR breaks in DT. |
|
@typescript-bot run DT |
|
Heya @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! |
|
Hey @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 |
|
@typescript-bot test this pretty please |
|
Heya @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! |
|
Heya @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! |
|
Heya @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! |
|
Heya @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! |
|
Heya @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! |
|
@gabritto Here are the results of running the user test suite comparing Everything looks good! |
|
@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
|
|
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) |
|
@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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
1 similar comment
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
jakebailey
reviewed
Jun 16, 2023
| @@ -455,7 +456,8 @@ function getModuleInstanceStateForAliasTarget(specifier: ExportSpecifier, visite | |||
| return ModuleInstanceState.Instantiated; // Couldn't locate, assume could refer to a value | |||
| } | |||
|
|
|||
| 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.
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.
andrewbranch
approved these changes
Jun 22, 2023
|
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[];
}
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.