[compiler] Named instantiation of template arguments #2546
Conversation
witemple-msft
requested review from
bterlson,
daviwil,
markcowl,
allenjzhang and
timotheeguerin
as code owners
|
Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status): Website: https://tspwebsitepr.z22.web.core.windows.net/prs/2546/ |
witemple-msft
changed the title
|
Validated in the playground that this allows the required customization for Microsoft Graph - thank you! |
| @@ -60,7 +60,7 @@ describe("compiler: templates", () => { | |||
| const diagnostics = await testHost.diagnose("main.tsp"); | |||
| strictEqual(diagnostics.length, 1); | |||
| strictEqual(diagnostics[0].code, "invalid-template-args"); | |||
| strictEqual(diagnostics[0].message, "Too few template arguments provided."); | |||
| strictEqual(diagnostics[0].message, "Template argument 'T' is required and not specified."); | |||
There was a problem hiding this comment.
Shouldn't that message stay the same? If we are missing positional parameters? Feels like otherwise this error leads you to believe we are only missing template argument T while we might be missing more.
There was a problem hiding this comment.
I struggled with this a little bit. You'll at least get one error per uninstantiated parameter name. When I thought about it, it just didn't seem like "too few" was really the right error anymore, for example:
Given alias Foo<T, U = string>, Foo<U = "bar">... well now I don't exactly have "too few" arguments. I have enough arguments; they're just the wrong ones and T is unbound.
I could also specify a list of args with bogus names in a template with different names, and then the problem wouldn't be that I have too few of them either:
Given alias Foo<T, U, V>, Foo<X = string, Y = numeric, Z = boolean>... I have enough args, they're just all wrong, so "too few" is again not quite the right error anymore.
I eventually landed on just raising one Missing required template argument error per missing arg, but I'm open to alternatives, just don't think "too few" is the one anymore.
There was a problem hiding this comment.
hhm I guess that's what you get in csharp when missing an argument on a function
| model A<T, U, V extends string = string> { a: T, b: U, c: V } | ||
|
|
||
| @test model B { | ||
| foo: ~~~A<boolean, V = "bar", string>~~~ |
There was a problem hiding this comment.
Shouldn't the error here be on string
There was a problem hiding this comment.
There are two errors, one on the whole typeref (Template argument 'U' is required and not specified.) and one on string (positional cannot follow named). I calculate the pos/end of the error on string from the outer type reference.
| Diagnostic[], | ||
| ]; | ||
|
|
||
| const foo = B.properties.get("foo")!.type; |
There was a problem hiding this comment.
nit: does it matter to test the content of this template if there was an error creating it?
There was a problem hiding this comment.
This test is just making sure I didn't break anything in the logic about how to instantiate unspecified types. We instantiate them to their constraint (or unknown in this case as the implied constraint of an unconstrained parameter).
docs/language-basics/templates.md
Outdated
There was a problem hiding this comment.
Can you also update the naming convention doc to say how to name template param (pascal case without T?)
|
@allenjzhang Gentle nudge on getting this into the build. It's been open quite a while. |
|
@garethj-msft Just to give you a concrete ETA: we've got a couple of open PRs blocking this one -- just renames of template args that we've been discussing. I'm aiming to have this merged next week for release in January. |
Co-authored-by: Timothee Guerin <[email protected]>
|
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/2546/ Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/prs/2546/ |
Implementation of named template arguments as described in #2340.
I added a new syntax node (TemplateArgument), and a cover grammar for this node:
<Expression> ('=' <Expression>)?Rather than ExpressionNode, the type of a template argument is now
TemplateArgumentNode.If the
=is parsed, we assert in the parser that the first Expression must be a "bare identifier" (i.e. a TypeReference withtarget: Identifier) and unwrap the identifier to produce a clean AST with `name?: IdentifierTemplate arguments are evaluated in the order that they were declared, not in the order they are specified in the instantiation. The new template argument checker replaces the previous one, and it performs normalization of argument order and typechecking of template arguments all in one pass.
TODO:
Closes #2340