jdaugherty commented on code in PR #16139:
URL: https://github.com/apache/grails-core/pull/16139#discussion_r3835321879
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/tags/GroovyDefTag.java:
##########
@@ -59,10 +59,35 @@ public void doStartTag() {
if (typeName.equals("def") || typeName.equals("Object")) {
out.println(expr);
} else {
- out.println(typeName + ".cast(" + expr + ")");
+ // A Groovy cast rather than Class.cast, which only accepts what
is already of the type
+ // and so rejects every conversion Groovy would otherwise have
made. The expression is
+ // parenthesised because it is written by the page and may be a
GString or an operation.
+ out.println("(" + typeName + ") (" + expr + ")");
}
}
+ /**
+ * The Groovy an attribute value stands for.
+ *
+ * <p>A lone <code>${...}</code> is the expression it holds, and text with
no expression in it is
+ * read as written, which is what an untyped tag naming a variable relies
on. Text mixing the two,
+ * or holding more than one expression, is neither: it is a GString, and
emitting it unquoted
+ * produced source that did not parse.
+ */
+ private static String groovyExpressionFor(String value) {
+ String text = value.trim();
+ if ((text.startsWith("\"") && text.endsWith("\"")) ||
(text.startsWith("'") && text.endsWith("'"))) {
+ text = text.substring(1, text.length() - 1).trim();
+ }
+ if (!text.contains("${")) {
+ return text;
+ }
+ if (text.startsWith("${") && text.endsWith("}") && text.indexOf("${",
2) < 0) {
+ return text.substring(2, text.length() - 1).trim();
+ }
+ return '"' + text.replace("\\", "\\\\").replace("\"", "\\\"") + '"';
Review Comment:
`text.replace("\\", "\\\\").replace("\"", "\\\"")` escapes the whole
attribute, including the code inside its `${...}` — but by the time this runs
the parser has already made the attribute valid Groovy.
`populateMapWithAttributes` strips a lone `${...}` down to the expression it
holds and wraps everything else in quotes, so what arrives here is either an
expression or a (G)String literal. Re-escaping it breaks two shapes:
| page | emitted | outcome |
| --- | --- | --- |
| `<g:def type="String" var="s" value="a ${[1].collect { "x$it" }} b"/>` |
`String s=(String) ("a ${[1].collect { \"x$it\" }} b")` | `Unexpected
character: '\'` — does not compile |
| `<g:def var="s" value="cost: \${1}"/>` | `("cost: \\${1}")` | renders
`cost: \1` |
The first fails to compile; the second is silently wrong output — the page
escaped the dollar to keep `${1}` literal and gets a backslash plus an
interpolation instead. Both go through the branch this line is in, and the
untyped tag reaches it too, since `groovyExpressionFor` runs before the type is
looked at.
`g:set type=` has neither problem, because it hands the parser's literal to
`getExpressionText` untouched. Doing the same here fixes both:
```java
private static String groovyExpressionFor(String value) {
String text = value.trim();
if (text.contains("${")) {
// Already Groovy: the parser stripped a lone ${...} to the
expression it holds and quoted
// anything else, so this is either that expression or a GString
literal.
return text;
}
if ((text.startsWith("\"") && text.endsWith("\"")) ||
(text.startsWith("'") && text.endsWith("'"))) {
text = text.substring(1, text.length() - 1).trim();
}
return text;
}
```
I ran this: the nested-quote page renders `a [x1] b`, `value="cost: \${1}"`
renders `cost: ${1}`, and `value='a ${["k": "v"]["k"]} b'` renders literally —
a single-quoted attribute does not interpolate, which is what every other GSP
attribute does with one, and better than the compile error it produces today.
`GspCompileStaticConfigSpec` stays green, including the two mixed-value `g:def`
rows, which round-trip through the new branch unchanged.
Those two rows (`"Total: ${1 + 1}"`, `"${1 + 1} and ${2 + 2}"`) are also
exactly the shapes the escaping happens to survive, so a nested-quote value and
an escaped `\${...}` would be worth adding beside them.
##########
grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePluginFunctionalSpec.groovy:
##########
@@ -62,4 +62,34 @@ class GroovyPagePluginFunctionalSpec extends
GradleSpecification {
result.output.contains('WEBAPP_HAS_PROVIDED_COMPILE=true')
result.output.contains('WEBAPP_HAS_CLASSES_DIR=true')
}
+
+ def "the page opt-in reaches both the build's page compiler and the JVM
running the application"() {
+ given:
+ setupTestResourceProject('gsp-compile-static')
+
+ when:
+ def result = executeTask('inspectGspCompileStatic')
+
+ then: 'the pages the build compiles ahead of time'
+ result.output.contains('PAGE_COMPILER=true')
+ result.output.contains('WEBAPP_PAGE_COMPILER=true')
+
+ and: 'and the pages compiled again while the application runs'
+ result.output.contains('RUNNING_APPLICATION=true')
+
+ and: 'strictness travels with it, to both'
+ result.output.contains('PAGE_COMPILER_STRICT=true')
+ result.output.contains('RUNNING_APPLICATION_STRICT=true')
+ }
+
+ def "pages compile the way configuration says where the opt-in is not
set"() {
+ given:
+ setupTestResourceProject('gsp-compile-classpath')
+
+ when:
+ def result = executeTask('inspectGspCompileClasspath')
+
+ then: 'the project applies grails-gsp without the grails extension and
still configures'
+ result.output.contains('HAS_GSP_COMPILE_CONFIGURATION=false')
Review Comment:
This asserts the same single line as `"plugin does not register a gspCompile
configuration"` at the top of the class, and that line is about the removed
`gspCompile` configuration — nothing in it is about the opt-in the test is
named for. It passes whatever `compileStatic` ends up as.
The behaviour is worth a test, though, and this is the project that
exercises it: `wireCompileStaticOptions` only reads the extension inside
`plugins.withType(GrailsGradlePlugin)`, so a project applying `grails-gsp` on
its own is the one where the extension never appears and the values set at
registration have to stand. Printing them from the same task states it:
```groovy
// gsp-compile-classpath/build.gradle, inside inspectGspCompileClasspath
println
"PAGE_COMPILER=${tasks.named('compileGroovyPages').get().compileStatic.get()}"
println
"WEBAPP_PAGE_COMPILER=${tasks.named('compileWebappGroovyPages').get().compileStatic.get()}"
```
with `result.output.contains('PAGE_COMPILER=false')` here, which is the
claim the name makes.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java:
##########
@@ -175,6 +208,7 @@ public class GroovyPageParser implements Tokens {
public static final String CONFIG_PROPERTY_GSP_GRAILS_LAYOUT_PREPROCESS =
"grails.views.gsp.layout.preprocess";
public static final String CONFIG_PROPERTY_GSP_COMPILESTATIC =
"grails.views.gsp.compileStatic";
public static final String CONFIG_PROPERTY_GSP_ALLOWED_TAGLIB_NAMESPACES =
"grails.views.gsp.compileStaticConfig.taglibs";
+ public static final String CONFIG_PROPERTY_GSP_COMPILESTATIC_STRICT =
"grails.views.gsp.compileStaticConfig.strict";
Review Comment:
This is a new settable configuration value, and it is not in
`grails-gsp/plugin/src/main/resources/META-INF/spring-configuration-metadata.json`
— the file `grails-doc/build.gradle` reads out of the `grails-gsp` jar to
generate the configuration reference. As it stands the setting is described in
prose on the new guide page but missing from the reference and from IDE
completion.
Its siblings are already there (`grails.views.gsp.encoding`,
`grails.views.gsp.layout.preprocess`, `grails.views.gsp.codecs.*`), so it is
one entry:
```json
{
"name": "grails.views.gsp.compileStaticConfig.strict",
"type": "java.lang.Boolean",
"description": "Whether a statically compiled page is held to the names it
declares, rather than only pages that declare a model.",
"defaultValue": false
}
```
`grails.views.gsp.compileStatic` and
`grails.views.gsp.compileStaticConfig.taglibs` are absent too. Those predate
this PR, but this is the PR that makes them the documented way to use the
feature, so it looks like the moment to add all three.
##########
grails-gsp/grails-web-taglib/src/main/groovy/org/grails/web/taglib/WebRequestTemplateVariableBinding.java:
##########
@@ -72,6 +72,13 @@ public Object evaluate(GrailsWebRequest webRequest) {
return webRequest.getServletContext();
}
});
+ // The same object as `application`, under the name the servlet API
calls it. A page reading
+ // servletContext got nothing at all before, since nothing bound the
name.
+ m.put("servletContext", new LazyRequestBasedValue() {
Review Comment:
This is the name the PR newly binds, and nothing exercises it.
`WebRequestTemplateVariableBinding` has no spec of its own, `'the servlet
scopes carry their own types'` covers `request`, `response`, `session` and
`application` but not `servletContext`, and the example app's
`frameworkNames.gsp` reads `controllerName`, `actionName`, `flash` and
`params`. `webRequest` — the other name typed here for the first time — is in
the same position.
Both do work: I compiled `${servletContext.serverInfo}` and
`${webRequest.currentRequest}` statically and the accessors resolve with the
types the guide's table gives them. A row each in that `where:` block would
cover the typing, and `frameworkNames.gsp` is the natural place to prove the
binding at render time — it is the same object as `application`, so
`${application.serverInfo}` and `${servletContext.serverInfo}` in one page is
the assertion.
Worth noting for the risk side of it: the new entry cannot shadow anything,
since `getVariable` checks the model and the request attributes before the lazy
map.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageTypeCheckingExtension.groovy:
##########
@@ -39,40 +41,91 @@ import org.codehaus.groovy.transform.stc.StaticTypesMarker
*/
class GroovyPageTypeCheckingExtension extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
+ /**
+ * The operators the class writer emits directly, rather than leaving to a
call site.
+ *
+ * <p>These are the ones that cannot be handed a receiver of no known
type. A comparison or a
+ * logical operator is not among them: those report a type error of their
own, which is an answer
+ * a page can act on, so the receiver of one is still resolved the way
anything else is.</p>
+ */
+ private static final Set<Integer> WRITTEN_INTO_THE_CLASS = [
+ Types.LEFT_SQUARE_BRACKET,
+ Types.PLUS, Types.MINUS, Types.MULTIPLY, Types.DIVIDE,
Types.INTDIV, Types.MOD, Types.POWER,
+ Types.PLUS_EQUAL, Types.MINUS_EQUAL, Types.MULTIPLY_EQUAL,
Types.DIVIDE_EQUAL] as Set
Review Comment:
Three gaps in this set, each producing the outcome it exists to prevent.
`Types.MOD` is not the token this sees for `%`: with it in the set `${n %
2}` is still missed, and adding `Types.REMAINDER` is what catches it. So `%` is
listed here and not covered. (`Types.INTDIV` looks to be in the same position —
nothing in the language emits it.)
The shift and bitwise operators are absent, and the static writer emits them
the same way it emits `getAt`:
| page | error |
| --- | --- |
| `${n % 2}`, `<% n %= 2 %>` | `Cannot access method: remainder() of class:
java.lang.Object` |
| `${l << 1}`, `<% l <<= 1 %>` | `Cannot access method: leftShift() of
class: java.lang.Object` |
| `${n >> 1}`, `${n >>> 1}` | `rightShift()`, `rightShiftUnsigned()` |
| `${n & 1}`, `${n \| 1}`, `${n ^ 1}` | `and()`, `or()`, `xor()` |
| `<% n **= 2 %>` | `power()` |
That message comes from
`org.codehaus.groovy.classgen.asm.sc.StaticInvocationWriter`, i.e. the class
writer — the failure `'an operator on a closure parameter of no known type is
reported by type checking'` asserts against with `!message.contains('Cannot
access method')`.
`<=>` is worse than an unhelpful message. It crashes the compiler:
```
General error during canonicalization: Index 0 out of bounds for length 0
at
org.codehaus.groovy.transform.sc.transformers.BinaryExpressionTransformer.transformRelationComparison(BinaryExpressionTransformer.java:310)
```
for `<g:set var="n" value="${1}"/>${n <=> 2}`, for `${undeclared <=> 2}`,
and under a declared model too. Giving the receiver a type — `<g:set
type="int">` or a `model` declaration — makes every row above compile and
render, which is what puts the cause on the receiver rather than the operator.
Adding the missing tokens turns all of them into the page-level message:
```groovy
Types.PLUS_EQUAL, Types.MINUS_EQUAL, Types.MULTIPLY_EQUAL,
Types.DIVIDE_EQUAL,
Types.MOD_EQUAL, Types.POWER_EQUAL, Types.REMAINDER,
Types.REMAINDER_EQUAL,
Types.LEFT_SHIFT, Types.RIGHT_SHIFT, Types.RIGHT_SHIFT_UNSIGNED,
Types.BITWISE_AND, Types.BITWISE_OR, Types.BITWISE_XOR,
Types.LEFT_SHIFT_EQUAL, Types.RIGHT_SHIFT_EQUAL,
Types.RIGHT_SHIFT_UNSIGNED_EQUAL,
Types.BITWISE_AND_EQUAL, Types.BITWISE_OR_EQUAL,
Types.BITWISE_XOR_EQUAL,
Types.COMPARE_TO] as Set
```
I ran this: every row reports `The type of [n] is not known here, and [%]
cannot be applied to it`, the `<=>` crash becomes the same message, and
`GspCompileStaticConfigSpec` stays green. `<`, `>`, `<=`, `>=` and `==` still
compile on a receiver of no known type, so what the doc comment says about
comparisons holds for them — `<=>` is the one that is not left to a call site.
##########
grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:
##########
@@ -0,0 +1,245 @@
+////
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+////
+
+GSP pages are compiled dynamically by default. A page can instead be
statically compiled, so that expressions and scriptlets in the page are type
checked at compile time and dispatched without dynamic lookup at render time.
+
+Static compilation is enabled per page with the `compileStatic` page directive:
+
+[,xml]
+----
+<%@ page compileStatic="true" %>
+----
+
+==== Declaring the Model
+
+A page can state what it is rendered with, using the `model` page directive,
which names and types each variable:
+
+[,xml]
+----
+<%@ page model="Book book" %>
+<h1>${book.title}</h1>
+----
+
+Separate multiple declarations with semicolons, or use a multi-line value:
+
+[,xml]
+----
+<%@ page model="Book book; List<Review> reviews" %>
+----
+
+Because a declared model is what makes a page type checkable, the `model`
directive enables static compilation on its own — `compileStatic="true"` is
implied and does not need to be given as well.
+
+Declaring a model also states that the model is complete, so reading any name
outside it is reported rather than left to the render:
+
+----
+The variable [publisher] is undeclared.
+----
+
+A page that declares no model has stated nothing, and reads what it is
rendered with exactly as a dynamically compiled page does. This is what makes
static compilation adoptable for views that were never written with it in mind:
what the page does say is checked, and what it does not say still works.
+
+==== Names Supplied by the Framework
+
+The names bound into every page do not need to be declared. Most carry their
real types and are checked like anything else:
+
+[,xml]
+----
+<%@ page compileStatic="true" %>
+<g:if test="${params.id}">${request.contextPath} for
${controllerName}/${actionName}</g:if>
+----
+
+|===
+|Name |Type
+
+|`params`
+|link:{apiDocs}grails/util/TypeConvertingMap.html[TypeConvertingMap], so
`params.id` and `params.int('max')` both resolve
+
+|`flash`
+|link:{apiDocs}grails/web/mvc/FlashScope.html[FlashScope]
+
+|`request`, `response`, `session`
+|`HttpServletRequest`, `HttpServletResponse`, `HttpSession`
+
+|`application`, `servletContext`
+|`ServletContext` — the same object under both names
+
+|`webRequest`
+|`GrailsWebRequest`
+
+|`controllerName`, `actionName`, `namespace`
+|`String`
+|===
+
+
+Being typed, they are checked: `${request.contextPathTypo}` is a compilation
error. The typed attribute converters described in
link:theWebLayer.html#typeConverters[Simple Type Converters] resolve on them
for the same reason.
+
+`grailsApplication` and `applicationContext` are the exception and are
resolved dynamically. What pages read from them is answered at runtime rather
than declared — `grailsApplication.controllerClasses` is matched against the
artefact types an application happens to have, which no type can enumerate — so
they are read the way a dynamically compiled page reads them.
+
+A page may still declare a model variable using one of these names, and the
declared type then applies for that page — declaring `Map params` gives the
page a `Map`, with no other type imposed on it.
+
+==== Names a Page Introduces
+
+A page introduces names of its own through the `var` and `status` attributes
of the tags it calls, and those do not need declaring either:
+
+[,xml]
+----
+<%@ page compileStatic="true" %>
+<g:set var="total" value="${books.size()}"/>
+<g:each in="${books}" var="book" status="i">${i}. ${book.title}</g:each>
+Total: ${total}
+----
+
+What such a name holds is decided by the tag when the page renders, so it is
read dynamically rather than reported. Reading a member from it is fine;
applying an operator to it is not, because nothing established what it holds:
+
+[,xml]
+----
+Total: ${total + 1}
+----
+----
+The type of [total] is not known here, and an operator cannot be applied to it.
+----
+
+Giving the name a type answers that, and the value is converted to it:
+
+[,xml]
+----
+<g:set type="int" var="total" value="${books.size()}"/>
+Total: ${total + 1}
+----
+
+`type` declares a local of that type and still writes the name into the page
scope, so anything reading it afterwards is unaffected. It is only meaningful
alongside `value`: a tag given a body or a `bean` produces its value as it
runs, so there is nothing to declare from, and combining them is an error.
+
+Where the name is only read by the page itself, `<g:def>` declares it without
the scope write:
+
+[,xml]
+----
+<g:def type="int" var="total" value="${books.size()}"/>
+----
+
+==== Requiring Every Page to Declare What It Reads
+
+Reporting a name a page never declared can be asked for everywhere, rather
than only in the pages that declare a model:
+
+[source,yaml]
+.grails-app/conf/application.yml
+----
+grails:
+ views:
+ gsp:
+ compileStatic: true
+ compileStaticConfig:
+ strict: true
+----
+
+or from the build:
+
+[source,groovy]
+.build.gradle
+----
+grails {
+ compileStatic {
+ gsp = true
+ strictGsp = true
+ }
+}
+----
+
+With this on, every page is held to what it declares, which is the strongest
guarantee available and the most work to adopt. Without it, only the pages that
declare a model are.
+
+==== Enabling Static Compilation for Every Page
+
+Rather than adding the directive to each page, static compilation can be made
the default for an entire application with the `grails.views.gsp.compileStatic`
setting:
+
+[source,yaml]
+.grails-app/conf/application.yml
+----
+grails:
+ views:
+ gsp:
+ compileStatic: true
+----
+
+The setting applies both to pages precompiled by the build and to pages
compiled on the fly while the application runs in development mode, so a page
behaves the same way in both.
+
+The same thing can be asked for from the build instead, which is useful where
a project prefers to keep build options together:
+
+[source,groovy]
+.build.gradle
+----
+grails {
+ compileStatic {
+ gsp = true
+ }
+}
+----
+
+This states the same `grails.views.gsp.compileStatic` setting, for both the
pages the build compiles ahead of time and the application it runs, so a page
compiles the same way in either. Where the build states it, it takes precedence
over `application.yml` — the same order a system property takes over
configuration in a running application. Set it in one place or the other rather
than both.
+
+NOTE: `gsp` is not included in the `compileStatic { all = true }` shortcut
described in
link:staticTypeCheckingAndCompilation.html#grailsCompileStatic[GrailsCompileStatic].
The artefact opt-ins there fail on code that is doubtful anyway, whereas this
one fails on any page reading a model variable it has not declared, so it is
never enabled as a side effect of asking for everything.
Review Comment:
This NOTE still carries the pre-`strict` rationale that 9572cf6 corrected
eleven lines below it. "this one fails on any page reading a model variable it
has not declared" is the statement the new NOTE at line 204 contradicts — "A
page that reads a model variable it has not declared still compiles" — and that
line 54 contradicts as well.
The conclusion is still right, only the reason is stale. Something closer
to: the artefact opt-ins fail on code that is doubtful anyway, whereas this one
reports an operator applied to a value whose type nothing established, which
most applications that never declared a page model have somewhere. Or keep the
fact and drop the clause after the comma.
##########
grails-test-examples/gsp-compile-static/grails-app/views/demo/index.gsp:
##########
@@ -0,0 +1,35 @@
+<%--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+--%>
+<%@ page model="String title; java.util.List<gspstatic.Book> books" %>
+<!doctype html>
+<html>
+<head><title>${title}</title></head>
+<body>
+<h1>${title}</h1>
+<g:def type="int" var="total" value="${0}"/>
Review Comment:
`total` is declared here and never read. In the page that is the worked
example of the feature it reads as a leftover — either use it, or drop it and
leave the typed-local case to `declared.gsp`, which reads what its `g:set
type=` declares.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
