jdaugherty commented on code in PR #16139:
URL: https://github.com/apache/grails-core/pull/16139#discussion_r3827113471
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePlugin.groovy:
##########
@@ -54,6 +55,26 @@ class GroovyPagePlugin implements Plugin<Project> {
}
}
+ /**
+ * Whether pages are compiled statically, from {@code grails {
compileStatic { gsp } }}.
+ *
+ * <p>Resolved when the task runs rather than now: this plugin is applied
on its own as well as
+ * alongside the one that registers the {@code grails} extension, and
there is no ordering between
+ * them. Where the extension is absent, pages compile the way
configuration alone says.</p>
+ */
+ private static Provider<Boolean> resolveCompileStaticPages(Project
project) {
+ project.provider {
+
project.extensions.findByType(GrailsExtension)?.compileStatic?.gsp?.getOrElse(false)
?: false
Review Comment:
These two providers capture `project` and read `project.extensions` when
they are resolved, and they feed `compileGroovyPages.compileStatic` /
`compileStaticStrict`, which are `@Input`. That makes `Project` reachable from
a task input, which is the pattern the configuration cache rejects.
It is latent today — `org.gradle.configuration-cache` is commented out in
`gradle.properties` — but it is inconsistent with the rest of this PR and with
the surrounding code: `GrailsGspCompileStaticProvider` deliberately holds only
the `GrailsCompileStaticOptions` object, and `GrailsAppBaseDirProvider` carries
a comment explaining that it exists to keep a plain `File` on the task rather
than anything project-scoped.
Since the reason for deferring is plugin-application ordering, wiring it
when the extension appears keeps the laziness without capturing the project:
```groovy
project.plugins.withType(GrailsGradlePlugin) {
GrailsCompileStaticOptions options =
project.extensions.getByType(GrailsExtension).compileStatic
compileGroovyPages.configure {
it.compileStatic.set(options.gsp.orElse(false)) }
// ... and compileStaticStrict / compileWebappGroovyPages likewise
}
```
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/tags/GroovyDefTag.java:
##########
@@ -59,10 +71,18 @@ public void doStartTag() {
if (typeName.equals("def") || typeName.equals("Object")) {
out.println(expr);
} else {
- out.println(typeName + ".cast(" + expr + ")");
+ // Cast through the wrapper for a primitive: Class.cast on a
primitive class throws
+ // whatever it is handed, so int.cast(1) fails where
Integer.cast(1) is what was meant.
+ // The declared type stays primitive, and the result unboxes into
it.
+ out.println(castingTypeFor(typeName) + ".cast(" + expr + ")");
Review Comment:
Same `Class.cast` issue as the typed `g:set` — `(" + typeName + ") (" + expr
+ ")"` is the coercing form, and the primitive-wrapper map goes away with it.
Separately, and pre-existing rather than introduced here: `expr` is not
parenthesised on this path, so a GString value emits malformed code. `<g:def
type="String" var="s" value="Total: ${1 + 1}"/>` produces `String s=(String)
Total: ${1 + 1}` and fails with `Cannot find matching method
java.lang.Class#call(...)`. Since this PR is putting `g:def type=` in front of
users as the recommended way to give a page-local value a type — and the
welcome page now leans on it — it seems worth fixing in the same pass.
##########
grails-test-examples/gsp-compile-static/src/integration-test/groovy/gspstatic/GspCompileStaticSpec.groovy:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+package gspstatic
+
+import grails.testing.mixin.integration.Integration
+import org.grails.gsp.CompileStaticGroovyPage
+import org.grails.gsp.GroovyPagesTemplateEngine
+import org.springframework.beans.factory.annotation.Autowired
+import spock.lang.Specification
+
+/**
+ * An application whose pages are compiled statically, rendered through a
running server.
+ *
+ * <p>Static compilation of a page is settled by the build rather than by the
page, so what proves it
+ * is an application that turned it on and pages that only compile if it
happened. Each page here uses
+ * something the dynamic path would have resolved at render time - a declared
model, a name the
+ * framework binds, a tag call, a typed local - and the class the page
compiled to is checked as well,
+ * because a page that renders correctly renders correctly either way.
+ */
+@Integration
+class GspCompileStaticSpec extends Specification {
+
+ @Autowired
+ GroovyPagesTemplateEngine templateEngine
+
+ void 'a page that declares its model is compiled statically and renders'()
{
+ when:
+ String body = new
URL("http://localhost:${serverPort}/demo/declared";).text
+
+ then: 'the model is read with the types it declared'
+ body.contains('<p id="title">Ubik</p>')
+ body.contains('<p id="pages">224</p>')
+
+ and: 'arithmetic on a declared int is done on the int'
+ body.contains('<p id="total">672</p>')
+
+ and: 'a typed g:set declares a local and still writes the scope'
+ body.contains('<p id="upper">UBIK</p>')
+ }
+
+ void 'a page reading the names the framework binds is compiled statically
and renders'() {
+ when:
+ String body = new
URL("http://localhost:${serverPort}/demo/frameworkNames?n=7";).text
+
+ then:
+ body.contains('<p id="controller">demo</p>')
+ body.contains('<p id="action">frameworkNames</p>')
+ body.contains('<p id="flash">from flash</p>')
+
+ and: 'params keeps the conversions it declares, so int() is not a
dynamic call'
+ body.contains('<p id="param">7</p>')
+
+ and: 'a tag call still runs'
+ body.contains('<p id="link">/demo/declared</p>')
+ }
+
+ void 'a page using closures and a tag library renders what it computed'() {
+ when:
+ String body = new URL("http://localhost:${serverPort}/demo/index";).text
+
+ then:
+ body.contains('<li id="book-0">Dune has 412 pages</li>')
+ body.contains('<li id="book-1">Emma has 474 pages</li>')
+ body.contains('<p id="count">2</p>')
+ body.contains('<p id="longest">Emma</p>')
+
+ and: 'a tag library in its own namespace is reached'
+ body.contains('<p id="shout">QUIET</p>')
+ }
+
Review Comment:
Good addition — an application that actually turns the option on is much
stronger evidence than the unit specs alone.
One gap worth closing while it is fresh: nothing here asserts that the
*forked page compiler* produced statically compiled classes. `pageClassFor`
goes through `templateEngine.createTemplate(view)`, which resolves through the
page locator at test time, so which class it hands back depends on whether
`compileGroovyPages` output is on the integration-test runtime classpath rather
than on anything the test states. And on the build side,
`GroovyPagePluginFunctionalSpec` only asserts that the task's `compileStatic`
input property is `true`, not that the fork honoured it.
Given that the central claim of the build option is that it "reaches both
places a page is compiled", it would be good to have one assertion against a
class the `compileGroovyPages` task actually wrote — loading the precompiled
`gsp_...` class from the task's output directory and checking
`CompileStaticGroovyPage.isAssignableFrom(...)` would do it. As it stands both
existing assertions can be satisfied by the runtime path alone.
Also a smaller note: the two cases in `'a page that declares a model is
compiled statically because it declared one'` are, as the `where:` comment
says, static "whatever the build asked for" — so `frameworkNames.gsp` is the
only page here that tests the build flag at all.
##########
grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:
##########
@@ -0,0 +1,218 @@
+////
+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.
+
+==== 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.
+
+An individual page always wins over either, and can opt back out:
+
+[,xml]
+----
+<%@ page compileStatic="false" %>
+----
+
+[NOTE]
+====
+Enabling this for an existing application is not a transparent change: every
page that references an undeclared model variable will fail to compile until it
declares its model. Introduce the setting once the views it covers declare
their models, and use `compileStatic="false"` for the pages that are not ready
yet.
Review Comment:
This reads as a leftover from before the non-strict default landed, and it
now contradicts the page itself. Line 97 says a page that declares no model
"reads what it is rendered with exactly as a dynamically compiled page does",
and `GroovyPageTypeCheckingExtension` only reports undeclared names when
`strict` is set — so referencing an undeclared model variable does *not* fail
to compile by default, and the advice to "introduce the setting once the views
it covers declare their models" undersells how adoptable the feature actually
is.
The two things that genuinely are not transparent are worth naming in its
place, since they are what an existing application will actually hit:
- an operator applied to a value of no known type (`${a + b}`,
`${rows[0]}`), which is reported regardless of `strict` — this is the one that
will account for most of the work, and `+` on an undeclared name is common in
real views;
- a member that does not exist on a framework-supplied name, now that those
carry their real types — which the second paragraph of this note already covers
well.
##########
grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:
##########
@@ -0,0 +1,218 @@
+////
+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.
+
+==== 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
+----
Review Comment:
The `type` attribute is missing from the guide entirely. The section
explains that a name introduced by a tag "is read dynamically rather than
reported", but never mentions that `type` is how a page opts out of that — even
though the compiler error users will hit tells them to do exactly that:
*"Declare it in the model directive, or give it a type where it is
introduced."* As written, someone who reads that message and comes here finds
nothing about giving a name a type.
The PR description covers this well (the `g:def` vs `g:set type=`
distinction, and that `type` is only valid alongside `value`); it just has not
made it into the guide.
`grails-doc/src/en/ref/Tags - GSP/set.adoc` also needs updating — it carries
an explicit attribute list (`var`, `value`, `bean`, `scope`) and `type` is a
new public attribute on a documented tag, so per the project's docs requirement
it should be listed there with the value-only restriction noted.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java:
##########
@@ -1235,6 +1430,67 @@ private void flushBufferedWhiteSpace() {
currentlyBufferingWhitespace = false;
}
+ /**
+ * Declares the variable a {@code <g:set type="...">} names, so that the
rest of the page reads it
+ * with a type rather than through the page binding.
+ *
+ * <p>The tag keeps doing what it did: the declaration is written first
and the tag is then called
+ * with the declared variable as its value, so the write into the scope
still happens and
+ * {@code scope} still decides where. What it adds is that the page itself
no longer has to look
+ * the name up to read it.</p>
+ *
+ * <p>Only a {@code value} can be typed. Where the value is the tag's body
or a {@code bean}, there
+ * is no expression to declare the variable from -- both are produced when
the tag runs -- so
+ * asking for a type there is rejected rather than quietly ignored.</p>
+ */
+ private void writeTypedSetDeclaration(String ns, String tagName,
Map<String, String> attrs) {
+ if (!GroovyPage.DEFAULT_NAMESPACE.equals(ns) ||
!SET_TAG_NAME.equals(tagName)) {
+ return;
+ }
+ String type = attributeText(attrs, TYPE_ATTRIBUTE);
+ if (type == null) {
+ return;
+ }
+ String var = attributeText(attrs, VAR_ATTRIBUTE);
+ if (GrailsStringUtils.isBlank(var)) {
+ throw new GrailsTagException("Tag [set] with a [type] needs a
[var] naming what to declare",
+ pageName, getCurrentOutputLineNumber());
+ }
+ Object value = attrs.get("\"value\"");
+ if (value == null) {
+ throw new GrailsTagException("Tag [set] can only be given a [type]
together with a [value]; " +
+ "the body and the [bean] attribute are produced when the
tag runs, so there is nothing " +
+ "to declare the variable from", pageName,
getCurrentOutputLineNumber());
+ }
+ attrs.remove("\"" + TYPE_ATTRIBUTE + "\"");
+ // A name typed once is declared; typing it again assigns to what was
declared. Declaring it
+ // twice would not compile, where the untyped tag simply writes the
scope again.
+ String declaration = declareTypedSetVariable(var) ? type + " " + var :
var;
+ out.println(declaration + " = " + castingTypeFor(type) + ".cast(" +
getExpressionText(value.toString()) + ")");
Review Comment:
`Class.cast` does not coerce, so this is the same defect the `int.cast(...)`
fix addressed, just narrowed rather than removed. Swapping the primitive for
its wrapper only fixes exact-type boxing; every other conversion Groovy would
have performed still fails, and it fails at render after compiling clean.
Measured on this branch, all of these compile with no error and then throw
`ClassCastException` when the page renders:
| tag | outcome |
| --- | --- |
| `<g:set type="long" var="n" value="${2}"/>` | `Long.cast(Integer)` → CCE
at render |
| `<g:set type="double" var="d" value="${2}"/>` | `Double.cast(Integer)` →
CCE at render |
| `<g:set type="String" var="s" value="Total: ${n}"/>` |
`String.cast(GString)` → CCE at render |
| `<g:set type="java.util.List" var="l" value="${s.split(',')}"/>` |
`List.cast(String[])` → CCE at render |
A declared `long` or `double` initialised from an integer literal is about
as ordinary as this tag gets, and `type="String"` over a GString is the most
natural thing a page author would write. Worth noting that the passing cases in
`GspCompileStaticConfigSpec` are exactly the shapes that survive `Class.cast` —
`type="long"` is tested as `${2L}` and `type="double"` as `${1.5d}`, so the
suffixes are load-bearing and the test reads as agreeing with the bug rather
than pinning the behaviour.
Emitting a Groovy cast fixes it, and is what
`writeFrameworkSuppliedAccessors()` in this same PR already does for the
framework-supplied names:
```java
out.println(declaration + " = (" + type + ") " +
getExpressionText(value.toString()));
```
I ran this: `long`/`${2}` renders `2`, `double`/`${2}` renders `2.0`, and
the `String[]`-into-`List` case becomes an honest compile-time `Inconvertible
types: cannot cast java.lang.String[] to java.util.List` instead of a clean
compile and a render-time crash — which is the better outcome, since the whole
point of the option is to move failures to compile time. All 71 existing
`GspCompileStaticConfigSpec` tests still pass with the change.
`PRIMITIVE_WRAPPERS` and `castingTypeFor` then become unnecessary here.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageTypeCheckingExtension.groovy:
##########
@@ -116,16 +222,99 @@ class GroovyPageTypeCheckingExtension extends
GroovyTypeCheckingExtensionSupport
if (expression.thisExpression || expression.superExpression) {
return false
}
+ if (expression.getNodeMetaData(StaticTypesMarker.DYNAMIC_RESOLUTION)
== null) {
+ return false
+ }
+ // An operator applied to something of no known type is reported
whatever was asked for, and
+ // before the exemptions below, which is the whole point of it being
here: the receiver
+ // resolves through the getProperty this page inherits, and the class
writer fails with a
+ // GroovyBugError rather than an error when it writes an operator
against such a receiver.
+ // Reporting it is what keeps the compilation from reaching that.
+ if (isOperatorReceiver(expression)) {
+ return true
+ }
+ if (!currentScope.strict) {
+ // A page that has not declared its model reads the model it was
rendered with, which is
+ // how a page that is not compiled statically reads it. Only
strictness asks for it back.
+ return false
+ }
if (currentScope.allowedTagLibs.contains(expression.name)) {
return false
}
- expression.getNodeMetaData(StaticTypesMarker.DYNAMIC_RESOLUTION) !=
null
+ if (currentScope.pageScopeVariables.contains(expression.name)) {
+ return false
+ }
+ true
+ }
+
+ /**
+ * Whether the type of what a member is read from is not known, which is
what {@code Object} means
Review Comment:
This Javadoc block describes `isUnknownReceiver`, but it is stacked directly
above the Javadoc for `collectOperatorReceivers`, so `collectOperatorReceivers`
has two doc comments and `isUnknownReceiver` (line 290) has none — it carries
an inline comment instead. The `FRAMEWORK_DYNAMIC_NAMES` instance of this got
fixed; this one is still stranded. Moving it down onto `isUnknownReceiver`
finishes it.
##########
grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:
##########
@@ -0,0 +1,218 @@
+////
+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.
+
+==== 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.
+
+An individual page always wins over either, and can opt back out:
+
+[,xml]
+----
+<%@ page compileStatic="false" %>
+----
+
+[NOTE]
+====
+Enabling this for an existing application is not a transparent change: every
page that references an undeclared model variable will fail to compile until it
declares its model. Introduce the setting once the views it covers declare
their models, and use `compileStatic="false"` for the pages that are not ready
yet.
+
+A model variable sharing a name with one the framework supplies -- `params`,
`request`, `response`, `session`, `application`, `servletContext`, `flash`,
`webRequest`, `controllerName`, `actionName` or `namespace` -- is no longer
read as the model supplied it: the page reads it with the type the framework
value has. Reading a member that type does not have is a compilation error the
build reports. Passing a value of another type is not: it fails at render with
a `GroovyCastException`, so precompiling the views will not catch it. Rename
the model key, or declare it in the `model` directive to keep your own type.
+====
+
+==== Tag Libraries
+
+Tags are dispatched dynamically even in a statically compiled page, but only
for namespaces the page knows about. The `g`, `tmpl`, `f`, `asset` and `plugin`
namespaces are allowed by default:
+
+[,xml]
+----
+<%@ page compileStatic="true" %>
+${g.message(code: 'book.list.title')}
+----
+
+A tag library in any other namespace must be declared, otherwise its namespace
is treated as an undeclared variable. Declare the extra namespaces for a single
page with the `taglibs` page directive:
+
+[,xml]
+----
+<%@ page compileStatic="true" taglibs="myns, othertags" %>
+${myns.formatPrice(value: book.price)}
+----
+
+or for every page with the `grails.views.gsp.compileStaticConfig.taglibs`
setting, given either as a list or as a comma separated string:
+
+[source,yaml]
+.grails-app/conf/application.yml
+----
+grails:
+ views:
+ gsp:
+ compileStaticConfig:
+ taglibs:
+ - myns
+ - othertags
+----
+
+The declared namespaces are added to the defaults rather than replacing them.
+
+TIP: Static compilation of GSP pages is independent of the `grails {
compileStatic { } }` build options described in
link:staticTypeCheckingAndCompilation.html#grailsCompileStatic[GrailsCompileStatic],
which apply to controllers, services and tag library classes.
Review Comment:
This TIP is no longer true, and it contradicts the "Enabling Static
Compilation for Every Page" section 40 lines above it, which documents `grails
{ compileStatic { gsp = true } }`. GSP static compilation is now *reached
through* the `grails { compileStatic { } }` block; what remains true is the
narrower point that `all = true` does not include `gsp`, which the NOTE at line
211 already makes.
Suggest either dropping it or rewriting it to say that the artefact opt-ins
in that block apply to controllers, services and tag library classes, while
`gsp` applies to pages and is not covered by `all`.
--
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]
