sbglasius commented on code in PR #16139:
URL: https://github.com/apache/grails-core/pull/16139#discussion_r3819704947
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageTypeCheckingExtension.groovy:
##########
@@ -89,17 +142,65 @@ class GroovyPageTypeCheckingExtension extends
GroovyTypeCheckingExtensionSupport
if (isAllowedDynamicTaglibNamespace(objectExpression)) {
return makeDynamic(call)
}
+ // A call on a receiver that resolved to Object is a call on
something this page never knew
+ // the type of. Reporting it says nothing the page can act on, and
the same call in a page
+ // that is not compiled statically runs; it is only reported where
strictness was asked for.
+ if (!currentScope.strict &&
+ (isPageScopeVariable(objectExpression) ||
isUnknownReceiver(objectExpression))) {
+ return makeDynamic(call)
+ }
if (objectExpression instanceof VariableExpression &&
isUndeclaredDynamicVariable(objectExpression)) {
reportUndeclaredDynamicVariable(objectExpression)
return makeDynamic(call)
}
}
+ beforeVisitMethod { MethodNode methodNode ->
+ currentScope.operatorReceivers =
collectOperatorReceivers(methodNode)
+ }
+
afterVisitMethod { MethodNode methodNode ->
reportUndeclaredDynamicVariables(methodNode)
+ reportOperatorsOnValuesOfNoKnownType(methodNode)
}
}
+ /**
+ * Reports an operator applied to a value of no known type, whatever gave
it that type.
+ *
+ * <p>The receiver of one is held back from being resolved dynamically,
which reports the names
+ * that go through this extension. It does not reach a closure parameter:
nothing declared it and
+ * nothing resolved it, type checking simply inferred {@code Object} for
it, so the operator
+ * passes the check and fails in the class writer instead, which is the
wrong place to hear about
+ * it. Asking the type of every such receiver once the method has been
visited catches those too,
+ * and reports them the same way.</p>
+ */
+ private void reportOperatorsOnValuesOfNoKnownType(MethodNode methodNode) {
+ methodNode.code?.visit(new CodeVisitorSupport() {
+ @Override
+ void visitBinaryExpression(BinaryExpression expression) {
+ if (expression.operation.type in WRITTEN_INTO_THE_CLASS &&
isUnknownReceiver(expression.leftExpression)) {
+ reportOperatorOnUnknownType(expression)
+ }
+ super.visitBinaryExpression(expression)
+ }
+ })
+ }
+
+ private void reportOperatorOnUnknownType(BinaryExpression expression) {
+ String described = describe(expression.leftExpression)
+ if
(currentScope.undeclaredDynamicVariables.add("operator:$described")) {
+ typeCheckingVisitor.addStaticTypeError(
+ "The type of ${described} is not known here, and
[${expression.operation.text}] cannot be " +
+ 'applied to it. Declare it in the model directive,
or give it a type where it is ' +
+ 'introduced.', expression.leftExpression)
+ }
+ }
+
+ private static String describe(Expression expression) {
+ expression instanceof VariableExpression ? "[${((VariableExpression)
expression).name}]" : 'this value'
Review Comment:
`describe()` returns the constant `'this value'` for non-variable
expressions and the dedup key is built from it, so only the first
operator-on-unknown-type error in a page is ever reported.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java:
##########
@@ -200,6 +236,28 @@ public class GroovyPageParser implements Tokens {
private File keepGeneratedDirectory;
private Set<String> allowedTaglibNamespaces = new
LinkedHashSet<>(DEFAULT_TAGLIB_NAMESPACES);
+ /**
+ * The {@code var} and {@code status} attributes of the tags this page
calls, which are the names
+ * the page introduces for itself.
+ */
+ private final Set<String> pageScopeVariables = new LinkedHashSet<>();
+
+ /** Whether what a page never declared fails the compilation rather than
resolving at render time. */
+ private boolean compileStaticStrict;
+
+ /**
+ * Matches the {@code var} and {@code status} attributes of a namespaced
tag, which is how a page
+ * names something it introduces: {@code <g:set var="total"/>}, {@code
<g:each var="book"
+ * status="i">}, {@code <g:eachError var="error">}.
+ *
+ * <p>Read from the page source rather than from the parsed attributes
because attributes are
+ * parsed only on the pass that writes the class, by which point the
annotation carrying these
+ * names has already been written. Matching a name that turns out not to
be a page scope variable
+ * costs only that the name resolves dynamically, so the pattern errs
towards matching.</p>
+ */
+ private static final Pattern PAGE_SCOPE_VARIABLE_PATTERN = Pattern.compile(
Review Comment:
`PAGE_SCOPE_VARIABLE_PATTERN`'s `[^>]*?` stops at the first `>`, so `<g:each
in="${l.findAll { it.x > 1 }}" var="book">` never registers `book`; under a
declared model (strict implied) or `strictGsp` that's a spurious "variable
[book] is undeclared".
##########
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 {
+ /**
+ * Names the framework binds into every page whose members are answered at
runtime rather than
+ * declared: {@code grailsApplication.controllerClasses} is matched
against a name and answered
+ * from the artefact handlers, an open set no interface enumerates, and
what pages read from the
+ * application context belongs to an implementation rather than to the
interface. Every other name
+ * the framework binds is declared on the page with the type it holds, and
is checked normally.
+ */
+ /**
Review Comment:
What Javadoc block should stay and which should go?
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageParser.java:
##########
@@ -1235,6 +1396,64 @@ 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 + "\"");
+ out.println(type + " " + var + " = " + castingTypeFor(type) + ".cast("
+ getExpressionText(value.toString()) + ")");
Review Comment:
I don't know if this is a feature or a bug, but typed `g:set` emits a real
local declaration, so two typed sets of the same var in one block scope fail
with "scope already contains a variable"; the untyped form of `g:set` rebinds
happily.
--
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]
