Fully resolve aliases when checking symbol flags#50853
Merged
andrewbranch merged 9 commits intomicrosoft:mainfrom Sep 29, 2022
Merged
Fully resolve aliases when checking symbol flags#50853andrewbranch merged 9 commits intomicrosoft:mainfrom
andrewbranch merged 9 commits intomicrosoft:mainfrom
Conversation
…ged with an alias that resolves to a value 😵
typescript-bot
added
Author: Team
For Uncommitted Bug
Member
Author
|
@typescript-bot user test inline |
Collaborator
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at a97a11e. You can monitor the build here. |
Collaborator
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at a97a11e. You can monitor the build here. Update: The results are in! |
Collaborator
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at a97a11e. You can monitor the build here. Update: The results are in! |
Collaborator
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
Collaborator
|
@andrewbranch Here they are:
CompilerComparison Report - main..50853
System
Hosts
Scenarios
TSServerComparison Report - main..50853
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
JoostK
reviewed
Sep 20, 2022
andrewbranch
requested review from
DanielRosenwasser,
gabritto and
sandersn
gabritto
approved these changes
Sep 21, 2022
|
|
||
| ==== tests/cases/conformance/externalModules/a.ts (0 errors) ==== | ||
| function A() {} | ||
| export type { A }; |
Member
There was a problem hiding this comment.
Why is this not an error? Since we're exporting as type-only something that is not a type.
Member
Author
There was a problem hiding this comment.
Someone who imports this is allowed to use type ASignature = typeof A
| export const displayName = "A"; | ||
| } | ||
|
|
||
| A(); |
Member
There was a problem hiding this comment.
to check my own understanding, this is an error because we don't allow the function value to merge with the namespace?
Member
Author
There was a problem hiding this comment.
That’s right, but only because A is an alias. Normally, slightly disconcerting though it is, we allow a function to merge with an instantiated namespace: playground. I only learned this in the course of my PR, but the rules are stricter for merges across module boundaries (thankfully). Two values are just not allowed to merge across modules.
| !checkAndReportErrorForExtendingInterface(errorLocation) && | ||
| !checkAndReportErrorForUsingTypeAsNamespace(errorLocation, name, meaning) && | ||
| !checkAndReportErrorForExportingPrimitiveType(errorLocation, name) && | ||
| !checkAndReportErrorForUsingNamespaceAsTypeOrValue(errorLocation, name, meaning) && |
Member
There was a problem hiding this comment.
I think that's covered in the tests, but what's the consequence of changing this check's order?
Member
Author
There was a problem hiding this comment.
It’s been a while, but I believe it’s this:
A namespace is a value if it exports values. If it’s not a value, it’s not a type per se, but it’s fairly typey. I believe after tweaking the meaning flag checks in these functions to be a bit easier to read, the checkAndReportErrorForUsingTypeAsValue started reporting an error on typey (uninstantiated) namespaces, which was stepping on the toes of checkAndReportErrorForUsingNamespaceModuleAsValue, which wanted to issue essentially the same error, but slightly more precise in mentioning that the non-valuey thing is a namespace. So I simply moved that check first so it would take precedence. So I actually think this order change isn’t reflected in the tests, because it just neutralizes the effect of another readability change. (I also renamed checkAndReportErrorForUsingNamespaceModuleAsValue to checkAndReportErrorForUsingNamespaceAsTypeOrValue because nobody knows what a “namespace module” is, and it was already issuing errors when using a namespace as a type.)
jakebailey
reviewed
Sep 28, 2022
Member
Author
|
@typescript-bot perf test this faster |
Collaborator
|
Heya @andrewbranch, I've started to run the abridged perf test suite on this PR at fcefb22. You can monitor the build here. Update: The results are in! |
Collaborator
|
@andrewbranch Here they are:Comparison Report - main..50853
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
approved these changes
Sep 29, 2022
Member
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
LGTM, though I'm not sure I'm a massive fan of the ?? -1; it seems like most call sites do that but I don't know if I have any better method, besides maybe SymbolFlags.All or something, but that still requires the ??.
Member
Author
|
I might look at making |
andrewbranch
force-pushed
the
bug/44896
branch
from
68b3b16 to
72ff9e5
Compare
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.
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 #44896
Fixes #50455
I actually don’t think this is a breaking change in any meaningful way. I do have one test that exhibits new errors, but the scenario is extremely contrived and didn’t exactly work before. Mostly, this removes false errors and false import elisions. The most visible and understandable example is the one from #44896: