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

Compiler/Emitter behavior consistency and improvement #5660

Open
7 tasks
Tracked by #2070
allenjzhang opened this issue Jan 17, 2025 · 9 comments
Open
7 tasks
Tracked by #2070

Compiler/Emitter behavior consistency and improvement #5660

allenjzhang opened this issue Jan 17, 2025 · 9 comments
Assignees
Labels
1_0_E2E compiler:core Issues for @typespec/compiler feature New feature or request triaged:core
Milestone

Comments

@allenjzhang
Copy link
Member

allenjzhang commented Jan 17, 2025

Compiler improvements:

  • Display emitters being invoked. This is helpful when there are multiple emitters, users will be able to see the progress.
  • When multiple emitters run, if one emitter fails, currently compiler skip the rest. We should decide whether we want to change the behavior to run all. If not, minimally should display message telling user we have skipped the rest of xxx emitters.
  • When crash happens, write out detailed log including callstack in a temp file instead of showing on console

Consistent behavior among emitters

  • Currently C# directly write out to console on files generated while other emitters writes nothing. They should be consistent. We should decide what's the rule and ways to allow emitter/user to see message at different level.
  • Proposed rules:
    • When dealing with KNOWN bad states, each emitter should use diagnostic report at the consistent level.
    • For UNKNOWN bad states, they are allow to throw which generates callstacks & display File a bug message.
  • Each emitter output same given an empty spec
  • Champion scenario specs & common known bad state specs should be checked into Spector http-spec for regression tracking

More to add?

@ArcturusZhang
Copy link
Member

Hi @allenjzhang maybe we should also design pages for those known bad states (defined either in compiler/TCGC/language emitters) and put them inside typespec.io or typespec-azure.io about why this happens, how to fix it, examples, etc just like those compiler reported errors in other languages such as this.
In this way, when a user sees the diagnostic warning or error, there is a code that they could search for in our document pages.

To build this page, I would like to propose that we do it through automation inside of writing docs from scratches. Such as those reference docs for our decorators, we write them inside our code as comments, and there is a tool to extract them out as a doc page. We should do the same for those diagnostic error/warning pages.

@timotheeguerin
Copy link
Member

@ArcturusZhang we actually already have that where a diagnostic can define a url with additional documentation which then can be link in the terminal/ide. (Realized now that we never actually linked it in the website and we really need to go and do it for most diagnostic but here is an example https://github.com/microsoft/typespec/blob/main/website/src/content/docs/docs/standard-library/diags/triple-quote-indent.md
) I don't think however I really agree with having those pages be auto generated.

I think the point of those are to define a detailed document that shows how to resolve the issue and having to write those inline means they probably will be very limited.

@ArcturusZhang
Copy link
Member

@ArcturusZhang we actually already have that where a diagnostic can define a url with additional documentation which then can be link in the terminal/ide. (Realized now that we never actually linked it in the website and we really need to go and do it for most diagnostic but here is an example main/website/src/content/docs/docs/standard-library/diags/triple-quote-indent.md ) I don't think however I really agree with having those pages be auto generated.

I think the point of those are to define a detailed document that shows how to resolve the issue and having to write those inline means they probably will be very limited.

sure - having a page for each diagnostic is great.
I am suggesting if there could be a way that the document could be at the same place where the diagnostic is defined, so that the doc page may not be out of maintenance. But this is not really the important part - the important part is that we should have a page for each diagnostic we have explaining why this happens how this happens and how to resolve this.

@allenjzhang allenjzhang added the design:needed A design request has been raised that needs a proposal label Jan 30, 2025
@timotheeguerin
Copy link
Member

timotheeguerin commented Feb 2, 2025

Current state

The current state is that emitters and library should NOT log anything to stdout or stderr. Only things used should be

  • diagnostics
  • tracing api

And by default the compiler would just log that it completed successfully, keeping things clean.

If a library or an emitter throws an error it is reported as a bug to the user. Throwing errors should be avoided as much as possible as they prevent any additional collection of diagnostics.

Issue

This was fine when openapi was the only emitter you might use. Now with client emitters that create a lot more output and might be significantly slower to run and the ability to run multiple emitters together leaves the user wondering what is happening.

Proposal

  1. The rule above for now stays the same. Log is often just noise that is not necessary until you need to debug where extra flags can be used.
  2. The compile should log a spinner showing the current stage in progress(in the same style that vitest does Example code in chronus

When an emitter is completed it stays with the spinner replaced with a checkmark and showing the output dir
In CI the spinner is just logging the start and end state.

  1. Addition of a listOutputs flag which when enabled would log all the files emitted.
  2. If an emitter crash we can show a skip step for the other ones. For now as we have existing precedent for emitter depending on another one running first(Until we have a better solution for that and could assume that each emitter could run in parallel unless specified otherwise). Going forward could cause confusing errors.
    a. If the emitter crash the compiler will handle the error reporting. As long as the error thrown contains enough information(name and stacktrace). The file a bug url is retrieved from the package.json bug url which should be configured. We could add an ExternalToolError so you can pass that information explicitly from another process.

Examples

  • Compiler stages(parsing, checker, validation)
tsp compile .
⠼ Compiling...
  • 1st emitter in progress
tsp compile .
⠼ @typespec/openapi3 
  • 2nd emitter in progress
tsp compile .
✓ @typespec/openapi3    ./tsp-output/schemas/
⠏ @typespec/http-client-csharp
  • 2nd emitter completed
tsp compile .
✓ @typespec/openapi3    ./tsp-output/schemas/
✓ @typespec/http-client-csharp  ./tsp-output/clients/csharp
  • 2nd emitter completed with files output
tsp compile .
✓ @typespec/openapi3                 ./tsp-output/schemas/
       ./tsp-output/schemas/openapi.v1.yaml
       ./tsp-output/schemas/openapi.v2.yaml
✓ @typespec/http-client-csharp  ./tsp-output/clients/csharp
       ./tsp-output/clients/csharp/proj.csproj
       ./tsp-output/clients/csharp/FooClient.cs
       ./tsp-output/clients/csharp/models/A.cs
       ./tsp-output/clients/csharp/models/B.cs
  • 2nd emitter completed with tracing: With tracing the logs are always shown above in an interactive CLI.
tsp compile . --trace *
import-resolution Resolving file a.tsp
openapi3.emitSchema Emitting schema A
openapi3.emitSchema Emitting schema B
http-client-csharp.emittingClass A
http-client-csharp.emittingClass B
✓ @typespec/openapi3                 ./tsp-output/schemas/
       ./tsp-output/schemas/openapi.v1.yaml
       ./tsp-output/schemas/openapi.v2.yaml
✓ @typespec/http-client-csharp  ./tsp-output/clients/csharp
       ./tsp-output/clients/csharp/proj.csproj
       ./tsp-output/clients/csharp/FooClient.cs
       ./tsp-output/clients/csharp/models/A.cs
       ./tsp-output/clients/csharp/models/B.cs

@ArcturusZhang
Copy link
Member

@timotheeguerin It does look nicer in your proposal, but in our emitters we have some more technical difficulties.
If the emitter is fully written in ts, everything is fine, it could invoke the API provided by tsp compiler to report diagnostics, and prepare the file content to write and send it to the framework so that the compiler could output which file it is writing.

But in our implementation, quite a few emitters have a backend written in their own language - for instance in our http-client-csharp emitter, the ts part invokes a program written in C#, and the data calculation and file written are done in the C# part - the C# part currently does not have a way to invoke emitter APIs.
To achieve your proposal, we need to come up with a way to communicate between the ts part and the C# part.
As far as I know, other languages also have this difficulty, such as python and java.

In the old autorest, we used JRPC. What is your current suggestion?

@timotheeguerin
Copy link
Member

Ah, yeah I didn't see this being part of this proposal. The idea thrown around to is preciously was a one way stdout rpc for specific things like diagnostics. For emitting files I am not sure going through the rpc was a great idea in autorest but we could at least have a reporter

@ArcturusZhang
Copy link
Member

BTW @timotheeguerin I understand the diagnostics in your proposal, we have a page for it here: https://typespec.io/docs/extending-typespec/diagnostics/
Which part is the "tracing api"? Is this: https://typespec.io/docs/handbook/configuration/tracing/#_top

I am planning to work on a refactor in .net emitter to unify the way of writing logs and diagnostics, this could be important to align your proposal as the first step

@timotheeguerin
Copy link
Member

yep this is it. Goal is that every log belongs to an area(Starting with your library ideally) so user can decide to enable tracing for only a certain part.

@markcowl markcowl added this to the [2025] March milestone Feb 18, 2025
@markcowl markcowl added feature New feature or request and removed design:needed A design request has been raised that needs a proposal labels Feb 18, 2025
@markcowl
Copy link
Contributor

est: 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1_0_E2E compiler:core Issues for @typespec/compiler feature New feature or request triaged:core
Projects
None yet
Development

No branches or pull requests

8 participants