-
Notifications
You must be signed in to change notification settings - Fork 204
Support more expressions in @page
directive
#11805
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
base: main
Are you sure you want to change the base?
Conversation
I can't see any tests that validate the C# code-gen is mapped correctly for tooling purposes. Can you add a new integration test that exercises the new syntax, and perhaps a test on the tooling side in https://github.com/dotnet/razor/blob/main/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Semantic/SemanticTokensTest.cs. The same |
I'll add those tests too. Thanks for the guidance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Rekkonnect thanks for this PR.
Apologies, I jumped the gun saying we would take this PR, I hadn't appreciated it was an actual language change.
We need to review the language design and decide if it's something we want to add to razor, and if there are any back compat worries etc.
I'm going to block the change for now so we can get that scheduled on our side. I'll post back here once that's happened, and we can proceed or change it based on the decisions reached.
I would hold off making any further changes for now, as I don't want you to waste any more effort if we decide go in a different direction with it.
Thanks!
Closes #7519
Details
The issue asks about being able to provide constant expressions into the page directive, possibly including constant string interpolations, string concatenations, or mixes of the two.
This PR provides a primitive solution to that by accepting either a double-quoted string, or an identifier (single or qualified), or a razor expression. It is not required to place a @ before the identifier, but the razor expression requires that the expression be placed inside
@()
.To support this new syntax, a new token kind had to be created, IdentifierOrExpressionOrString. This follows the logic of its components, IdentifierOrExpression and String. The parser tries to first parse a razor expression, else an identifier, else a string, otherwise fails. While directly using a non-simple string literal is not supported, there exists the workaround of placing the non-simple string literal into a razor expression (like so:
@($"{AppRoutes.Forms}/{{id:int}}")
)Retrieving the string value is not possible when a non-string literal expression is used, since we don't have the ability to look into the compilation while generating the files. So instead, we introduce a new public constant field in the generated page class, named
__RouteTemplate
, which is used to contain the compilable expression, in order to guarantee the expression is always validly evaluated. If we directly used that expression in the assembly attribute, resolving the actual expression would fail 99% of the time, as we'd be in the global namespace and the expression's top level identifier will possibly refer to a namespaced member.We can also leverage that technique when any expression other than a double-quoted string is used. This ensures we don't introduce custom parsing logic which would introduce the risk of deviating from the intended C# behavior. So, arbitrary razor expressions are also accepted, whose contents will be used for the instantiation of the generated constant.
The assembly attribute containing the page name will refer to the generated class's public field by fully qualifying the member. This is possible because we know the namespace and the class name of the generated class, and the public field's name is defined by us, which is placed in a constant. The field is only used when the route template isn't a simple string, and thus isn't known in Razor's context. If the field doesn't exist (because the
@page
directive has no defined template), no field is used and the behavior remains the same.Consequences
Risks
The parser becomes more complicated by having a new token kind. Especially with the string part of the new token kind not adhering to all C# rules.
The
@page
directive generates a new public constant field for every non-simple route template page. This bloats the generated code (very minorly).The
@page
directive is now designed to accept bothDirectiveTokenIntermediateNode
andLazyIntermediateToken
, aside from the simple double-quoted string literal, possibly evaluating both newly added properties in some cases. while the runtime overhead is negligible, code involving aPageDirective
instance must be aware of these changes.CC @chsienki @javiercn