[Sema] SR-15940: Assert crash in TypeCheckStmt.cpp

[hank121314]

Summary

This PR fixes: SR-15940 #58201.

In TypeCheckStorage.cpp, it will type-check default initializer and if there is a autoclosure in default initializer, its discriminator will also be set.
And In TypeCheckDeclPrimary.cpp, when visiting PatternBindingDecl it will type-check the autoclosure in default initializer again which might cause the assertion error.

Solution

This PR tries to stop the recursive check in TypeCheckStmt.cpp if the autoclosure is already checked in default initializer.

Thanks

Please feel free to critique, and thanks for your code review 😄 .

[xedin]

@swift-ci please test

[xedin]

@swift-ci please test source compatibility

[hborla]

Hi @hank121314, thanks for digging into this!

In TypeCheckStorage.cpp, it will type-check default initializer and if there is a autoclosure in default initializer, its discriminator will also be set.

As you've discovered, this bit of code calls typeCheckSynthesizedWrapperInitializer, which will contextualize closures. What I'm curious about it your next point:

And In TypeCheckDeclPrimary.cpp, when visiting PatternBindingDecl it will type-check the autoclosure in default initializer again which might cause the assertion error.

Notice the comment above this bit of code that you linked:

        // Note that we don't contextualize the initializer for a property
        // with a wrapper, because the initializer will have been subsumed
        // by the backing storage property.

Why isn't the initializer skipped in this case based on the condition !(PBD->getSingleVar() && PBD->getSingleVar()->hasAttachedPropertyWrapper()))?

[hank121314]

Hi @hborla, Thanks for the review!

Why isn't the initializer skipped in this case based on the condition !(PBD->getSingleVar() && PBD->getSingleVar()->hasAttachedPropertyWrapper()))?

For example:

@propertyWrapper
public class SR_15940Bar<Value> {
  public init(wrappedValue: @autoclosure () -> Value) {}
  public var wrappedValue: Value {}
}

class SR_15940 {
  @SR_15940Bar var a: Bool?
}

The property declaration @SR_15940Bar var a: Bool? will create another PatternBindingDecl which is var _a: SR_15940Bar<Bool?>(In TypeCheckStorage.cpp).

When visiting var _a: SR_15940Bar<Bool?>, it does not have any attached property wrapper. So hasAttachedPropertyWrapper will return false.

[xedin]

@hank121314 If I understand correctly _a initializer is synthesized while synthesizing one for a, so it would be great if you could figure out why exactly the initializer of _a gets checked twice, which is the underlying issue in this case. Your changes, albeit correct, fixed the symptom instead…

[hank121314]

For this test case, the initializer of a and the initializer of _a(its backing storage property) are the same.
In TypeCheckStorage.cpp, when the initializer of a gets checked, initializer of _a will also get checked.
And according to comments In TypeCheckDeclPrimary.cpp:

// Note that we don't contextualize the initializer for a property
// with a wrapper, because the initializer will have been subsumed
// by the backing storage property.

The initializer of _a will get checked again because it is a backing storage property.

IMHO, the fixes should be checking whether _a is a backing storage property and whether its initializer gets checked.
If _a is a backing storage property and it is already getting checked, skip the contextualize in TypeCheckDeclPrimary.cpp.

Thanks for pointing out if I have misunderstood anything!

[xedin]

@hank121314 That sounds like the right approach!

[hank121314]

Hi @xedin !

I am trying to add a property isSynthesized to PatternBindingEntry to check whether the initializer gets checked.
In TypeCheckDeclPrimary.cpp, It will check whether _a is a backing storage property.
If it is a backing storage property, check whether its original PatternBindingDecl is synthesized.
If the original PatternBindingDecl is already synthesized skip it.

Thanks for the review once again!

[xedin]

I don't think you actually need synthesized flag since we already have isInitializerChecked(unsigned) and based on PropertyWrapperInitializerInfoRequest::evaluate - backing property always sets it, so really all you need to check is whether given variable is a backing property of some properly wrapper + assert that it's initializer is marked as checked and a continue;.

Does that make sense, @hborla?

[hank121314]

According to comments in TypeCheckStorage.cpp:

// Force the default initializer to come into existence, if there is one,
// and the wrapper doesn't provide its own.

PropertyWrapperInitializerInfoRequest::evaluate will only synthesize the initializer which does not have default initializer existence.
If the PatternBindingDecl have default initializer(ex. @Lazy var foo = 17, @UnsafeMutablePointer(mutating: addressOfInt) var someInt), its initiailizer will set as checked in PropertyWrapperBackingPropertyTypeRequest::evaluate by TypeChecker::typeCheckPatternBinding. But will not sythesize in TypeCheckStorage.cpp.

Thanks for pointing out if I have misunderstood anything!

[xedin]

So instead of an assert it would be a check?

[hank121314]

If the PatternBindingDecl have a default initializer existence, even if isInitializerChecked(unsigned) returns true, It might not be synthesized yet.
So I think it might be unsafe to use isInitializerChecked(unsigned) instead of the synthesized flag.

[xedin]

If there is no initializer yet for a backing property isInitializerChecked should not return true unless it’s a bug though, isn’t?

[hank121314]

If there is no initializer yet for a backing property isInitializerChecked should not return true unless it’s a bug though, isn’t?

Yes, In PropertyWrapperInitializerInfoRequest::evaluate, it will set initializer for a backing property and setInitializerChecked.
In TypeCheckDeclPrimary.cpp, isInitializerChecked of a backing property will always return true , but it might not be synthesized yet. because PropertyWrapperInitializerInfoRequest::evaluate will only synthesize the initializer which does not have default initializer existence.

[xedin]

Sorry, you are making contradictory statements. When PropertyWrapperInitializerInfoRequest::evaluate sets initializer for a backing property its sets a completely type-checked initializer taken from the original property regardless whether it's a default one or not (non-default initializer should already be type-checked). The act of setting it on pbd in that method means that the initializer has been synthesized and we just want to skip that in visitPatternBindingDecl.

[xedin]

Also note that when it comes to visitPatternBindingDecl we don't really care about "synthesized" in a narrow sense as in "produced by the compiler", we only care that the initializer of a backed property has already been type-checked.

[hank121314]

Also note that when it comes to visitPatternBindingDecl we don't really care about "synthesized" in a narrow sense as in "produced by the compiler", we only care that the initializer of a backed property has already been type-checked.

But if we skip the TypeChecker::contextualizeInitializer in visitPatternBindingDecl when isInitializerChecked return true, some auto closure might not get contextualize.

Here is how I am trying to skip the TypeChecker::contextualizeInitializer in visitPatternBindingDecl:

if (!DC->isLocalContext() &&
    !(PBD->getSingleVar() &&
      PBD->getSingleVar()->hasAttachedPropertyWrapper())) {
  auto *initContext =
      cast_or_null<PatternBindingInitializer>(PBD->getInitContext(i));
  if (initContext) {
    // Check whether `var` is a backing storage property and its
    // initializer is already checked.
    bool shouldSynthesized = true;
    if (auto var = PBD->getSingleVar()) {
      if (auto kind =
              var->getPropertyWrapperSynthesizedPropertyKind()) {
        if (kind == PropertyWrapperSynthesizedPropertyKind::Backing) {
          assert(PBD->isInitializerChecked(i));
          shouldSynthesized = false;
        }
      }
    }
    if (shouldSynthesized) {
      TypeChecker::contextualizeInitializer(initContext, init);
    }
    checkInitializerActorIsolation(initContext, init);
    TypeChecker::checkInitializerEffects(initContext, init);
  }
}

And the failed test example when auto closure is not getting contextualize:

// RUN: %target-swift-frontend -emit-ir %s

@propertyWrapper
public class SR_15940Bar<Value> {
    private var _value: Value

    public var wrappedValue: Value {
        get { _value }
        set {
            _value = newValue
        }
    }
    public init(wrappedValue value: @autoclosure @escaping () -> Value) {
        self._value = value()
    }
}

// SR-15940
class SR_15940_A {
    @SR_15940Bar var a: Bool = false
}

Thank you for all your assistance ❤️ !

[xedin]

Then the question is why an initializer that is marked as type-checked is not getting contextualized although it should be.

[xedin]

Meaning typeCheckSynthesizedWrapperInitializer does contextualization but why it’s not happening when there is explicit initializer like in your example with @SR_15940Bar var a: Bool = false?…

[hank121314]

Meaning typeCheckSynthesizedWrapperInitializer does contextualization but why it’s not happening when there is explicit initializer like in your example with @SR_15940Bar var a: Bool = false?…

If there is no explicit initializer(ex. @SR_15940Bar var a: Bool?), it will satisfy these conditions in TypeCheckStorage.cpp and do the contextualization with typeCheckSynthesizedWrapperInitializer.
But if there is explicit initializer, it will not satisfy !parentPBD->isInitialized(patternNumber), because it already has an initializer which is set by ParseDecl.

[xedin]

The question I am asking is why isn’t explicit initializer contextualized before PropertyWrapperInitializerInfoRequest::evaluate if it is considered to be fully type-checked…

[hank121314]

The question I am asking is why isn’t explicit initializer contextualized before PropertyWrapperInitializerInfoRequest::evaluate if it is considered to be fully type-checked…

In this test case, the initializer is set as fully type-checked in TypeCheckConstraints.cpp when doing visitClassDecl. But it is not contextualized yet.

[xedin]

It seems reasonable to contextualize explicit initializers in PropertyWrapperInitializerInfoRequest::evaluate then too?
Alternatively, typeCheckSynthesizedWrapperInitializer can get a flag to indicate whether it should run the checks or not and it would only be turned on in case where backed property gets independent initializer. This way the behavior of relying on backed property to run contextualization in visitPatternBindingDecl would be made consistent between default and non-default initializers.

[hank121314]

Alternatively, typeCheckSynthesizedWrapperInitializer can get a flag to indicate whether it should run the checks or not and it would only be turned on in case where backed property gets independent initializer. This way the behavior of relying on backed property to run contextualization in visitPatternBindingDecl would be made consistent between default and non-default initializers.

This solution sounds great!
Many thanks for your assistance and detailed explanation!
I will update the PR after the local testing is passed!

[hank121314]

Hi @xedin !

I have updated the PR!

typeCheckSynthesizedWrapperInitializer can get a flag to indicate whether it should run the checks or not and it would only be turned on in case where backed property gets independent initializer.

For this test case:

@propertyWrapper
struct SR_15940Bar {
  var wrappedValue: Int { 0 }
}
struct SR_15940_B {
  func a() {
    @SR_15940Bar var b: Int
  }
}

I also notice that the flag should be turned on in case where property wrapper has defaultInit and it is a local variable. (visitingPatternBindingDecl will filter out local variables).

Many thanks for the review once again!

[xedin]

Thank you! I'll take a look tomorrow.

[xedin]

@swift-ci please test

[hborla]

After this return statement, the function also checks actor isolation and initializer effects. Do we have tests to make sure the cases where contextualize is false still perform actor isolation and effects checking? We might also want to call the parameter something else since it skips more than contextualizing closures.

[xedin]

That's a good question. @hank121314 You can comment out the logic in visitPatternBinding and see if we get any test failures from that related to property wrappers.

[hank121314]

We might also want to call the parameter something else since it skips more than contextualizing closures.

Agree! Is there any recommendation about the parameter name?

Do we have tests to make sure the cases where contextualize is false still perform actor isolation and effects checking?

There are two kinds of situations that make contextualize false.

1. Forcing the default initializer to come into existence. In this situation, actor isolation and effects checking will be perform in visitPatternBinding, if dc->isLocalContext returns false. If dc->isLocalContext returns true, actor isolation and effects checking will be perform in TypeCheckFunctionBodyRequest.

Test example:

@propertyWrapper
public class SR_15940Bar<Value> {
  private var _value: Value

  public var wrappedValue: Value {
    get { _value }
    set {
      _value = newValue
    }
  }

  public init(wrappedValue value: @autoclosure @escaping () -> Value) {
    self._value = value()
  }
}

struct SR_15940_A {
  // If `dc->isLocalContext` returns `false`, actor isolation and effects checking will be perform in `visitPatternBinding`.
  @SR_15940Bar var a: Bool?
  func b() {
    // If `dc->isLocalContext` returns `true`, actor isolation and effects checking will be perform in `TypeCheckFunctionBodyRequest`.
    @SR_15940Bar var c: Bool?
  }
}

2. PatternBindingDecl is not initialized, wrapper has defaultInit and dc->isLocalContext is false. In this situation, actor isolation and effects checking will be perform in visitPatternBinding.

Test example:

@propertyWrapper
struct SR_15940Foo {
  var wrappedValue: Int { 0 }
}
struct SR_15940 {
  @SR_15940Foo var a: Int
}

[hank121314]

I have added a test to Concurrency/global_actor_inference.swift to make sure actor isolation and effects checking is performed in the second situation as mentioned above.
As for the first situation, tests are added in Sema/property_wrappers.swift and SILGen/property_wrappers.swift.
Please let me know if anything is missing.

Thanks for the review!

[hborla]

Thank you!

[hborla]

@swift-ci please smoke test

[hborla]

@swift-ci please test source compatibility