codeconsole commented on code in PR #16139: URL: https://github.com/apache/grails-core/pull/16139#discussion_r3827316645
########## 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: Right, that was left over from before the non-strict default. Rewritten in 9572cf6 to lead with the operator case as the one that accounts for most of the work, and to say that a page reading an undeclared variable still compiles. ########## 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: Rewritten rather than dropped: the block applies to controllers, services and tag libraries, `gsp` applies to pages, and `all = true` is the one that does not include 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 +---- Review Comment: Added in 9572cf6 — in the section the error message sends people to, with `g:def` beside it and the value-only restriction. `set.adoc` now lists `type` in its attribute list with the same restriction. ########## 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: Fixed in 16c7624, wired as you suggested. The tasks hold the `GrailsCompileStaticOptions` object rather than a provider reaching through the project, with `false` set at registration so the plugin applied on its own still has a value. ########## 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: Moved onto `isUnknownReceiver` in 16c7624, with the inline comment folded into it. -- 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]
