Could you add advanced/experimental option to ignore default initializer ( and to enable Memberwise Initializers )?

[lolgear]

Hi!
Could you add support for swift 5.1?

By the way, how could I disable all checks for swift 4.2 in generated proto files?
I would like to have only latest swift version in generated proto files.

Thanks!

[thomasvl]

Are you seeing issues with Swift 5.1? If so, what?

The checks in the code for older swift versions are harmless, they will compile away. It keeps thing simpler to generate the support for our documented support tool range instead of making all those things conditional at generation time.

[lolgear]

@thomasvl Fast, thanks!

First of all, generated files don't use Swift .init() syntax everywhere. It would be nice to have this option.

  var page: MyPage {
    get {
      if case .page(let v)? = _storage._content {return v}
      return MyPage() // .init() <- more readable.
    }
  }

Next, it would be nice to have structs with auto-generated memberwise initializers:

// Proto file.
struct MyPage {
  var id: String = String() // .init() more readable.
  init()
}

My options are

protoc -I ./ --swift_opt=FileNaming=DropPath --swift_opt=Visibility=Internal --swift_out=./protobuf

Yes, I know, that Visibility level is Internal, but I would like to have additional option that will hide all memberwise initializers.

As I remember, memberwise initializers have new "outfit/behavior" in Swift 5.1, right?

When I try to .init MyPage struct, it doesn't suggest memberwise initializers.

// Generate data.
class Create {
  func page() -> MyPage {
    .init() // no memberwise initializers are available.
  }
}

[lolgear]

Ah, I see.

swift-protobuf/Sources/protoc-gen-swift/MessageGenerator.swift

Lines 156 to 161 in 1f798cf

	// Generate the default initializer. If we don't, Swift seems to sometimes
	// generate it along with others that can take public proprerties. When it
	// generates the others doesn't seem to be documented.
	p.print(
	"\n",
	"\(visibility)init() {}\n")

Could you add option to ignore default initializer? Yes, it is undocumented, but, let's assume this functionality is only for advanced users. If anything goes wrong, it is only their problem.

[lolgear]

@thomasvl
Could you tell why this decision was accepted?
I would like to have an option to ignore default initializer as advanced/experimental option. It seems natural for language that have annual updates with syntax and sugar features.

[allevato]

First of all, generated files don't use Swift .init() syntax everywhere. It would be nice to have this option.

 var page: MyPage {
   get {
     if case .page(let v)? = _storage._content {return v}
     return MyPage() // .init() <- more readable.
   }
 }

It's debatable whether using the implicit member reference notation for initializers is more readable than using the more common explicit type name form.

But more importantly, why are you that concerned about minor syntactic details of the implementation of generated code? The important details are in the API, not in the implementation. There's no benefit to supporting maintaining multiple options and code paths that allow users to change the syntax of generated code but with no semantic difference.

Regarding memberwise initializers, there's more discussion about why they aren't supported here: #86

[lolgear]

@allevato

But more importantly, why are you that concerned about minor syntactic details of the implementation of generated code? The important details are in the API, not in the implementation.

• Why do you try to format generated code?
  Compiler doesn't care if you write everything in one line.
• Why do you add indentation for nested structures?
  Compiler doesn't care about it too.
• Why do you choose sane names for parameters in generated code?
  Again, Compiler doesn't care.

  mutating func decodeMessage<D: SwiftProtobuf.Decoder>(decoder: inout D) throws {/*...*/}

All of these questions have one answer - because you care about person who opens .pb.swift file.
Am I wrong?

[allevato]

None of those cases are analogous. You've chosen to ignore the word "minor" in the phrase "minor syntactic details".

It's very important that the user be able to read the API of the generated messages so that they understand how to use them. Writing all the code on one line, not using indentation, or not choosing sane names for parameters would make that more difficult.

On the other hand, the choice of explicit TypeName() vs. implied .init() in the implementation of the generated code is an extremely minor syntactic point, and you've offered no evidence to your claim that the latter is more readable nor a significant reason to motivate the change.

[thomasvl]

Re the initializers – proto file authors are free reorder/insert fields without an impact on the wire. If we start generating or allowing generation of initializers that latch on to the order, then it effectively becomes a place a non breaking .proto file change could break a users swift code. And I wouldn't call initializers "advanced" as after all Swift recent went out of the way to try a generate these in more simple cases. This breaking pattern is what went into the decision to do with with support instead, as it would keep regenerating from an updated .proto file from breaking someones existing code.

[lolgear]

@allevato

What I see:

struct Error {
  // SwiftProtobuf.Message conformance is added in an extension below. See the
  // `Message` and `Message+*Additions` files in the SwiftProtobuf library for
  // methods supported on all messages.

  var code: Error.Code = .null // First style

  var description_p: String = String() // Second style

  var unknownFields = SwiftProtobuf.UnknownStorage() // Third style
}

  // clumsy, but now it has one style and I see all variables.
  var code = Error.Code.null
  var description_p = String()
  var unknownFields = SwiftProtobuf.UnknownStorage()

   // hm, variables and their types and don't care about their `init` except enum.
  var code: Error.Code = .null
  var description_p: String = .init()
  var unknownFields: SwiftProtobuf.UnknownStorage = .init()

In last example you see pair on left ( variable and type ) and something on right. If statements on right are the same, you just skip them and focus on left part.

@thomasvl
You have a context of current protobuf message scheme. Person on other side only have a result in Swift.
Building custom parsers or keep up-to-date forks of this repository on GitHub is exhausting. Using custom plugin for protoc is also something everybody wants to avoid.

Even this may help ( all fields except unknownFields initializer ).

// it can be convenience, of course.
extension Error {
  init(code: Error.Code, description_p: String) {/*...*/}
}

[thomasvl]

Your convenience case is what would break if a new field got added to the message and it suddenly inserted an arg inbetween code and description_p.

As far as your what I see case, that is one possible generation, depending on the message, it might generate using storage (in part for memory or because it has to because the message has a field of its own type).

[lolgear]

Your convenience case is what would break if a new field got added to the message and it suddenly inserted an arg inbetween code and description_p.
Sure, let it go.

After running script update_to_recent_api.sh I will expect that something may change. Moreover, I will expect that current data/user workflow may change and it requires efforts to keep them up-to-date.
But without it I even don't know that new field (with default value) is added.

[thomasvl]

Not sure what update_to_recent_api.sh.

At the moment, this library has the goal of never breaking existing code on a regeneration if the proto files have also made no breaking changes. Protocol buffers defines the addition of a new field as non breaking as a server would still have to work for old clients that don't know about the field either.

[lolgear]

@thomasvl
Oh, non-breaking changes...
Could you add examples "How to generate convenience memberwise initializers in extension?" via swift-protobuf patching?
Also, could you add to Readme.md or to Internals.md that convenience initializers are not supported in case of non-breaking changes?

[Jasperav]

I support the idea of having initializers that take all fields expect for the unknownFields property. If I add/remove a field, I get a compile time error instead of unexpected values server side. I now do everything with the with method, but I rather have stricter checking (hence that the initializer makes sure you covered all the fields).

[thomasvl]

Again, adding a field is defined as non breaking from protobuf pov.

instead of unexpected values server side

This will never happen. Protobuf has defined behaviors around default values and for proto2 syntax the there is also the has properties to check if things are set. It is all part of the spec so there are no unexpected things.

[lolgear]

@Jasperav
Look at
#86 (comment)
I've added Memberwise Initializer generator via SwiftRewriter.

[Jasperav]

@lolgear not sure why I can not add an issue to the repo, but I get an error running this command:

swift run swift-rewriter run --path ./image_identifier.pb.swift

error:

Error Domain=com.arguments Code=-100 "File not exists or path is nil! RunOptions(path: "./image_identifier.pb.swift", debug: false, idempotent: false, outputPath: Optional(""))" UserInfo={NSLocalizedDescription=File not exists or path is nil! RunOptions(path: "./image_identifier.pb.swift", debug: false, idempotent: false, outputPath: Optional(""))}

I placed image_identifier.pb.swift in the root directory of the project itself

[Jasperav]

@thomasvl I understand that adding a field should be nonbreaking, in the sense of the api should still work if users are using an older version. IMO implementing the api should/can act differently. I don't see a reason why I should not get a compile time error in Swift when I add a new field in Protobuf, because why would I add that field anyway? Adding a new field in protobuf always results in me changing Swift code because I want to use that field. But I am sure this discussion has already been debated.

[thomasvl]

@Jasperav I think you might be focused a little specifically on your exact use case of protobuf.

If protos are used to define a the data models for taking to a server, that service might add some new fields as at some point, but that doesn't mean all clients need to use those fields. For example, say a proto is used to define some server for hosting/syncing a users' contacts. It starts with an initial set of fields (first, middle, last, title, emails, phone numbers, ...); but say a year later they add some more fields (nickname, family members, etc.). A client is going to be creating these message for new contacts their users create, they do not need to set all these new fields because they might never show the data for their specific usage. Forcing that client that happens to talk to this service to update because the service supported some more fields doesn't really make sense, and worse, a developer might start passing dummy values that then get persisted as if the user set them.

Now in your case, you might be the sole consumer of your protos from a client/server pov, and in those cases, your specific needs might include that your clients also have to start adding what ever your new fields are; but from a generic pov, in all the ways protos can be used, and especially where one team/company/etc. might publish protos for any client to use, nothing says all those fields are needed by everyone.