codeconsole commented on code in PR #16139: URL: https://github.com/apache/grails-core/pull/16139#discussion_r3836309813
########## 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: Right — the fact stands, the reason was stale. Reworded in c74ae38 to say it reports an operator applied to a value whose type nothing established, which most applications that never declared a page model have somewhere. ########## 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: Fixed in c204890. The task now prints `PAGE_COMPILER_STATIC` and `WEBAPP_PAGE_COMPILER_STATIC`, and the test asserts both are false — which is the claim its name makes, and the one project where `wireCompileStaticOptions` never finds the extension. ########## 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: Added all three in c74ae38 — `grails.views.gsp.compileStatic`, `.compileStaticConfig.strict` and `.compileStaticConfig.taglibs`, beside `layout.preprocess` in `grails-gsp/plugin`. ########## 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: Both covered in c204890. `${servletContext.serverInfo}` and `${webRequest.currentRequest}` are rows in that `where:` block, and `frameworkNames.gsp` reads `servletContext.serverInfo == application.serverInfo` so the binding is proved rather than assumed. ########## 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: Dropped. `declared.gsp` already reads what its `g:set type=` declares, so the typed-local case is covered without a leftover in the worked example. -- 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]
