swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.58k stars 10.36k forks source link

[SR-14070] Property wrapper SIL assertion failure in development snapshot #56459

Closed xAlien95 closed 3 years ago

xAlien95 commented 3 years ago
Previous ID SR-14070
Radar rdar://problem/73406156
Original Reporter @xAlien95
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Environment swift-DEVELOPMENT-SNAPSHOT-2021-01-17-a.xtoolchain Target: x86_64-apple-darwin20.2.0
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 0 | |Component/s | Compiler | |Labels | Bug, PropertyWrappers, Regression | |Assignee | @hborla | |Priority | Medium | md5: 0a81634482216a0c94e91c76a6e6d6e4

Issue Description:

Consider the following code snippet:

import SwiftUI

struct S {
  @State var value: Int

  init() {
    value = 10  // <1>
  }
}

With Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28), line <1> shows the error

Variable 'self.value' used before being initialized

With Swift Development Snapshot 2021-01-17, no errors are shown, but if you start building you get an abort trap (see attachment "abort_trap.txt").


Related: if you add a generic parameter to S

import SwiftUI

struct S<T> {
  @State var value: Int

  init() {
    value = 10  // <1>
  }
}

with Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28), line <1> shows the error

Variable 'self.value' used before being initialized

With Swift Development Snapshot 2021-01-10 and later, no errors are shown, but if you start building you get an abort trap (see attachment "abort_trap_generic.txt").

xAlien95 commented 3 years ago

#35218 diff with the changed behavior. Those tests were error checking tests and weren't supposed to have sample code to be run involving the provided structs and property wrappers. With the compile error removed, they should be moved to test/SILOptimizer/di_property_wrappers.swift? cc @hborla

hborla commented 3 years ago

Thanks for finding this! From the log you attached, it looks like DI is doing the right thing (re-writing the property wrapper assignment to initialization), but there's a verification failure on the property-wrapper setter, which is unused. I think this should only affect asserts compilers, but obviously it should be fixed either way.

As for your second question, I think it's fine for "error" tests to contain cases that are okay. We probably don't gain anything by making that particular case executable.

hborla commented 3 years ago

@swift-ci create

hborla commented 3 years ago

I also can't seem to reproduce this if the property wrapper is not imported from another module, even with -sil-verify-all. Looks like in this case the SIL is deserialized, and there's a different verification path that isn't reachable otherwise

xAlien95 commented 3 years ago

This reduced snippet reproduces the Abort trap: 6 error without imports on my machine (it's the first one in di_property_wrappers_errors.swift):

@propertyWrapper class ClassWrapper<T> {
  var wrappedValue: T

  init(wrappedValue initialValue: T) {
    self.wrappedValue = initialValue
  }
}

struct IntStructWithClassWrapper {
  @ClassWrapper var wrapped: Int

  init() {
    wrapped = 42
  }
}
hborla commented 3 years ago

Perfect, thank you! Thankfully it still looks like the same issue. You're right that this should be moved to a separate file that doesn't contain errors. My mistake - I thought this was the same case as StructWithClassWrapper from test/SILOptimizer/di_property_wrappers.swift, but it's not because that assignment in the initializer gets re-written to the property wrapper setter since the Int? is default initialized to nil.

hborla commented 3 years ago

This should be fixed by https://github.com/apple/swift/pull/36253

xAlien95 commented 3 years ago

Unfortunately it's not. With swift-DEVELOPMENT-SNAPSHOT-2021-03-05-a (which contains #36253) I still get the same SIL failure.

hborla commented 3 years ago

Okay, this one should fix it https://github.com/apple/swift/pull/36384

It wasn't the closure that was causing the assertion failure, but the `self` that the closure captured.

xAlien95 commented 3 years ago

Yes, I just tested with swift-DEVELOPMENT-SNAPSHOT-2021-03-20-a. It has been fixed. Thank you @eeckstein and @hborla!

May I share another PropertyWrapper related bug which hasn't gotten a Radar URL yet? https://bugs.swift.org/browse/SR-14080

xAlien95 commented 3 years ago

Resolved in swift-DEVELOPMENT-SNAPSHOT-2021-03-20-a.