Implementation of "Restrict cross-module struct initializers to be delegating" (#2) #12834
Conversation
| if (TheMemory.isStructInitSelf() && | ||
| !Module.getASTContext().isSwiftVersionAtLeast(5)) { | ||
| SILLocation fnLoc = TheMemory.getFunction().getLocation(); | ||
| if (auto *ctor = fnLoc.getAsASTNode<ConstructorDecl>()) { |
There was a problem hiding this comment.
I'd rather we encode this in the MarkUninitializedInst than add more places in DI where we reach into the AST.
| ty = assign->getSrc()->getType(); | ||
| else if (auto *copyAddr = dyn_cast<CopyAddrInst>(inst)) | ||
| ty = copyAddr->getSrc()->getType().getObjectType(); | ||
| return ty && ty == TheMemory.MemorySILType; |
There was a problem hiding this comment.
You should check if the value being assigned to is TheMemory.MemoryInst, instead of just comparing types
There was a problem hiding this comment.
Oh, of course. I didn't know how to do this and my brain came up with this silly approximation.
| Type selfTy; | ||
| SILLocation fnLoc = TheMemory.getFunction().getLocation(); | ||
| if (auto *ctor = fnLoc.getAsASTNode<ConstructorDecl>()) | ||
| selfTy = ctor->getImplicitSelfDecl()->getType()->getInOutObjectType(); |
There was a problem hiding this comment.
Why isn't this always just TheMemory.getType()?
There was a problem hiding this comment.
This way preserves (some) sugar, which is nice.
There was a problem hiding this comment.
If there's a better way to get at the decl or sugared type I'd be glad to get rid of the ugly reaching up into the function.
There was a problem hiding this comment.
How about selfTy.getAsNominalOrBoundGenericNominal()->getDeclaredInterfaceType()?
There was a problem hiding this comment.
That doesn't handle constrained extensions (see test case).
There was a problem hiding this comment.
I don't think that matters, instead you can use getDeclaredType() to print the type without any generic arguments at all
There was a problem hiding this comment.
Well, I wanted the generic arguments, but if you think it's better without them then that would make things simpler.
test/decl/init/cross-module.swift
Outdated
| @@ -0,0 +1,174 @@ | |||
| // RUN: %empty-directory(%t) | |||
| // RUN: %target-swift-frontend -emit-module -emit-module-path=%t/OtherModule.swiftmodule %S/Inputs/OtherModule.swift | |||
| // RUN: %target-swift-frontend -emit-sil -verify -I %t -swift-version 4 %s > /dev/null | |||
There was a problem hiding this comment.
Since this is a SIL test, it should go into test/SILOptimizer/
|
@slavapestov, @gottesmm, this is my first attempt at putting the pre-Swift-5 check in DI. What would you like me to change here? [Slava beat me to it as I was making this comment.] To do:
|
...as detected by initializing an individual field without having initialized the whole object (via `self = value`). This only applies in pre-Swift-5 mode because the next commit will treat all cross-module struct initializers as delegating in Swift 5.
...unless the struct contains a field that cannot be zero-initialized, such as a non-nullable pointer. This suggestion is only made for C structs because 'init()' may not be the right choice for other structs.
jrose-apple
force-pushed
the
restrict-cross-module-struct-initializers-2
branch
from
14878d3 to
1f12e72
Compare
(and when the struct in question is non-fixed-layout, which was already implemented) This ensures that these initializers are never fieldwise in Swift 5 mode, which makes it safe for library authors to add new fields.
...being careful to only do it once per initializer. Additionally, /don't/ offer the suggestion if there was already a conditional assignment to 'self', because that would wipe it out and the user should think harder.
These also have to delegate to another initializer even though there are no stored properties to initialize.
|
Done! Definitely needs review, though. @swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
jrose-apple
changed the title
|
@swift-ci Please test compiler performance |
Build comment file:Compilation-performance test failed |
jrose-apple
force-pushed
the
restrict-cross-module-struct-initializers-2
branch
from
81b0b01 to
93e6123
Compare
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
1 similar comment
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test compiler performance |
Build comment file:Compilation-performance test failed |
Now that struct initializers "just" fall into the delegating case when they're made inlinable, the only interesting case is class initializers, which can be checked in a more direct way than what we were doing before.
jrose-apple
force-pushed
the
restrict-cross-module-struct-initializers-2
branch
from
93e6123 to
ec5ba41
Compare
|
Aargh. That's what I get for trying to edit files while reinstalling Xcode. |
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
|
I think these are unrelated failures. The perf tests actually did seem to finish, too; the results just didn't get posted because some of the projects had trouble. It looks like the full compiler perf tests produce a massive amount of information too, but in most cases there is no change at all from this except in debug builds, where LLVM IR size went up by at most 0.1% and sometimes even went down. @swift-ci Please test |
|
@swift-ci Please smoke test compiler performance |
|
@swift-ci build toolchain |
|
Unable to build the toolchain due to |
|
/cc @slavapestov, @shahmishal you’d want a clean build i think |
|
@swift-ci clean test |
|
Build failed |
|
@swift-ci Please clean test Linux |
|
Build failed |
|
@swift-ci Please clean test Linux |
|
@swift-ci Please test |
|
@swift-ci clean test |
|
@shahmishal are the tests even running on this one? |
|
Linux passed - https://ci.swift.org/job/swift-PR-Linux/1645/ |
|
@swift-ci build toolchain |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
macOS Toolchain Install command |
|
And the compiler perf test success/failure isn't significant—what's important is that the numbers didn't go up for this PR, which they didn't. Merging! |
jrose-apple
added
swift evolution approved
| @@ -327,7 +328,6 @@ extension Decimal { | |||
|
|
|||
| var i = UInt32(0) | |||
| // this is a bit ugly but it is the closest approximation of the C initializer that can be expressed here. | |||
There was a problem hiding this comment.
I think this comment can be removed?
There was a problem hiding this comment.
*shrug* I thought the loop still counted as ugly, but maybe the comment was just referring to the tuple of zeros. @phausler?
AnthonyLatsis
added
swift evolution implemented
Implementation of SE-0189. See the first commit for how this affects the Apple SDK overlays.
rdar://problem/34777878