[Sema] Synthesize default values for memberwise init

[Azoy]

Previously we were synthesizing something like the following:

struct Dog {
  var age = 0
  var name = ""

  // compiler synthesized
  init(age: Int, name: String)

  // compiler synthesized
  init() {}
}

This patch is a QoL change to the way we synthesize the memberwise constructor for structures.

struct Dog {
  var age = 0
  var name = ""

  // compiler synthesized
  init(age: Int = 0, name: String = "")
}

Swift Evolution review: https://forums.swift.org/t/se-0242-synthesize-default-values-for-the-memberwise-initializer/20618

cc: @jrose-apple @harlanhaskins

[slavapestov]

@Azoy Instead of moving the initializer into a helper function, a different strategy would be to add a new DefaultArgumentKind that simply points at the VarDecl.

VarDecls with an initial value already emit a generator function in SILGen, and this function can itself be used as the default argument for the constructor.

This way you avoid emitting a helper function entirely.

I would prefer this strategy because it should be simpler to implement and is less error prone (imagine if in the future we add another type of Expr that requires special handling here, we'd have to remember to update it).

[Azoy]

@slavapestov what value can I give the parameter for 1. the type checker to be happy and 2. be able to call the variable initializer?

[slavapestov]

@Azoy Sorry, I'm not sure what you mean by "parameter"

[Azoy]

The parameter for the constructor.

struct X {
  var y = 0
  // synthesized
  init(y: Int = 0)
}

In this example the parameter being y: Int = 0. I guess we don't have to explicitly give the ParamDecl a defaultValue, but just set the default argument kind to the new kind (maybe implicit). I assume I have to special case this default argument in the type checker, but once we need to emit the default argument in silgen find the variable initializer and call it. But how would I be able to point back to the VarDecl though?

[slavapestov]

I guess we don't have to explicitly give the ParamDecl a defaultValue, but just set the default argument kind to the new kind (maybe implicit).

Yes, the 'default value' is an Expr only used with DefaultArgumentKind::Normal. I would add a new DefaultArgumentKind::StoredProperty (or something similar).

I assume I have to special case this default argument in the type checker,

I think you want to explicitly construct a ParamDecl with this DefaultArgumentKind when you synthesize the memberwise initializer. However when calling a memberwise initializer, nothing needs to be special cased I think -- Sema doesn't really care about the presence of the expression when calling the initializer.

but once we need to emit the default argument in silgen find the variable initializer and call it. But how would I be able to point back to the VarDecl though?

Add the VarDecl to struct StoredDefaultArgument. Perhaps change the Expr * field to a PointerUnion<Expr *, VarDecl *>. In SILGen, when emitting the default argument, handle the stored property case by emitting a call to the stored property's generator function instead of the default argument generator function. Take a look at DelayedArgument::emitDefaultArgument().

[Azoy]

@slavapestov Apologies if something here isn't correct, I'm new to the SILGen codebase, but I think for the most part this looks ok? You were right about Sema not caring about the presence of the expression, so that was a reliever!

struct X {
  var y = 0
}

The memberwise constructor's default argument looks like this:

// default argument 0 of X.init(y:)
sil hidden @$s4test1XV1yACSi_tcfcfA_ : @$convention(thin) () -> Int {
bb0:
  // function_ref variable initialization expression of X.y
  %0 = function_ref @$s4test1XV1ySivpfi : @$convention(thin) () -> Int // user: %1
  %1 = apply %0() : @$convention(thin) () -> Int //user: %2
  return %1 : $Int
} // end sil function '$s4test1XV1yACSi_tcfcfA_'

Currently the stored property kind is printed as the variable name, so @jrose-apple or @harlanhaskins might have some input on how to properly print it.

[slavapestov]

I’ll take a look tomorrow. I don’t think it matters how we print these default arguments since memberwise initializers are never public.

Also your PR needs tests :)

[Azoy]

Yes, tests will be added shortly!

[Azoy]

@slavapestov I've run into some interesting behavior with optionals concerning with the creation of the implicit constructor(s).
Swift 4.2:

struct X {
  var y: Int?
  // Compiler synthesized
  init(y: Int?)
  // Compiler synthesized
  init() { return }
}

let x = X()

Would it make sense to assign the default value of y: Int? in the memberwise constructor to nil?

struct X {
  var y: Int?
  // Compiler synthesized
  init(y: Int? = nil)
}

let x = X()

[slavapestov]

@Azoy, yes, properties with optional type have a "default" initial value of nil. I suggested to swift-evolution once to remove this edge case, but there was push back so we're stuck with it. :)

[Azoy]

I'm having some trouble with some of the SIL being generated for the default argument for some types.
Given:

import Foundation
struct X {
  var y = Data(bytes: [])
}

Default argument function:

SIL verification failed: entry point has wrong number of arguments: entry->args_size() == (fnConv.getNumIndirectSILResults() + fnConv.getNumParameters())
In function:
// default argument 0 of X.init(y:)
sil hidden @$s6REPL_11XV1yAC10Foundation4DataV_tcfcfA_ : $@convention(thin) () -> @out Data {
bb0:
  // function_ref variable initialization expression of X.y
  %0 = function_ref @$s6REPL_11XV1y10Foundation4DataVvpfi : $@convention(thin) () -> @out Data // user: %2
  %1 = alloc_stack $Data                          // users: %3, %2
  %2 = apply %0(%1) : $@convention(thin) () -> @out Data
  return %1 : $*Data                              // id: %3
} // end sil function '$s6REPL_11XV1yAC10Foundation4DataV_tcfcfA_'

I see what it's trying to do with ResultPlan noticing the indirect @out Data and wanting to create a temporary and return that, but obviously its tripping over trying to apply the temporary to the variable initializer with 0 arguments. Am I setting up the emitApplyOfStoredPropertyInitializer incorrectly?

[Azoy]

@slavapestov I'm running tests locally and I'm getting some weird behavior with some structures not wanting to print their stored property. For example, in test/attr/accessibility_print.swift:29 we have:

// CHECK-LABEL: private{{(\*/)?}} struct BB_PrivateStruct {
private struct BB_PrivateStruct {
  // CHECK: internal var x
  var x = 0
  // CHECK: internal init(x: Int)
  // CHECK: internal init()
} // CHECK: {{^[}]}}

It complains that it couldn't find the first init, but it suggests internal init(x: Int = x) right, but if I change the check to // CHECK: internal init(x: Int = x) then it asserts on ParamDecl.getDefaultValueStringRepresentation with the default value pointer not being set yet.

[Azoy]

@harlanhaskins Could you take a look at the latest commit? I haven't run validation tests yet, but the only failing test I'm getting is with SILOptimizer/exclusivity_static_diagnostics_inlined.swift error: cannot use instance member 'someV' as a default parameter. I'm not entirely sure how I'm getting that diagnostic, but I think its because we're printing the instance member name as a the default text.

[Azoy]

@slavapestov is there a specific test file for the memberwise constructor that I'm missing, or is it some other test file?

[natecook1000]

@Azoy This looks great, glad to see it coming up for review! Would you mind resolving the conflict? I'd like to build a toolchain with this feature.

[Azoy]

@natecook1000 done and done!

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c679df6a3796d654f4aef2bc4a0096b42b5e1d9b

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c679df6a3796d654f4aef2bc4a0096b42b5e1d9b

[slavapestov]

Sorry to come back with more review feedback at this stage; I also missed the 'lazy' case and I want to make sure we get it right because 'lazy' has tripped all of us up before.

[slavapestov]

... in fact this already appears to correctly handle the lazy property case. Maybe just remove the areLazyAccessorsSynthesized check above and see what happens?

Also, can you restructure this code a bit so that setDefaultArgumentKind() is only called once? It's a bit confusing to see it overwritten later. I'm guessing you don't really need to call setStoredProperty() either in the NilLiteral case?

[Azoy]

@slavapestov ok, I think this is the one! I made it so that we check for NilLiteral cases before we set stored property and just return if it's the case.

[slavapestov]

The new version looks great! I still have a small cleanup suggestion, and one concern related to serialization with -enable-testing but I think we should merge this now, and address the below in a follow-up PR.

[slavapestov]

Does this work across modules? Normally you can’t call a memberwise initializer since they’re internal, but if a module is built with -enable-testing and imported with @testable import you can. I might be wrong but I don’t see ParamDecl::getStoredProperty() serialized anywhere.

[Azoy]

I tried to spin up a quick test to test the current behavior, and I don't believe that we currently deserialize any default value from a module, but rather just the arg kind and the string representation. I'd be curious to hear how we could go about setting the stored property arg after we deserialize (maybe just take param name and look for the corresponding property?). Right now this emits an apply to the default arg generator (which there is none, so I believe the linker will complain...).

[slavapestov]

I think the right solution is to change the code they serializes a ParamDecl to serialize a cross reference to the corresponding VarDecl. I would try to avoid a name-based lookup here. @jrose-apple might have more suggestions

[jrose-apple]

I think emitting an apply to the default arg generator is the correct behavior. The fact that the expression comes from a stored property shouldn't matter except when generating the function.

[Azoy]

Emitting a default arg generator in this case doesn't work because the module with the stored property default arg doesn't emit a default arg generator function. So when we go to deserialize a stored property default arg, if we emit a default arg generator apply then we're going to be linking against undefined symbols.

[jrose-apple]

Emitting a default arg generator in this case doesn't work because the module with the stored property default arg doesn't emit a default arg generator function.

Hm. Why doesn't it? Isn't this a bug? (under -enable-testing, anyway)

[slavapestov]

@Azoy do you mind updating the PR description now that the proposal has been accepted and the implementation has moved away from synthesizing closure?

[slavapestov]

@swift-ci Please test source compatibility

[slavapestov]

@swift-ci Please test

[slavapestov]

Looks like CI is having problems. I'm on vacation until Tuesday; @DougGregor or @tkremenek will help you get this merged in the mean time.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - dcedba7

[natecook1000]

@swift-ci Please test Linux platform

[tkremenek]

@slavapestov has approved, so I am merging.

[tkremenek]

@Azoy: Please create a PR against swift-5.1-branch to pull this into 5.1

[tkremenek]

I just noticed the recent commentary on this PR. I think this is OK to merge now for incremental development, but let's address the remaining concerns in a separate PR. A 5.1 PR can contain changes from both PRs.

[rintaro]

When creating PR for swift-5.1-branch for this, please include #23564 as well.

[tkremenek]

@Azoy: is there a 5.1 PR for this yet?

[Azoy]

@tkremenek No, sorry not yet. I'm working with Jordan and Slava to get #23648 merged. Once that's merged (which is hopefully tonight), I will submit a 5.1 PR.

[tkremenek]

@Azoy #23648 has been merged