Skip to content

[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

Merged
merged 17 commits into from
Mar 26, 2018

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented Mar 25, 2018

Fix a number of small issues with BoundNameAliasType to bring it up to parity with NameAliasType, so the former will be able to replace the latter. This includes:

  • Making BoundNameAliasTypes profiling (for FoldingSet) and formation more robust against ill-formed ASTs
  • Not recording module types or existential parent types in the AST, because serialization cannot handle the former and the latter shouldn't be well-formed at all anyway
  • Adding lots of redundant logic so that we handle a BoundNameAliasType everywhere that we handle a NameAliasType, including for special cases where we currently always get a NameAliasType. 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 for BoundNameAliasType.

NameAliasType is dead. Long live NameAliasType!

@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 31897efafa1c3c2926a870d123e4b24907722fd3

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 31897efafa1c3c2926a870d123e4b24907722fd3

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.
@DougGregor DougGregor force-pushed the remove-name-alias-type branch from 31897ef to 095c023 Compare March 26, 2018 04:06
@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 31897efafa1c3c2926a870d123e4b24907722fd3

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 31897efafa1c3c2926a870d123e4b24907722fd3

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!
@DougGregor DougGregor force-pushed the remove-name-alias-type branch from 095c023 to b2b69e8 Compare March 26, 2018 04:35
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

The LLDB failure is real and should be addressed by apple/swift-lldb#452

@DougGregor
Copy link
Member Author

Source compatibility is being tested by https://ci.swift.org/job/swift-PR-source-compat-suite/948/; looks good thus far.

@rudkx
Copy link
Contributor

rudkx commented Mar 26, 2018

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?

@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

@DougGregor
Copy link
Member Author

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.
@DougGregor
Copy link
Member Author

@swift-ci smoke test

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci smoke test

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor DougGregor merged commit 4797fae into swiftlang:master Mar 26, 2018
@DougGregor DougGregor deleted the remove-name-alias-type branch March 26, 2018 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants