-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[AST] Fully replace NameAliasType with the new, sugary-sweet BoundNameAliasType #15497
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
[AST] Fully replace NameAliasType with the new, sugary-sweet BoundNameAliasType #15497
Conversation
@swift-ci please test |
@swift-ci please test source compatibility |
@swift-ci please test |
@swift-ci please test source compatibility |
@swift-ci please test |
@swift-ci please test source compatibility |
Build failed |
Build failed |
Serialization can’t handle them, and I’m not up for fixing it just yet.
This doesn't have a specific effect now, because all of these places are likely to only see NameAliasType, but it is refactoring with the intent of eliminating NameAliasType entirely.
With the exception of “has type variable”, which affects the arena used for storage of a BoundNameAliasType, only propagate recursive properties from the underlying type to a BoundNameAliasType, because the other properties (e.g., “has archetype” or “has type parameter”) pulled from syntactic sugar don’t apply.
When we’re performing type transformations involving types with type parameters or archetypes, drop the sugar rather than performing potentially-harmful transformations on it.
We currently accept code that refers to `ProtocolName.TypeName` when `TypeName` is a typealias whose underlying type doesn’t refer to any type parameters, which is awful. We should ban it, but until we do… don’t represent the protocol in sugar or form substitutions, because they won’t work.
We should always profile all of the fields, lest we run into odd unintentional collisions.
The generic signature used to perform substitutions for a BoundNameAliasType in ill-formed code might not line up with the generic signature in the typealias. Separately record the signature we used to build the BoundNameAliasType to make the AST more robust against such issues.
31897ef
to
095c023
Compare
@swift-ci please test |
@swift-ci please test source compatibility |
@swift-ci please smoke test |
Build failed |
@swift-ci please smoke test |
@swift-ci please test source compatibility |
Build failed |
Rather than relying on the NameAliasType we get by default for references to non-generic typealiases, use BoundNameAliasType consistently to handle references to typealiases that are formed by the type checker.
… diff This test depends on comparing the binary .swiftmodule files (with cmp) using the sib and non-sib paths. However, the binary .swiftmodule can differ slightly due to type sugar along this path, so use a textual diff instead... which is also far more readable.
We still use the old layout for NameAliasType for builtin types, so rename the Layout struct and corresponding code to describe its new (more restricted) purpose.
NameAliasType is dead! Long live NameAliasType!
095c023
to
b2b69e8
Compare
@swift-ci please smoke test |
The LLDB failure is real and should be addressed by apple/swift-lldb#452 |
Source compatibility is being tested by https://ci.swift.org/job/swift-PR-source-compat-suite/948/; looks good thus far. |
Can you update the PR description to provide more detail on the motivation and benefit of the changes? Is this just about producing better diagnostics as a result of being sugar-and-substitution-preserving? |
@swift-ci please smoke test and merge |
1 similar comment
@swift-ci please smoke test and merge |
It's partly about better diagnostics (because we keep more sugar), partly about explicitly modeling substitutions in Types (which we want to do elsewhere as well), and partly because we depend on that sugar for some semantics (including requirement inference, e.g., in support of #15513, and also for access control, which checks based on Type sugar). |
Something in the interface between LLDB and Swift is causing this check to fire. Disable it temporarily while we work on that interface.
@swift-ci smoke test |
1 similar comment
@swift-ci smoke test |
@swift-ci please smoke test |
Fix a number of small issues with
BoundNameAliasType
to bring it up to parity withNameAliasType
, so the former will be able to replace the latter. This includes:BoundNameAliasType
s profiling (forFoldingSet
) and formation more robust against ill-formed ASTsBoundNameAliasType
everywhere that we handle aNameAliasType
, including for special cases where we currently always get aNameAliasType
. This part is pure staging.Once done, wwitch all references to type aliases over to the new sugar-and-substitution-preserving
BoundNameAliasType
. Then eliminate the (now-unused)NameAliasType
and steal its name forBoundNameAliasType
.NameAliasType
is dead. Long liveNameAliasType
!