Add support for operation templates and operation signature reuse

[daviwil]

This change fixes #459 by implementing support for both templated operations and the definition of operations that are based on the signature of another operation (no matter how it was defined).

Here's a contrived example (sample output here):

import "@cadl-lang/rest";

using Cadl.Http;

@error
model ErrorDetails {
  code: int32;
  message: string;
}

model CodeSignAccount {
  name: string;
}

model AccountProfile {
  value: int32;
}

@get
@doc("Reads an instance of the {name} resource.", TResource)
op ResourceReadBase<TResource, TError>(@path name: string): TResource | TError;
op ResourceRead<TResource>: ResourceReadBase<TResource, ErrorDetails>;

@post
@doc("Reads an instance of the {name} resource.", TResource)
op ResourceCreateBase<TResource, TError>(@body resource: TResource): TResource | TError;
op ResourceCreate<TResource>: ResourceCreateBase<TResource, ErrorDetails>;

@route("codeSignAccounts")
interface CodeSignAccounts {
  get: ResourceRead<CodeSignAccount>;
  create: ResourceCreate<CodeSignAccount>;
}

interface ResourceOperations<TResource> {
  get: ResourceRead<TResource>;
  create: ResourceCreate<TResource>;
}

@route("accountProfiles")
interface AccountProfiles extends ResourceOperations<AccountProfile> {
}

A couple of important things to note:

• You can base an operation on the signature of an operation that was derived from yet another operation (to aid in building abstractions)
• Decorators applied to operations that you reference are applied directly to the final operation, similar to model is semantics

[bterlson]

Great start! A couple things to consider here. We also chatted offline about not introducing a new node type for "operation instance" but instead having operation nodes gain a "signature" node which is either a reference signature (containing a type reference) or a parameters signature (containing a parameter list).

[azure-pipelines]

You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/552/

[bterlson]

Discussed this with @daviwil but I wanted to raise a concern I had here also in case others have ideas - I think the syntax is maybe somewhat reasonable in interfaces, but in operations I find this hard to grok. The issue is that colon sets off the return type, and now it might also set off a reference to another operation. So given code like:

op ResourceCreate<TResource>: ResourceCreateBase<TResource, ErrorDetails>;

I can see two reasonable reads - an operation which takes no parameters and returns a ResourceCreateBase, or initializing ResourceCreate to ResourceCreateBase.

A couple fixes were discussed, though none jumped out as excellent:

1. Use is, which makes a lot of sense since I believe this aligns with model is semantics (makes a new type, copies decorators etc.). Issue is that it looks less good in interfaces.
2. Use => to set off return types instead of : which I like for many reasons but would make colon less ambiguous.

Any thoughts here?

[daviwil]

I think it'd be a lot more disruptive to change operation return type syntax. I'm OK with using the following for non-interface operations:

op ResourceCreate<TResource> is ResourceCreateBase<TResource, ErrorDetails>;

I think it will be more common for folks to use signature references inside of interfaces where the syntax is smoother. That's what I've been doing today and it looks pretty nice.

[nguerrera]

If we make any distinction between namespace and interface operations for this syntax, it will kind of makes me regret allowing op previously in interfaces. I think it should remain the case that you can paste any namespace operation into an interface to keep that justified.

I agree changing return type now would be very disruptive, much as I like => for other reasons. And even though I like =>, I still don't like having => and : go in the same position to mean different things. I'd probably end up getting confused, especially since we had : for so long with the other meaning.

[nguerrera]

The other style strangeness I see is the casing. We previously said op names should be camelCased, but : camelCased<T> looks weird to me now. Would we recommend different casing for operations and operation templates?

[daviwil]

Yeah, the pattern I've been following without really thinking about it is to have reusable signature names be pascal-cased. I didn't follow that in the unit tests I wrote, though.

[daviwil]

@bterlson How should we proceed here?

[bterlson]

is is really that bad in interfaces? @nguerrera what do you think about is for this?

[nguerrera]

I don't have a strong feeling. I'm OK with is everywhere for this.

[daviwil]

Here's what it would look like to use is inside of an interface:

interface Deployments {
  @doc("Creates a new deployment or updates an existing one.")
  createOrUpdate is ResourceCreateOrUpdate<Deployment>;

  @doc("Gets the details of a deployment.")
  get is ResourceRead<Deployment>;

  @doc("Deletes a project deployment.")
  delete is ResourceDelete<Deployment>;
}

It looks a little "off" to me aesthetically (and "get is ResourceRead ..." sounds weird), but if the consensus is that we should go with is everywhere, I'm happy to make the change.

[daviwil]

Or in other words, please let me know if I should make the change and I'll get it done today :)

[daviwil]

I went ahead and pushed a commit to this PR with the op is changes, take a look and let me know what you think @bterlson

[nguerrera]

I'm not sure, but I think this change might throw off the tmlanguage coloring, so that's worth double checking. Maybe add a test to packages/cadl-vscode/test.

I thought of this as I was thinking coloring to set-off the name from the is might help it look a little nicer. This syntax with is does feel slightly off to me, but not terrible. And then I think about how it really is directly analogous to model is and it seems like a lesser problem than overloading the meaning of : and I'm convinced this is the right way to go, at least vs. anything else that's been proposed so far.

[daviwil]

PR here: #576

[daviwil]

Oh lord, what now...

Azure Artifacts Configuration Analysis found 1 vulnerable NuGet package manifest in the repository. Visit http://aka.ms/azure-artifacts-configuration-analysis for more details.

[daviwil]

Since this is unrelated to this PR, I'll go ahead and merge and then chat with @markcowl and others about how we can resolve the security alert issue correctly (it's referring to NuGet packages).

Let me know if you want me to make any further tweaks in a future PR!

[bterlson]

Retroactive looks great, excited to use this (not excited to merge with my PR).