[tcgc] crash if duplicate models / enums across namespaces when flattened with namespace flag or multi service #3775
Conversation
…cgc/duplicateModelNames
iscai-msft
requested review from
joheredi,
lmazuel,
m-nash and
srnagar
as code owners
microsoft-github-policy-service
bot
added
the
lib:tcgc
commit: |
|
All changed packages have been documented.
Show changes
|
iscai-msft
requested review from
bterlson,
markcowl and
timotheeguerin
as code owners
| // Skip `@clientName` on versioning types | ||
| continue; | ||
| } | ||
| if (type.kind === "Operation") { |
There was a problem hiding this comment.
The following @clientLocation logic for operation has one legacy issue: we now allow to move to existed ns with string (@@clientLocation(op, "Test"); namespace Test {}), which is not handled in the code.
There was a problem hiding this comment.
ooh good catch, thanks!
| movedTo, | ||
| tcgcContext.program.getGlobalNamespaceType(), | ||
| ); | ||
| if (isMultiService) { |
There was a problem hiding this comment.
I don't think multiple service is a criteria for this check. Shall we use the criteria: whether honor TypeSpec namespace?
There was a problem hiding this comment.
I want to run the check only when we really need to, that's why I want with multi service. what criteria would you use to evaluate whether it honors the tsp namespace? since we don't have access to the namespace flag at this point
There was a problem hiding this comment.
If we honor TypeSpec namespace in multiple services scenario, we should not do the validation. With Tim's opinion, I believe we could only do the validation in the createSDKContext.
| /** | ||
| * Collect all types from a namespace and its nested namespaces recursively. | ||
| */ | ||
| function collectTypesFromNamespace( |
There was a problem hiding this comment.
The problem here is the check will also for the types that is not used in the client.
There was a problem hiding this comment.
yeah, but the problem here is that we don't have access to the sdkcontext at this point, so we still don't know whether a type will be used in the eventual client. What scenario do you have where we don't care if there's collisions?
packages/typespec-client-generator-core/src/validations/types.ts
Outdated
Show resolved
Hide resolved
packages/typespec-client-generator-core/src/validations/types.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // Validate cross-namespace names if namespace flag is set (flattens namespaces) | ||
| // Can't validate in $onValidate bc we don't have access to the namespace flag | ||
| validateCrossNamespaceNamesWithFlag(context, diagnostics); |
There was a problem hiding this comment.
We always have duplicate logic because of the config for emitter could not be get when compile or validate. Could you help to confirm with TypeSpec team to see if there is any solution that we could have such info.
There was a problem hiding this comment.
@timotheeguerin do you have any suggestions here? Basically we want to do validations in $onValidate, but that takes in the tcgc context without flags. When we want to do validation with flags, we have to rerun validation with sdk context and access to the flags. Is this the only way? Thanks!
There was a problem hiding this comment.
where do those flags come from?
There was a problem hiding this comment.
while missing some context I think you either needs to do the validation in onValidate for every case scenario once(Like I believe it was done for @clientName conflict validating all emitters) or you have to let that validation happen when the emitter set their flag
There was a problem hiding this comment.
So, I believe if our validation is based on emitter's config, we need to do that in the createSDKContext.
| const allUnions: Union[] = []; | ||
| const allScalars: Scalar[] = []; | ||
|
|
||
| for (const serviceNs of serviceNamespaces) { |
There was a problem hiding this comment.
This missed the check for imported lib: Azure.Core, Azure.ResourceManager, etc.
|
Shall we have some discussions with TypeSpec team to see their suggestions for such kind of validation, since it will impact perf. |
4 participants
No description provided.