Unverified Commit 66692b35 authored by Fahad Zubair's avatar Fahad Zubair Committed by GitHub
Browse files

Merge documentation of same inline modules (#4008) 



When two inline modules with the same name are generated from different
parts of the codebase, their documentation should be merged. However,
other metadata must match exactly, as it is an error for one part of the
codebase to define an inline module with pub visibility while another
defines it with pub(crate) visibility.

This PR enables documentation merging while maintaining strict
validation of other metadata fields.

Currently, the following sample model fails to generate code because
both `SomeList` and `member` are generated in the same inline module but
with different doc comments:

```smithy
@documentation("Outer constraint has some documentation")
@length(max: 3)
list SomeList {
    @length(max: 8000)
    member: String
}
```

Co-authored-by: default avatarFahad Zubair <fahadzub@amazon.com>
parent 77e80622

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/RustCrateInlineModuleComposingWriter.kt

+59 −10
Original line number Diff line number Diff line
@@ -352,23 +352,72 @@ class InnerModule(private val moduleDocProvider: ModuleDocProvider, debugMode: B 
        inlineModuleList: MutableList<InlineModuleWithWriter>,

        lookForModule: RustModule.LeafModule,

    ): RustWriter {

        val inlineModuleAndWriter =

            inlineModuleList.firstOrNull {

                it.inlineModule.name == lookForModule.name

            }

        return if (inlineModuleAndWriter == null) {

        val index = inlineModuleList.indexOfFirst { it.inlineModule.name == lookForModule.name }



        return if (index == -1) {

            val inlineWriter = createNewInlineModule()

            inlineModuleList.add(InlineModuleWithWriter(lookForModule, inlineWriter))

            inlineWriter

        } else {

            check(inlineModuleAndWriter.inlineModule == lookForModule) {

                """The two inline modules have the same name but different attributes on them:

                1) ${inlineModuleAndWriter.inlineModule}

                2) $lookForModule"""

            }



            inlineModuleAndWriter.writer

        }

            val existing = inlineModuleList[index]



            // Verify everything except documentation matches.

            val existingModule = existing.inlineModule

            check(

                existingModule.name == lookForModule.name &&

                    existingModule.rustMetadata == lookForModule.rustMetadata &&

                    existingModule.parent == lookForModule.parent &&

                    existingModule.tests == lookForModule.tests,

            ) {

                """

                An inline module with the same name `${lookForModule.name}` was earlier created with different attributes:

                1) Metadata:

                    `${existingModule.rustMetadata}`

                    `${lookForModule.rustMetadata}`

                2) Parent:

                    `${existingModule.parent}`

                    `${lookForModule.parent}`

                3) Tests:

                    `${existingModule.tests}`

                    `${lookForModule.tests}`

                4) DocumentationOverride:

                    `${existingModule.documentationOverride}`

                    `${lookForModule.documentationOverride}`

                """

            }



            // Merge documentation.

            val mergedDoc =

                mergeDocumentation(existingModule.documentationOverride, lookForModule.documentationOverride)



            // Replace the element in the list with merged documentation.

            inlineModuleList[index] =

                InlineModuleWithWriter(

                    existingModule.copy(documentationOverride = mergedDoc),

                    existing.writer,

                )



            existing.writer

        }

    }



    // @VisibleForTesting

    internal fun mergeDocumentation(

        existingModule: String?,

        lookForModule: String?,

    ): String? {

        val mergedDoc =

            when {

                existingModule == null && lookForModule == null -> null

                existingModule == null -> lookForModule

                lookForModule == null -> existingModule

                else ->

                    listOf(

                        existingModule.trim(),

                        lookForModule.trim(),

                    ).joinToString("\n")

            }

        return mergedDoc?.trim()

    }



    private fun writeDocs(innerModule: RustModule.LeafModule) {

codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ConstraintsMemberShapeTest.kt

+36 −1
Original line number Diff line number Diff line
@@ -14,7 +14,10 @@ import software.amazon.smithy.model.SourceLocation 
import software.amazon.smithy.model.shapes.ShapeId

import software.amazon.smithy.model.traits.RequiredTrait

import software.amazon.smithy.model.traits.Trait

import software.amazon.smithy.rust.codegen.core.rustlang.RustModule

import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter

import software.amazon.smithy.rust.codegen.core.rustlang.Writable

import software.amazon.smithy.rust.codegen.core.smithy.ModuleDocProvider

import software.amazon.smithy.rust.codegen.core.smithy.transformers.OperationNormalizer

import software.amazon.smithy.rust.codegen.core.testutil.IntegrationTestParams

import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel
@@ -87,6 +90,7 @@ class ConstraintsMemberShapeTest { 
            @pattern("^[g-m]+${'$'}")

            constrainedPatternString : PatternString



            constrainedList : ConstrainedList

            plainStringList : PlainStringList

            patternStringList : PatternStringList

            patternStringListOverride : PatternStringListOverride
@@ -118,6 +122,11 @@ class ConstraintsMemberShapeTest { 
            @range(min: 10, max:100)

            long : RangedInteger

        }

        @length(max: 3)

        list ConstrainedList {

            @length(max: 8000)

            member: String

        }

        list PlainStringList {

            member: String

        }
@@ -285,7 +294,12 @@ class ConstraintsMemberShapeTest { 


    @Test

    fun `generate code and check member constrained shapes are in the right modules`() {

        serverIntegrationTest(sampleModel, IntegrationTestParams(service = "constrainedMemberShape#ConstrainedService")) { codegenContext, rustCrate ->

        serverIntegrationTest(

            sampleModel,

            IntegrationTestParams(

                service = "constrainedMemberShape#ConstrainedService",

            ),

        ) { _, rustCrate ->

            fun RustWriter.testTypeExistsInBuilderModule(typeName: String) {

                unitTest(

                    "builder_module_has_${typeName.toSnakeCase()}",
@@ -347,6 +361,27 @@ class ConstraintsMemberShapeTest { 
        }

    }



    @Test

    fun `merging docs should not produce extra empty lines`() {

        val docWriter =

            object : ModuleDocProvider {

                override fun docsWriter(module: RustModule.LeafModule): Writable? = null

            }

        val innerModule = InnerModule(docWriter, false)

        innerModule.mergeDocumentation("\n\n", "") shouldBe ""

        innerModule.mergeDocumentation(null, null) shouldBe null

        innerModule.mergeDocumentation(null, "some docs\n") shouldBe "some docs"

        innerModule.mergeDocumentation("some docs\n", null) shouldBe "some docs"

        innerModule.mergeDocumentation(null, "some docs") shouldBe "some docs"

        innerModule.mergeDocumentation("some docs", null) shouldBe "some docs"

        innerModule.mergeDocumentation(null, "some docs\n\n") shouldBe "some docs"

        innerModule.mergeDocumentation("some docs\n\n", null) shouldBe "some docs"

        innerModule.mergeDocumentation(null, "some docs\n\n") shouldBe "some docs"

        innerModule.mergeDocumentation("left side", "right side") shouldBe "left side\nright side"

        innerModule.mergeDocumentation("left side\n", "right side\n") shouldBe "left side\nright side"

        innerModule.mergeDocumentation("left side\n\n\n", "right side\n\n") shouldBe "left side\nright side"

    }



    /**

     *  Checks that the given member shape:

     *  1. Has been changed to a new shape