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

[http-client-csharp] spector case http/client/naming cannot compile #5653

Open
ArcturusZhang opened this issue Jan 17, 2025 · 6 comments
Open
Assignees
Labels
emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp

Comments

@ArcturusZhang
Copy link
Member

This spector has a sub-client ClientModel and a model ClientModel under the same namespace: https://github.com/Azure/typespec-azure/blob/20444c9531da7b8b1bba8932d29269e96cacb270/packages/azure-http-specs/specs/client/naming/main.tsp#L147-L164

In our generated code, now we just generated a client type ClientModel, and a model type ClientModel.
This will never compile because they both have a parameterless constructor. And on the other hand, even if this could compile, this will never work.

This issue will not happen on main branch. The code on main branch also have one client ClientModel and one model ClientModel but they are defined in different namespaces therefore it is actually fine.
But with the "honor tsp namespace" feature, they are now defined in the same namespace, and we now have a trouble.

@ArcturusZhang ArcturusZhang added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Jan 17, 2025
@ArcturusZhang
Copy link
Member Author

ArcturusZhang commented Jan 17, 2025

TypeSpec itself allows us to define a model with the same name under a namespace, such as:

namespace Foo {
    model Foo {}
}

We have checks on those clientName decorators to ensure within the same namespace, we do not have models with the same name.
But we did not check model-client cases because typespec itself allows it. @live1206

Should we do a check in the emitter and report another diagnostic on this, and then automatically rename the model? Or we should ignore it and fix on its spec?

@JoshLove-msft
Copy link
Contributor

JoshLove-msft commented Jan 21, 2025

I'm curious how this can work for other typed languages, e.g. Java. If a customer is explicitly trying to name the model and the client as the same thing, I don't think we should try to rename one of them. We should let the compilation fail.

But don't we end up suffixing all clients with "Client"?

Or is this because it is a subclient?

@ArcturusZhang
Copy link
Member Author

I'm curious how this can work for other typed languages, e.g. Java. If a customer is explicitly trying to name the model and the client as the same thing, I don't think we should try to rename one of them. We should let the compilation fail.

But don't we end up suffixing all clients with "Client"?

Or is this because it is a subclient?

in this case, it is a sub-client, therefore we did not append any suffix.
In other languages like java, they always append the suffix therefore they look fine: https://github.com/Azure/autorest.java/blob/7914c8c37f250b11381d2e18a84ed613070eacde/typespec-tests/src/main/java/client/naming/ClientModelClient.java#L26

@ArcturusZhang
Copy link
Member Author

So based on our principal, it is fine that we generate code that cannot compile - the ground rule is that we have to generate something, it may be fine if in some edge cases it cannot compile. Diagnostics should always be reported when such cases happen.

In the namespace PR, considering it already contains so many changes (1000+ files changed with their namespaces), I would temporarily remove this case in the namespace PR to make sure everything is working properly.
And later we could introduce this case back, by asserting it cannot compile

@JoshLove-msft
Copy link
Contributor

And later we could introduce this case back, by asserting it cannot compile

We should assert that the model/client with the same names are generated.

@JoshLove-msft
Copy link
Contributor

We can make the project build by adding <compile include = none for one of the files. We can then add a test case that asserts both files exist. Or we can use Roslyn to compile into a workspace and validate that there is one error. We then would just write "PASS" for the test case in the coverage file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp
Projects
None yet
Development

No branches or pull requests

2 participants