Skip to content

Struct and enum declarations no longer go through finalizeDecl() #26159

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

Conversation

slavapestov
Copy link
Contributor

Builds on #26119. Now that getStoredProperties() is a request, Sema no longer has to finalize struct and enum declarations.

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8f0d39f

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8f0d39f

@slavapestov slavapestov force-pushed the do-not-finalize-structs-and-enums branch from 8f0d39f to 097df9a Compare July 16, 2019 20:41
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8f0d39f

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8f0d39f

@slavapestov
Copy link
Contributor Author

@swift-ci Please test compiler performance

@slavapestov slavapestov marked this pull request as ready for review July 16, 2019 23:45
@slavapestov
Copy link
Contributor Author

(Both source compat failures are UPASS because an unrelated problem was fixed on master)

@slavapestov slavapestov force-pushed the do-not-finalize-structs-and-enums branch from 097df9a to e965c00 Compare July 17, 2019 19:56
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

Instead of adding a resolveDeclSignature() call here, I'm going to live
dangerously and try to only get the enum element type in the case where
SIL type lowering has already computed it, that is, if the enum is not
indirect.

Soon this will become moot anyway because getInterfaceType() will be a
request.
Since getStoredProperties() is a request that lowers lazy properties
and property wrappers to their underlying storage, and SIL can validate
stored property and enum element types, there's no longer any need for
Sema to explicitly finalize members of structs and enums. SILGen can
trigger any necessary type checkin work just by lowering a struct or
enum type.

Now the only remaining reason we need finalizeDecl() is adding implicit
methods to classes, and synthesizing accessors for storage in classes
and protocols.
Now that SILGen and IRGen can trigger type checking work, we have to run the
full pipeline to get a complete picture.

Thankfully everything still passes!
@slavapestov slavapestov force-pushed the do-not-finalize-structs-and-enums branch from e965c00 to 84b0267 Compare July 17, 2019 22:07
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov slavapestov requested review from DougGregor and xymus July 17, 2019 22:10
@slavapestov slavapestov merged commit 0db8bbd into swiftlang:master Jul 18, 2019
@@ -237,7 +237,7 @@ namespace {
llvm_unreachable("shouldn't get an inout type here");
}
RetTy visitErrorType(CanErrorType type) {
llvm_unreachable("shouldn't get an error type here");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are error types wandering into SIL type lowering?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type checking is now sufficiently lazy that if a struct has a field with an invalid type, and you don't directly reference the invalid field from source, SILGen might end up lazily validating the stored properties of the struct anyway, if you nevertheless emit an expression involving a value of the struct's type.

@DougGregor
Copy link
Member

This is fantastic!

@DougGregor
Copy link
Member

DougGregor commented Jul 18, 2019

Do we get detection of recursive struct/enum definitions for free now from type layout?

CodaFi added a commit to CodaFi/swift that referenced this pull request Oct 31, 2019
Push through an easy refactoring to the way we validate and install
implicit constructors.  This patch would be NFC but for a regression
test that now must diagnose.  swiftlang#26159 changed validation order in such
a way that the code in validation-test-macosx-x86_64/compiler_crashers_2_fixed/0124-sr5825.swift
used to be accepted.  This patch once again changes validation order, so
we now reject this code, restoring the behavior seen on all prior
versions of Swift.

On its face, this test should work.  In order for it to do so, witness
matching has to be smarter about the declarations it asks for their
interface type, or it will risk these circular constructions
accidentally being accepted or rejected on a whim.
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.

4 participants