Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug][tcgc] getSdkUnion would merge underlying enum members to top level #379

Closed
MaryGao opened this issue Mar 7, 2024 · 7 comments · Fixed by #389
Closed

[Bug][tcgc] getSdkUnion would merge underlying enum members to top level #379

MaryGao opened this issue Mar 7, 2024 · 7 comments · Fixed by #389
Assignees
Labels
bug Something isn't working lib:tcgc Issues for @azure-tools/typespec-client-generator-core library

Comments

@MaryGao
Copy link
Member

MaryGao commented Mar 7, 2024

Describe the bug
When we adopt to the latest tcgc our two cases failed and we found that

  • Sometimes the getSdkUnion could return correct isFixed value like Case 2 but sometimes it can't like Case 1
  • Also for union of EnumA | EnumB tcgc would flatten all underlying enums into top level

TCGC are supposed to provide consistant isFixed information, otherwise it would be a big issue for JS because RLC didn't adopt tcgc but Modular does. So it would introduce type inconsistancy between them and finally fails the compile process.

Also I think current behavior for prompting members to top level is debetable. It would lose the name info which may be useful for emitters.

Case 1: Nullable enum

enum Color {
  Color1: 1,
  Color2: 2
}
model Test {
  color: Color | null;
}
op read(@body body: Test): void;

image

Case 2: EnumeA | EnumB

 enum LR {
  left,
  right,
}
enum UD {
  up,
  down,
}

model Test {
  color: LR | UD;
}
op read(@body body: Test): void;

image

@MaryGao MaryGao added the bug Something isn't working label Mar 7, 2024
@tadelesh tadelesh added the lib:tcgc Issues for @azure-tools/typespec-client-generator-core library label Mar 7, 2024
@MaryGao MaryGao changed the title [Bug][tcgc] isFixed when calling getSdkUnion is wrong [Bug][tcgc] isFixed is wrong when calling getSdkUnion Mar 7, 2024
@tadelesh
Copy link
Member

tadelesh commented Mar 7, 2024

TCGC get this info from two places, first is isFixed (code) from @fixed decorator, second is the open property in UnionEnum type (code). I think current problem should mainly from losing information for hierarchy union.

@lirenhe
Copy link
Member

lirenhe commented Mar 7, 2024

@MaryGao, in JS, do we have a workaround for this issue?

@MaryGao
Copy link
Member Author

MaryGao commented Mar 7, 2024

@MaryGao, in JS, do we have a workaround for this issue?

I don't think we have a quick workaround here because our modular logic has serveral dependencies with isFixed column, currently we just fallback to tcgc dev.17 version.

Currently RLC leverages the isFixed in core and for modular we leverages the tcgc. The inconsistancy would exist if we have gaps between core isFixed and tcgc.

@iscai-msft
Copy link
Contributor

Here we are only calling what typespec and typespec-azure are providing us, like @tadelesh said. Talked with @lmazuel and for case 1, it is by design that isFixed is still returning False. The tsp team will change the behavior of isFixed once everyone has moved to adopting "union with base type is extensible enum"

@lmazuel
Copy link
Member

lmazuel commented Mar 7, 2024

Both your cases are by design and work as expected:

enum Color {
  Color1: 1,
  Color2: 2
}

is an extensible enum. We do have plan to switch this to fxed enum, when we can confirm all emitters are treating the union correctly

Flattening enum values is by design as well.

@MaryGao I'm worried to be honest of what "workaround" you would do here, since both are by designed. I'm closing this issue, and we can confirm this off-line.

@tadelesh
Copy link
Member

tadelesh commented Mar 8, 2024

The inconsistency is come from the flatten. TCGC uses getUnionAsEnum to see if a union is an enum, and this function does the judgement from the flattened elements view, because TypeSpec type system's UnionEnum type could has child union or enum element. Since client type system could only has union or enum type, I made a fix to change back to union if union has union or enum variants, regardless of it is an enum with flatten view.

@MaryGao MaryGao changed the title [Bug][tcgc] isFixed is wrong when calling getSdkUnion [Bug][tcgc] getSdkUnion would merge underlying enum members to top level Mar 8, 2024
@tadelesh tadelesh removed the Blocking label Mar 12, 2024
@tadelesh
Copy link
Member

tadelesh commented Mar 12, 2024

JS has workaround for the inconsistency of whether is open for flattened union as enum. Remove Blocking label.

@tadelesh tadelesh self-assigned this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants