TSP, TCGC common types, use TCGC description instead of getting from raw

[haolingdong-msft]

Fix https://github.com/Azure/autorest.java/issues/2732

Code change are all in code-model-builder.ts: https://github.com/Azure/autorest.java/pull/2776/files?file-filters%5B%5D=.ts&show-viewed-files=true

Generation diff caused by two cases:

1. javadoc on client method: additional description (default description generated by codegen) is added when sdkType.detail is empty. detailed reason: TCGC sdktType.detail will map to code-model schema's description. If detail is null/empty, schema's description will be null/empty as well, and codegen will give a default description like The name parameter., Javadoc is a combination of summary and description, so The name parameter. is added.

    * @param name A sequence of textual characters.
    * 
   ++     * The name parameter.
    * @param requestOptions The options to configure the HTTP request before HTTP client sends it.
    * @throws HttpResponseException thrown if the request is rejected by server.
    * @throws ClientAuthenticationException thrown if the request is rejected by server on status code 401.
    * @throws ResourceNotFoundException thrown if the request is rejected by server on status code 404.
    * @throws ResourceModifiedException thrown if the request is rejected by server on status code 409.
    * @return used in an internal operation, should be generated but not exported along with {@link Response} on
    * successful completion of {@link Mono}.
    */
   @Generated
   @ServiceMethod(returns = ReturnType.SINGLE)
   Mono<Response<BinaryData>> internalDecoratorInInternalWithResponse(String name, RequestOptions requestOptions) {
       return this.serviceClient.internalDecoratorInInternalWithResponseAsync(name, requestOptions);
   }

For this one, I think the default description on client method parameter is not necessary when summary is not empty, @weidongxu-microsoft @XiaofeiCao, please let me know your thoughts, I'm debugging to see where the default is added. Update: https://github.com/Azure/autorest.java/blob/main/javagen/src/main/java/com/azure/autorest/util/MethodUtil.java#L94-L116 -> this is the code where the default description is added.

2. javadoc on model property and getter/setter: scalar type's description get from TCGC, instead of previously generated default one.

   /*
   --     * The configuration property.
   ++     * A sequence of textual characters.
    */
   @Generated
   private String configuration;

    /**
--     * Set the configuration property: The configuration property.
++     * Set the configuration property: A sequence of textual characters.
     * 
     * @param configuration the configuration value to set.
     * @return the JobData object itself.
     */
    @Generated
    public JobData setConfiguration(String configuration) {
        this.configuration = configuration;
        return this;
    }

[weidongxu-microsoft]

which commit is code change?

also a quick summary on what changed in generated code (other than 600+ files changed).

[haolingdong-msft]

which commit is code change?

also a quick summary on what changed in generated code (other than 600+ files changed).

updated in the PR's description

[weidongxu-microsoft]

For this one, I think the default description on client method parameter is not necessary when summary is not empty, @weidongxu-microsoft @XiaofeiCao, please let me know your thoughts, I'm debugging to see where the default is added. Update: https://github.com/Azure/autorest.java/blob/main/javagen/src/main/java/com/azure/autorest/util/MethodUtil.java#L94-L116 -> this is the code where the default description is added.

I would agree with you. The fallback logic should only happen, e.g. after merge summary and description (currently the last line), the result is still empty.

[weidongxu-microsoft]

However, I've have some concern over "A sequence of textual characters." "A 32-bit integer. (-2,147,483,648 to 2,147,483,647)" (for the latter, it even have "`" in javadoc, which likely not rendering as expected in html).

They are not really very informative description to the parameter/property. Basically String or int means the same thing. Do you know whether they come from getSummary or getDoc from typespec/compiler (not TCGC -- we want to know it end2end)

We may also want to have a quick check on other languages. Do they do this as well?

[haolingdong-msft]

However, I've have some concern over "A sequence of textual characters." "A 32-bit integer. (-2,147,483,648 to 2,147,483,647)" (for the latter, it even have "`" in javadoc, which likely not rendering as expected in html).

They are not really very informative description to the parameter/property. Basically String or int means the same thing. Do you know whether they come from getSummary or getDoc from typespec/compiler (not TCGC -- we want to know it end2end)

We may also want to have a quick check on other languages. Do they do this as well?

It comes from typespec compiler's scalar type. (returned by getDoc()): https://github.com/microsoft/typespec/blob/main/packages/compiler/lib/intrinsics.tsp#L93-L96

[haolingdong-msft]

Found our codegen logic is a bit tricky. Try to summarize on current codegen logic:

1. description for model property or getter/setter https://github.com/Azure/autorest.java/blob/main/javagen/src/main/java/com/azure/autorest/mapper/ModelPropertyMapper.java#L62-L73
   • init two variables: summary and description
   • summary = model property's summary
   • if model property's summary is null/empty, summary = property's type schema's summary (fallback to schema's summary)
   • description = model property's description
   • if both summary and description are null/empty, return dummy description
   • otherwise, merge summary and description and return.

Take belwo case as example:

model A {
 a: string
}

Before adopting TCGC, property's summary is from getSummary() and property's description is from getDoc(), getDoc() and getSummary both return empty, and the fallback logic is on schema's summary which is also empty, so we will generate dummy description. After adopting TCGC, property's summary is null/empty, but schema's summary is from sdkType.description which is A sequence of textual characters. Since we have the fallback on schema's summary logic, we will generate A sequence of textual characters..

2. description for client method parameter:
   • init two variables: summary and description
   • summary = parameter's summary
   • description = parameter's description
   • if parameter's description is null/empty, description = schema's description (fall back to schema's description)
   • if description is still null/empty, description = dummy description
   • merge summary and description and return.

Take below case as example：

op getA(@query id: string): A

Before adopting TCGC, parameter's summary is from getSummary() and parameter's description is from getDoc(), getDoc() and getSummary both return empty, and we use schema's description which is from getDoc() , so we will generate A sequence of textual characters.. After adopting TCGC, parameter's summary and description are both null/empty, parameter schema's summary is from sdkType.description which is A sequence of textual characters., plus the dummy description we add, we will generate

@param id A sequence of textual characters. 
The id parameter

My thoughts: The current logic on merging summary and description for model property and client method parameter is inconsistent which makes the generated javadoc a bit tricky. I think we can remove the fallback logics both on description and summary, and only return dummy description when both summary and description are empty/null. Like below. But I'm not sure why we do the fallback previously, so I'm also fine if we keep the fallback, but only modify the part 'only return dummy description when both summary and description are empty/null'.

• init two variables: summary and description
• summary = model property/param's summary
• description = model property's description
• if both summary and description are null/empty, return dummy description
• otherwise, merge summary and description and return.

[weidongxu-microsoft]

Agree that we should check how much impact on autorest, if we remove the "fallback to summary/description" logic.

It is reasonable to avoid the "fallback to schema summary/description". It may have some value (e.g. when the schema is a model/class), but the potential inconsistency probably outweighs its value.

[weidongxu-microsoft]

Please also keep an eye on possible places that previous code didn't do the "merge of summary and description" (so far, I didn't see anything major on this).

[haolingdong-msft]

Will do, plan to create a PR include both removing fallback and generate dummy description only when both summary and description are empty to see the impact.

Update: PR created https://github.com/haolingdong-msft/autorest.java/pull/3/files

[haolingdong-msft]

remove the fallback logics both on description and summary, and only return dummy description when both summary and description are empty/null

Hi @srnagar , @alzimmermsft, I'm working on using TCGC returned description for typespec generated sdks. There are two cases on the description diff. I summarized the case in the PR's description. And the reasons here: https://github.com/Azure/autorest.java/pull/2776#issuecomment-2121865214.

Would like to hear your thoughts on this proposal:

• remove the fallback logics both on description and summary
• only return dummy description when both summary and description are empty/null.

The impact of the change on non-typespec can be found in the PR https://github.com/haolingdong-msft/autorest.java/pull/3/files

Major impacts are:

1. we will not fallback to use parameter/model property's schema's description, if the description is not found on property or parameter. - I checked with .Net(@archerzz ) and Python(@tadelesh ), they will not have the fallback logic to get the parameter/property's type's description when property/parameter's description is empty/null.
2. we will not have extra default description when summary is not empty.

Thanks!