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

Emitter framework feedback #2729

Open
timotheeguerin opened this issue Dec 4, 2023 · 7 comments · May be fixed by #2799
Open

Emitter framework feedback #2729

timotheeguerin opened this issue Dec 4, 2023 · 7 comments · May be fixed by #2799
Assignees
Labels
design:needed A design request has been raised that needs a proposal triaged:core
Milestone

Comments

@timotheeguerin
Copy link
Member

timotheeguerin commented Dec 4, 2023

Forgot to file this before but those are pain points I noted when migrating openapi3 to the emitter framework

  • Not clear how to use context apart from the few example around scoping
  • Typed context
  • Context readonly
  • Too mutable, too easy to mutate a declaration from somewhere else and have state bleed over
  • Declaration trace back to OG type
  • Collect diagnostics instead of emitting to program

There was some others that have either already been addressed in PRs or have dedicated issue(duplicate name tracker)

Brainstormed idea

createAssetEmitter(
  union: () => ... my handler ...
  unionVariants: JoinVariantsWith(" | ")
  unionVariant: KeepRef,
)
@timotheeguerin timotheeguerin added the design:needed A design request has been raised that needs a proposal label Dec 11, 2023
@timotheeguerin timotheeguerin added this to the [2024] February milestone Dec 11, 2023
@tjprescott
Copy link
Member

tjprescott commented Dec 11, 2023

In using the emitter framework (and separately but also very importantly, the emitter framework documentation) I found the following issues:

1. Understanding What You Need to Implement [Framework]

There’s this line (emphasis mine):

To support emitting all TypeSpec types, you should expect to implement all of these methods. But if you don't want to support emitting all TypeSpec types, you can either throw or just not implement the method, in which case the type will not be emitted.

If this is the case, why is it that all of these have default do-nothing implementations? This makes it hard to understand what you even can implement, much less what you should. Sometimes, the default implementation does some functional magic (like iterating through a collection) but other times it does nothing, which from a user perspective is frustrating.

Other inconsistencies like having an enumMemberReference but not a corresponding unionVariantReference deepen the mystery for a user trying to figure out what they need to implement.

I’m not sure users care about the distinction between modelLiteral and modelDeclaration. Rather they care about handling models generally. For me it seems preferable to implement a single method for models and let a discriminator handle the difference between a named and anonymous model rather than have to implement two separate methods. The discriminator would ensure both of these cases are handled—something like ModelKind.literal and ModelKind.declaraction.

2. Improved Examples [Docs]

https://microsoft.github.io/typespec/extending-typespec/emitter-framework#builders
Here it uses the code tag exclusively but provides no example of actually using a StringBuilder, even though I definitely needed to use that and expect that other uses will as well. The documentation should also provide examples for ObjectBuilder and ArrayBuilder, or omit them.

3. Unnecessary Focus on Implementation Details [Docs]

Largely, the documentation reads like a deep-dive into the inner workings of the TypeSpec emitter framework but is of limited value for an emitter author who simply wants to create an emitter using the framework.

https://microsoft.github.io/typespec/extending-typespec/emitter-framework#implementing-your-emitter

Why do we walk through these types? We mention StringBuilder, ObjectBuilder, and ArrayBuilder but never provide examples of these. The example under TypeEmitter actually references a CodeTypeEmitter, which could confuse users. In the section of EmitterOutput, why bother talking about CircularEmit if the user doesn’t create this?

4. Emitter Output: Declarations [Framework]

It doesn’t make sense to me why this.emitter.result.declaration requires a name and code. There are cases where I need to register the type name as a declaration so it can be referenced later, but I do not want to emit that type (specifically, for unions, which I will replace when referenced with a union literal). I should be able to provide null here instead of empty string. Alternatively, I feel the return type could be the code and that registering a declaration should just a standalone call that does not contribute output. This would mean you have to make two calls for most type declarations, one to register the type as a type and the return call which emits the output.

@tjprescott
Copy link
Member

tjprescott commented Dec 12, 2023

Two more additions:

5. Provide a way to know if a given type has been declared [Framework]

We have a specific result call for declaration so it seems like somewhere internally there's a record of what has been declared. It would be useful to the emitter author to be able to query that map to see if a given type has been declared or not. In my case, for a given type reference, I need to reference it differently if it is going to be a forward reference (declared after it is used) as opposed to a "normal" reference (where it is defined first). I had to add my own internal workaround to query information that, by all appearances, should definitely be available.

6. Emitter Framework Coverage is Incomplete [Framework]

There are no methods to support emitting template declarations of any kind. Conceptually, I should be able to rewrite typespec-apiview to use the emitter framework, but I can't because I need to emit template declarations the way I would any other declaration, and that's not possible in its current state. I could accomplish it with some workarounds that essentially mean I'm not using the framework, but that just highlights the gap in the framework's functionality.

I think we should let emitter authors decide whether they want to emit template declarations rather than take that choice from them by not even exposing a method.

@timotheeguerin
Copy link
Member Author

timotheeguerin commented Dec 12, 2023

Related to @tjprescott #5 maybe have declaration keep a "depth" reference index in the typegraph so if an emitter care to have element only reference element declared before it could sort by that depth.(Circular reference would need some special handeling of course)

@tjprescott
Copy link
Member

And one more:

7. Allow sourceFile to return null to indicate that you don't want to emit a file

I have to create a source file when creating a context, but it's entirely possible that the source file ends up being empty. In that case, when writeOutput is calling sourceFile I'd like to be able to detect that there were no declarations and return null to indicate that writeOutput should skip that file, but currently you must return a file.

@tjprescott
Copy link
Member

8. AssetEmitter.writeOutput should automatically exit if the compiler is set to noEmit

I spent a solid two hours trying to figure out why I kept getting bizarre errors when opening a *.tsp file even though everything else worked fine (debugging, cli, test, playground). It turns out it was because I had a tspconfig.yaml file with the emit property set. Despite the IDE running the compiler in noEmit mode, it was picking up this emit option and forcibly trying to emit output. I had to add a guard around the writeOutput statement in my $onEmit. However, the documentation doesn't do that, and this error was frustrating to track down. writeOutput should simply handle this situation gracefully.

@timotheeguerin timotheeguerin self-assigned this Jan 17, 2024
@timotheeguerin timotheeguerin linked a pull request Jan 18, 2024 that will close this issue
25 tasks
@tjprescott
Copy link
Member

Some more:

9. Need a way to track declarations that need to be emitted later

In my repo, I created a DeclarationsManager so that I could keep track of "deferred declarations". These are declarations that are discovered while emitting something else that I need to throw in a stack to ensure I emit later on. They would not normally be emitted.

For example:

model Foo<T> {
  prop: T;
}

model MyFoo {
  foo: Foo<string>;
}

In my emitter, nothing is every called for Foo<T> (see item 6) but the way I output this in Python is to create a class call FooString which I discover while emitting MyFoo. I have to track this, so I add Foo<string> to my "deferred declarations" so I can be sure to emit it later on.

10. Prefer getTypeEmitter over getAssetEmitter

My emitters make extensive use of subclassing CodeTypeEmitter so they can add methods and properties necessary to synchronize with one another. However, to create an actual TypeEmitter I have to provide an AssetEmitter, which means a call to getAssetEmitter. This makes me pass in a TypeEmitter class, which is used to instantiate a local typeEmitter within the body of that call. There is no way to access the typeEmitter that is created in the method, which resulted in hours spent trying to track down a frustrating bug where a property on my TypeEmitter did not have the data it should have. It turns out the data was being set on the internal typeEmitter which then disappeared when it went out of scope.

Given a TypeEmitter I can easily access its AssetEmitter, but the converse is not true. As a result, I copied the entire 1k LOC implementation of getAssetEmitter just so that it would return [typeEmitter, assetEmitter] instead of just return assetEmitter. Doing so absolutely fixed my issue.

It is reasonable to assume people will want to subclass TypeEmitter (in my case CodeTypeEmitter) so that they can provide default implementations, etc.

My recommendation would be to change getAssetEmitter to be getTypeEmitter.

@markcowl markcowl modified the milestones: [2024] February, [2024] April Feb 7, 2024
@timotheeguerin
Copy link
Member Author

10. getAssetEmitter can result in version conflict issue.

Because that helper is defined on compiler it will use the compiler loaded during tsp compile . however an emitter might then import the different class from a different version of the compiler causing some issues when doing instanceof. This can make the emitter framework not render the object builders and other elements correclty.

@markcowl markcowl modified the milestones: [2024] April, [2024] May, Backlog Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants