Fix Bugs in Swift Doc-Comment Generation #3122

InsertCreativityHere · 2024-11-11T16:45:29Z

This PR fixes a bunch of small formatting problems in how slice2swift generates doc comments, including all the ones listed in #2274. Here's a list which shows the diffs between the old comments and the new comments (which this PR now generates).

Section titles are now correctly capitalized (Parameters, Returns, and Throws), and we no longer write the type after the name.
Also removed the extra newlines we generated between every parameter. Now we only generate new lines between sections:

     /// Read an optional `ObjectSeq?` sequence from the stream.
     ///
-    /// - parameter istr: `Ice.InputStream` - The stream to read from.
-    ///
-    /// - parameter tag: `Swift.Int32` - The numeric tag associated with the value.
+    /// - Parameter istr: The stream to read from.
+    /// - Parameter tag: The numeric tag associated with the value.
     ///
-    /// - returns: `ObjectSeq` - The sequence read from the stream.
+    /// - Returns: The sequence read from the stream.
     public static func read(from istr: InputStream, tag: Swift.Int32) throws -> ObjectSeq? {

If there are more than 2 parameters, we generate a parameter list, instead of each on one line:

-    /// Wite an optional `ObjectSeq?` sequence to the stream.
+    /// Write an optional `ObjectSeq?` sequence to the stream.
     ///
-    /// - parameter ostr: `Ice.OuputStream` - The stream to write to.
-    ///
-    /// - parameter tag: `Int32` - The numeric tag associated with the value.
-    ///
-    /// - parameter value: `ObjectSeq` The sequence value to write to the stream.
+    /// - Parameters:
+    ///   - ostr: The stream to write to.
+    ///   - tag: The numeric tag associated with the value.
+    ///   - value: The sequence value to write to the stream.
     public static func write(to ostr: OutputStream,  tag: Swift.Int32, value v: ObjectSeq?) {

We document the 'parameter labels' instead of the 'argument label' now:

     /// Write a `Identity` structured value to the stream.
     ///
-    /// - parameter _: `Identity` - The value to write to the stream.
+    /// - Parameter v: The value to write to the stream.
     func write(_ v: Identity) {

Updated the link syntax to use 'ticks' instead of tags.
This is actually still incorrect, because links should use double-ticks. But now that we're actually generating correctly formatted links now, it turns out they're all broken. See: #3120.

-/// A sequence of {@link RegistryInfo} structures.
+/// A sequence of `RegistryInfo` structures.
 public typealias RegistryInfoSeq = [RegistryInfo]

Also removed lots of other extra whitespace in other places we were generating things. And fixed Returns to not emit the type tuple after the section header.

 /// The Ice router interface. Routers can be set either globally though the Communicator, or with
 /// ice_router on specific proxies.
 ///
 /// RouterPrx Methods:
-///
 ///  - getClientProxy: Get the router's client proxy, i.e., the proxy to use for forwarding requests from the client to the router. If a null proxy is returned, the client will forward requests to the router's endpoints.
-///
 ///  - getClientProxyAsync: Get the router's client proxy, i.e., the proxy to use for forwarding requests from the client to the router. If a null proxy is returned, the client will forward requests to the router's endpoints.
-///
 ///  - getServerProxy: Get the router's server proxy, i.e., the proxy to use for forwarding requests from the server to the router.
-///
 ///  - getServerProxyAsync: Get the router's server proxy, i.e., the proxy to use for forwarding requests from the server to the router.
-///
 ///  - addProxies: Add new proxy information to the router's routing table.
-///
 ///  - addProxiesAsync: Add new proxy information to the router's routing table.
 public extension RouterPrx {
     /// Get the router's client proxy, i.e., the proxy to use for forwarding requests from the client to the router.
     /// If a null proxy is returned, the client will forward requests to the router's endpoints.
     ///
-    /// - parameter context: `Ice.Context` - Optional request context.
+    /// - Parameter context: Optional request context.
     ///
-    /// - returns: `(returnValue: ObjectPrx?, hasRoutingTable: Swift.Bool?)`:
-    ///
-    ///   - returnValue: `ObjectPrx?` - The router's client proxy.
-    ///
-    ///   - hasRoutingTable: `Swift.Bool?` - Indicates whether or not the router supports a routing table. If it is supported, the
+    /// - Returns:
+    ///   - returnValue: The router's client proxy.
+    ///   - hasRoutingTable: Indicates whether or not the router supports a routing table. If it is supported, the
     /// Ice runtime will call addProxies to populate the routing table. This out parameter is only supported
     /// starting with Ice 3.7.
     /// The Ice runtime assumes the router has a routing table if the hasRoutingTable is not set.
     func getClientProxy(context: Context? = nil) async throws -> (returnValue: ObjectPrx?, hasRoutingTable: Swift.Bool?) {

InsertCreativityHere · 2024-11-11T16:48:10Z

cpp/src/slice2swift/SwiftUtil.cpp

@@ -154,20 +154,23 @@ namespace
        }
    }

+    // TODO: fix this to emit double-ticks instead of single-ticks once we've fixed all the links.


Swift links should be wrapped in double-quotes, and they use / as a separator, not ..

Previously we weren't emitting links at all, we just wrote the raw-text as is.
This PR doesn't make the full update to use double-ticks, because many of our links are actually broken. See: #3120.

InsertCreativityHere · 2024-11-11T16:49:21Z

cpp/src/slice2swift/SwiftUtil.cpp

@@ -349,29 +352,42 @@ SwiftGenerator::writeDocSummary(IceInternal::Output& out, const ContainedPtr& p)
        return;
    }

+    bool hasStarted = false;


If a doc comment didn't have an overview, but did document other things, we would emit a leading line:

/// /// - Parameter hello: something.

Because we were always assuming an overview was present. This bool lets us only write the separating line when necessary.

InsertCreativityHere · 2024-11-11T16:50:53Z

cpp/src/slice2swift/SwiftUtil.cpp

    auto docParameters = doc->parameters();
-    for (ParamInfoList::const_iterator q = allInParams.begin(); q != allInParams.end(); ++q)
+
+    // Document all the in parameters.


If there would be more than 2 parameters, use the list style instead of the line style.

- Parameter hello: a - Parameter there: b

vs

- Parameters: - hello: a - there: b

InsertCreativityHere · 2024-11-11T16:52:13Z

cpp/src/slice2swift/SwiftUtil.cpp

                StringList opdocOverview = opdoc->overview();
                if (!opdocOverview.empty())
                {
+                    out << ": ";


Don't emit : unless it's actually going to be followed by a comment.

opdoc could be non-null but not contain an overview (if it only documents params/returns without an overview).

InsertCreativityHere · 2024-11-11T16:52:53Z

cpp/src/slice2swift/SwiftUtil.cpp

@@ -623,46 +653,6 @@ SwiftGenerator::writeServantDocSummary(IceInternal::Output& out, const Interface
    }
 }

-void
-SwiftGenerator::writeMemberDoc(IceInternal::Output& out, const DataMemberPtr& p)


Apart from the order that we generated things in, this was identical to writeDocSummary, and only called in one place.
I deleted this one, and now we just call writeDocSummary for everything.

InsertCreativityHere · 2024-11-11T16:53:56Z

cpp/src/slice2swift/Gen.cpp

-    out << nl << "/// Returns the Slice type ID of the interface supported by this object.";
-    out << nl << "///";
-    out << nl << "/// - returns: `String` - The Slice type ID of the interface supported by this object.";
+    out << nl << "/// - Returns: The Slice type ID of the interface supported by this object.";


When the overview and returns section say the exact same thing, it's fine to just have a lone Returns section.

InsertCreativityHere · 2024-11-11T16:55:00Z

cpp/src/slice2swift/Gen.cpp

@@ -1082,7 +1073,9 @@ Gen::ProxyVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
    out << nl << "///    - communicator: The communicator of the new proxy.";
    out << nl << "///    - proxyString: The proxy string to parse.";
    out << nl << "///    - type: The type of the new proxy.";
+    out << nl << "///";


There should be newlines between different sections (Parameters, Throws, and Returns).

InsertCreativityHere · 2024-11-11T16:56:17Z

cpp/src/slice2swift/Gen.cpp

@@ -826,13 +820,13 @@ Gen::TypesVisitor::visitDictionary(const DictionaryPtr& p)
    out << nl << "return v";
    out << eb;

+    out << sp;


We were missing a newline between these two functions.

InsertCreativityHere · 2024-11-11T16:56:36Z

cpp/src/slice2swift/Gen.cpp

@@ -668,13 +664,13 @@ Gen::TypesVisitor::visitSequence(const SequencePtr& p)
    out << nl << "return v";
    out << eb;

+    out << sp;


We were missing a newline between these functions.

InsertCreativityHere · 2024-11-11T16:57:32Z

cpp/src/Slice/Parser.cpp

        while (pos != string::npos)
        {
            string::size_type endpos = comment.find('}', pos);
            if (endpos != string::npos)
            {
                // Extract the linked to identifier.
                string::size_type identStart = comment.find_first_not_of(" \t", pos + link.size());
-                string::size_type identEnd = comment.find_last_not_of(" \t", endpos) + 1;


A bug in my previous comment-formatting PR that caused us to read one character past the link's identifier.

InsertCreativityHere · 2024-11-11T16:58:11Z

cpp/src/Slice/Parser.cpp

@@ -659,15 +659,15 @@ namespace

        // Fix any link tags using the provided link formatter.
        const string link = "{@link ";
-        pos = comment.find(link, pos);


There was no point to passing in pos, it's just 0 here.

InsertCreativityHere · 2024-11-11T16:59:37Z

cpp/src/slice2swift/Gen.cpp

@@ -557,7 +554,7 @@ Gen::TypesVisitor::visitStructStart(const StructPtr& p)

    out << nl << "/// Write a `" << name << "` structured value to the stream.";
    out << nl << "///";
-    out << nl << "/// - parameter _: `" << name << "` - The value to write to the stream.";
+    out << nl << "/// - Parameter v: The value to write to the stream.";


Always document the parameter label, not the argument label.
That's backwards of what you'd expect IMO, but that's how it is.
I guess it's avoid awkward situations like this where there is no argument label?

InsertCreativityHere · 2024-11-11T17:01:53Z

cpp/src/slice2swift/SwiftUtil.cpp

+    // Document the return type & any out parameters.
+    ParamInfoList allOutParams = getAllOutParams(p);
+    useListStyle = allOutParams.size() > 1;
+    if (useListStyle)
    {
-        ParamInfo ret = allOutParams.front();
        out << nl << "///";
-        out << nl << "/// - returns: `" << ret.typeStr << "`";
-        if (p->returnType())
-        {
-            StringList docReturns = doc->returns();
-            if (!docReturns.empty())
-            {
-                out << " - ";
-                writeDocLines(out, docReturns, false);
-            }
-        }
-        else
-        {
-            map<string, StringList>::const_iterator r = docParameters.find(ret.name);
-            if (r != docParameters.end() && !r->second.empty())
-            {
-                out << " - ";
-                writeDocLines(out, r->second, false);
-            }
-        }
+        out << nl << "/// - Returns:";
    }
-    else if (allOutParams.size() > 1)
+
+    if (!allOutParams.empty())
    {
-        out << nl << "///";
-        out << nl << "/// - returns: `" << operationReturnType(p) << "`:";
-        if (p->returnType())
+        // `getAllOutParams` puts the return-type parameter at the end, we want to move it to the front.
+        allOutParams.push_front(allOutParams.back());
+        allOutParams.pop_back();
+
+        // Document each of the out parameters.
+        for (const auto& outParam : allOutParams)
        {
-            ParamInfo ret = allOutParams.back();
-            out << nl << "///";
-            out << nl << "///   - " << ret.name << ": `" << ret.typeStr << "`";
-            StringList docReturns = doc->returns();
-            if (!docReturns.empty())
+            // First, check if the user supplied a message in the doc comment for this parameter / return type.
+            StringList docMessage;
+            if (outParam.param == nullptr) // This means it was a return type, not an out parameter.
            {
-                out << " - ";
-                writeDocLines(out, docReturns, false);
+                docMessage = doc->returns();
+            }
+            else
+            {
+                const auto result = docParameters.find(outParam.name);
+                if (result != docParameters.end())
+                {
+                    docMessage = result->second;
+                }
            }
-        }

-        for (ParamInfoList::const_iterator q = allOutParams.begin(); q != allOutParams.end(); ++q)
-        {
-            if (q->param != 0)
+            if (useListStyle)
            {
-                out << nl << "///";
-                out << nl << "///   - " << q->name << ": `" << q->typeStr << "`";
-                map<string, StringList>::const_iterator r = docParameters.find(q->name);
-                if (r != docParameters.end() && !r->second.empty())
+                out << nl << "///   - " << outParam.name;
+                if (!docMessage.empty())
                {
-                    out << " - ";
-                    writeDocLines(out, r->second, false);
+                    out << ": ";
+                    writeDocLines(out, docMessage, false);
                }
            }
+            else if (!docMessage.empty())
+            {
+                out << nl << "///";
+                out << nl << "/// - Returns: ";
+                writeDocLines(out, docMessage, false);
+            }
        }
    }


This is just a simplification of the code for emitting return doc-comments.
All I changed was the capitalization, and that we no longer emit the types of anything.

InsertCreativityHere added 3 commits November 6, 2024 17:02

Fixed various issues with slice2swift doc comments.

15b6e9e

Minor fixes to other things I found.

7841758

Swift uses '/' as it's doc-comment scope separator.

2e7b762

InsertCreativityHere requested review from externl and bernardnormier November 11, 2024 16:45

InsertCreativityHere commented Nov 11, 2024

View reviewed changes

bernardnormier approved these changes Nov 11, 2024

View reviewed changes

externl approved these changes Nov 11, 2024

View reviewed changes

InsertCreativityHere merged commit 57d604f into zeroc-ice:main Nov 11, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bugs in Swift Doc-Comment Generation #3122

Fix Bugs in Swift Doc-Comment Generation #3122

InsertCreativityHere commented Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

InsertCreativityHere Nov 11, 2024

Fix Bugs in Swift Doc-Comment Generation #3122

Fix Bugs in Swift Doc-Comment Generation #3122

Conversation

InsertCreativityHere commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment