[CodeGen Protobuf support] SwiftProtobuf parser to CodeGenerationRequest #1772

stefanadranca · 2024-01-24T14:50:54Z

Motivation:

We need the parser to transform a FileDescriptor object into a CodeGenerationRequest objet which can be used as input in the new code generation process.

Modifications:

Created the parser struct
Created a test for the type

Result:

FileDescriptors can now be transformed into CodeGenerationRequest, which will
be useful in implementing the protoc-gen-grpc-swift v2.

Motivation: We need the parser to transform a FileDescriptor object into a CodeGenerationRequest objet which can be used as input in the new code generation process. Modifications: - Created the parser struct - Created a test for the type Result: FileDescriptors can now be transformed into CodeGenerationRequest, which will be useful in implementing the `protoc-gen-grpc-swift` v2.

stefanadranca · 2024-01-24T15:26:11Z

Sources/GRPCProtobuf/InputParser.swift

+    var documentation = String()
+    // Field number used to collect .proto file comments.
+    let documentationPath = IndexPath(index: 12)
+    if let syntaxLocation = input.sourceCodeInfoLocation(path: documentationPath) {
+      documentation = syntaxLocation.asSourceComment(
+        commentPrefix: "///",
+        leadingDetachedPrefix: "//"
+      )
+      // If the was a leading or tailing comment it won't have a blank
+      // line, after it, so ensure there is one.
+      if !documentation.isEmpty && !documentation.hasSuffix("\n\n") {
+        documentation.append("\n")
+      }


This part is taken from here https://github.com/apple/swift-protobuf/blob/ea9ac1c4831d14118372ce7e1dcebd142c74e7f3/Sources/protoc-gen-swift/FileGenerator.swift#L71. Apparently there are fixed path numbers for different things that appear in a proto file - 12 is for the header. Should I add to NOTICES that I've used this small piece of code from SwiftProtobuf or should I add some link in the comment above the line containing the index?

glbrntt · 2024-01-25T10:25:34Z

Sources/GRPCProtobuf/InputParser.swift

@@ -0,0 +1,122 @@
+/*


This shouldn't be in this module, this module is for protobuf components which are used at runtime (i.e. when running a client or server). This is used at code generation time so should be in its own module.

glbrntt · 2024-01-25T10:26:30Z

Sources/GRPCProtobuf/InputParser.swift

+import struct GRPCCodeGen.CodeGenerationRequest
+
+/// Parses a ``FileDescriptor`` object into a ``CodeGenerationRequest`` object.
+public struct InputParser {


I think we can use a more specific name here.

glbrntt · 2024-01-25T10:28:26Z

Sources/GRPCProtobuf/InputParser.swift

+    var header = String()
+    // Field number used to collect the .proto file header.
+    let headerPath = IndexPath(index: 12)
+    if let headerLocation = input.sourceCodeInfoLocation(path: headerPath) {
+      header = headerLocation.asSourceComment(
+        commentPrefix: "///",
+        leadingDetachedPrefix: "//"
+      )
+      // If the was a leading or tailing comment it won't have a blank
+      // line, after it, so ensure there is one.
+      if !header.isEmpty && !header.hasSuffix("\n\n") {
+        header.append("\n")
+      }
+    }


I think this would make more sense as an extension on FileDescriptor

glbrntt · 2024-01-25T10:29:09Z

Sources/GRPCProtobuf/InputParser.swift

+      // DO NOT EDIT.
+      // swift-format-ignore-file
+      //
+      // Generated by the Swift generator plugin for the protocol buffer compiler.
+      // Source: \(fileName)
+      //
+      // For information on using the generated types, please see the documentation:
+      //   https://github.com/apple/swift-protobuf/\n\n


This is from Swift Protobuf and points to the protobuf docs which isn't right.

glbrntt · 2024-01-25T10:29:41Z

Sources/GRPCProtobuf/InputParser.swift

+    let dependencies = input.dependencies.map {
+      CodeGenerationRequest.Dependency(module: $0.name)
+    }


The dependencies should include GRPCProtobuf as well because that's where the serializer/deserializer are imported from.

glbrntt · 2024-01-25T10:30:57Z

Sources/GRPCProtobuf/InputParser.swift

+    )
+  }
+
+  fileprivate func createCodeGenerationServices(


nit: use make instead of create (https://www.swift.org/documentation/api-design-guidelines/#strive-for-fluent-usage), also this creates service descriptors, not services

In general though, I think this is worth moving these to be conversion functions on the init of the descriptors, it makes them much easier to test in isolation:

extension CodeGenerationRequest.ServiceDescriptor { init(descriptor: ServiceDescriptor, package: String) { // ... } } extension CodeGenerationRequest.ServiceDescriptor.MethodDescriptor { init(descriptor: ServiceDescriptor.MethodDescriptor) { // ... } }

Then this function boils down to something like:

let descriptors = input.services.map { descriptor in CoreGenerationRequest.ServiceDescriptor(descriptor: descriptor, package: package) }

glbrntt · 2024-01-25T10:43:19Z

Tests/GRPCProtobufTests/InputParserTests.swift

+      // Copyright 2015 gRPC authors.\n//\n// Licensed under the Apache License, \
+      Version 2.0 (the \"License\");\n// you may not use this file except in \
+      compliance with the License.\n// You may obtain a copy of the License \
+      at\n//\n//     http://www.apache.org/licenses/LICENSE-2.0\n//\n// Unless required by \
+      applicable law or agreed to in writing, software\n// distributed under \
+      the License is distributed on an \"AS IS\" BASIS,\n// WITHOUT WARRANTIES \
+      OR CONDITIONS OF ANY KIND, either express or implied.\n// See the License \
+      for the specific language governing permissions and\n// limitations under the License.\n\n


I think this somewhat misses the point of multiline string literals..., you don't need to put in "\n" manually, you just write the text as you'd like it. The \ at the end of each line stops the newline being inserted.

See https://docs.swift.org/swift-book/documentation/the-swift-programming-language/stringsandcharacters#Multiline-String-Literals

glbrntt · 2024-01-25T10:44:01Z

Tests/GRPCProtobufTests/InputParserTests.swift

+    )
+  }
+
+  var dummyFileDescriptor: FileDescriptor {


It's not really a dummy file descriptor, it's the hello world file descriptor

glbrntt · 2024-01-25T10:44:47Z

Tests/GRPCProtobufTests/InputParserTests.swift

+    XCTAssertEqual(
+      parsedCodeGenRequest.services[0].methods[0].documentation,
+      "/// Sends a greeting.\n"
+    )


Can we pull out the first service? I.e. guard let service = request.services.first else { return XCTFail() }

Also we should validate the number of services and methods is as expected

glbrntt · 2024-01-26T11:04:33Z

Package.swift

  static let reflectionService: Self = .target(name: "GRPCReflectionService")
  static let grpcCodeGen: Self = .target(name: "GRPCCodeGen")
  static let grpcProtobuf: Self = .target(name: "GRPCProtobuf")
+  static let grpcProtobufParser: Self = .target(name: "GRPCProtobufParser")


Can we call this GRPCProtobufCodeGen -- it will do more than parsing

glbrntt · 2024-01-26T11:05:08Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+      // DO NOT EDIT.
+      // swift-format-ignore-file
+      //
+      // Generated by the Swift generator plugin for the protocol buffer compiler.


Suggested change

// Generated by the Swift generator plugin for the protocol buffer compiler.

// Generated by the gRPC Swift generator plugin for the protocol buffer compiler.

glbrntt · 2024-01-26T11:05:36Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+      // Source: \(input.name)
+      //
+      // For information on using the generated types, please see the documentation:
+      //   https://github.com/grpc/grpc-swift\n\n


Suggested change

// https://github.com/grpc/grpc-swift\n\n

// https://github.com/grpc/grpc-swift

```

glbrntt · 2024-01-26T11:07:31Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+}
+
+extension FileDescriptor {
+  fileprivate func getHeader() -> String {


If you have get in your name you probably want a computed property instead...

glbrntt · 2024-01-26T11:08:53Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+      // Ensuring there is a blank line after the header.
+      if !header.isEmpty && !header.hasSuffix("\n\n") {
+        header.append("\n")
+      }


This logic is separate from getting the header, looks like it should be in parse

glbrntt · 2024-01-26T11:11:30Z

Tests/GRPCProtobufParserTests/ProtobufToCodeGenParserTests.swift

+      //     http://www.apache.org/licenses/LICENSE-2.0
+      //
+      // Unless required by applicable law or agreed to in writing, software
+      // distributed under the License is distributed on an \"AS IS\" BASIS,


Suggested change

// distributed under the License is distributed on an \"AS IS\" BASIS,

// distributed under the License is distributed on an "AS IS" BASIS,

glbrntt · 2024-01-26T11:12:14Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+
+    return CodeGenerationRequest(
+      fileName: input.name,
+      leadingTrivia: leadingTrivia + header,


This should be the other way around, any copyright header etc. should come first

glbrntt · 2024-01-26T11:12:36Z

Tests/GRPCProtobufParserTests/ProtobufToCodeGenParserTests.swift

+
+

I don't think we want all this whitespace in the leading trivia.

glbrntt · 2024-01-26T11:15:31Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+import struct GRPCCodeGen.CodeGenerationRequest
+
+/// Parses a ``FileDescriptor`` object into a ``CodeGenerationRequest`` object.
+public struct ProtobufToCodeGenParser {


Given we now have this extra module we can put more of the logic in it when constructing protoc-gen-grpc-swift, and have protoc-gen-grpc-swift basically do nothing apart from parsing the options and calling the generator we provide in this module. The upshot of this is that I don't think this type needs to be public.

glbrntt · 2024-01-26T11:15:59Z

Sources/GRPCProtobufParser/ProtobufToCodeGenParser.swift

+    let services = input.services.map {
+      CodeGenerationRequest.ServiceDescriptor(descriptor: $0, package: input.package)
+    }


This whole flow is much easier to follow!

Package.swift

…est (grpc#1772) Motivation: We need the parser to transform a FileDescriptor object into a CodeGenerationRequest objet which can be used as input in the new code generation process. Modifications: - Created the parser struct - Created a test for the type Result: FileDescriptors can now be transformed into CodeGenerationRequest, which will be useful in implementing the `protoc-gen-grpc-swift` v2. --------- Co-authored-by: George Barnett <gbarnett@apple.com>

stefanadranca added 5 commits January 24, 2024 14:25

fixed files added by mistake

694b2bc

deleted files

7b52658

import Foundation only

a4c9886

solved Package issue

10813c8

stefanadranca commented Jan 24, 2024

View reviewed changes

stefanadranca and others added 2 commits January 24, 2024 15:28

naming fix

396fa77

Merge branch 'main' into sd-protobuf-parser

7f394bc

stefanadranca requested a review from glbrntt January 24, 2024 16:03

stefanadranca marked this pull request as ready for review January 24, 2024 16:33

stefanadranca requested a review from gjcairo January 24, 2024 16:34

glbrntt requested changes Jan 25, 2024

View reviewed changes

stefanadranca and others added 4 commits January 25, 2024 16:51

implemented feedback

d3af5e7

Merge branch 'main' into sd-protobuf-parser

d3d2796

added link to proto

00ab710

small fixtures

530f145

stefanadranca requested a review from glbrntt January 25, 2024 17:18

glbrntt requested changes Jan 26, 2024

View reviewed changes

stefanadranca and others added 2 commits January 26, 2024 13:53

implemented feedback

97135c9

Merge branch 'main' into sd-protobuf-parser

06c21fc

glbrntt approved these changes Jan 26, 2024

View reviewed changes

Package.swift Outdated Show resolved Hide resolved

Update Package.swift

918f2b1

stefanadranca merged commit 373d9f4 into grpc:main Jan 29, 2024

glbrntt added the version/v2 Relates to v2 label Feb 8, 2024

	// Generated by the Swift generator plugin for the protocol buffer compiler.
	// Generated by the gRPC Swift generator plugin for the protocol buffer compiler.

	// distributed under the License is distributed on an \"AS IS\" BASIS,
	// distributed under the License is distributed on an "AS IS" BASIS,

[CodeGen Protobuf support] SwiftProtobuf parser to CodeGenerationRequest #1772

[CodeGen Protobuf support] SwiftProtobuf parser to CodeGenerationRequest #1772

Uh oh!

Conversation

stefanadranca commented Jan 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants