[
https://issues.apache.org/jira/browse/GROOVY-12472?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131659#comment-18131659
]
ASF GitHub Bot commented on GROOVY-12472:
-----------------------------------------
paulk-asert commented on PR #2998:
URL: https://github.com/apache/groovy/pull/2998#issuecomment-6393228177
Thanks Daniel.
1. A recursive return edge contributes the declared type (Object), so the
result is Object rather than something wider than the base cases: nothing is
inferred for such a method. testRecursiveCallUsesDeclaredType pins that
(countdown(n) returning 'done' or countdown(n - 1) still fails on
.toUpperCase()).
2. Agreed.
3. Good idea. I've added a test that pins pick(name()) selecting
pick(String) under @CompileStatic.
4. Agreed, that's why it's spelled out in COMPATIBILITY.md.
5. On checking, the exclusion isn't needed. An enum with constant bodies is
already non-final by the time type checking runs, in the class file as well,
and a plain enum is final. So the final-class test already does the right
thing, and the extra !type.isEnum() only kept methods of plain enums from being
inferred. I've removed it and added tests for both shapes: a plain enum gets
the inferred type, and a method overridden in a constant body keeps the
declared one.
6. One refinement: under @TypeChecked, a static method called through its
class name (A.name()), or from a static context, does get the inferred type,
because there the call can't reach a subclass. Only instance-receiver calls
such as this.name(), or calls from a closure, keep the declared type.
testTypeCheckedStaticMethodViaClass and
testTypeCheckedPrivateOrStaticMethodViaInstance cover the two sides.
> STC: propagate inferred return types of non-overridable methods, in the same
> unit and via @TypeCheckingInfo
> -----------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12472
> URL: https://issues.apache.org/jira/browse/GROOVY-12472
> Project: Groovy
> Issue Type: Improvement
> Components: Static Type Checker
> Reporter: Paul King
> Priority: Major
> Fix For: 7.x
>
>
> h2. Context
> {{@TypeChecked}} infers a return type for every method body it checks, but
> since GROOVY-6401 (2013) that type never leaves the body.
> {{getType(MethodNode)}} still prefers a stored {{INFERRED_RETURN_TYPE}} over
> the declared type, but nothing stores one on a {{MethodNode}} any more,
> {{addTypeCheckingInfoAnnotation}} has no caller, {{silentlyVisitMethodNode}}
> has no caller, and the reader ({{pickInferredTypeFromMethodAnnotation}}) and
> its test ({{PrecompiledClassSTCTest}}) were deleted in d1131470e0. So a call
> to a {{def}} method is typed as {{Object}} at the call site, even within one
> class:
> {code:groovy}
> @CompileStatic class A {
> def name() { 'x' }
> String use() { name().toUpperCase() }
> // [Static type checking] - Cannot find matching method
> java.lang.Object#toUpperCase()
> }
> {code}
> GROOVY-6401 removed the propagation because it was unsound for overridable
> methods. The inferred type is a fact about one body; a subclass may override
> the method and return a different subtype of the declared type:
> {code:groovy}
> @CompileStatic class A {
> Greeter createGreeter() { new HelloGreeter() } // inferred
> HelloGreeter
> void sayHello() { Greeter g = createGreeter(); g.sayHello() }
> }
> @CompileStatic class B extends A {
> Greeter createGreeter() { new HelloThereGreeter() }
> }
> new B().sayHello() // GroovyCastException: cannot cast HelloThereGreeter to
> HelloGreeter
> {code}
> Under {{@TypeChecked}} this produced wrong type errors; under
> {{@CompileStatic}} it also emitted a {{checkcast}} to the inferred type,
> which failed at run time.
> GROOVY-12446 made the version-1 payload free of checker-only union nodes.
> GROOVY-12445 fixed the unknown-version rule: a {{null}} codec means "no
> inferred signature, use the declared type, do not read the bytes". Both
> prepare the ground for a reader that does not exist yet. This issue adds the
> missing piece.
> h2. Proposal
> Propagate a method's inferred return type to its call sites only when the
> method cannot be overridden. For those methods the inferred type is a fact
> about the only body that can run, so the GROOVY-6401 counter-example cannot
> arise.
> A method is *non-overridable* when any of these holds:
> * it is {{private}}
> * it is {{static}}
> * it is {{final}}
> * its declaring class is {{final}} (including records)
> Everything else keeps the declared return type. In particular: non-final
> instance methods, interface default methods, non-final enum methods (an enum
> constant body may override them), and trait methods (the helper rewrite
> changes their shape) are *out*.
> The rule applies identically to the three places the type can flow:
> # *Same compilation unit.* When a call resolves
> ({{DIRECT_METHOD_CALL_TARGET}}) to a non-overridable method in the current
> source unit that has not been visited yet, visit it first
> ({{silentlyVisitMethodNode}}), store its inferred return type on the
> {{MethodNode}}, and use it at the call site.
> # *Writer.* At the end of {{startMethodInference}} for a non-overridable
> method, call {{addTypeCheckingInfoAnnotation}} so the inferred type is
> recorded on the class file as {{@TypeChecked.TypeCheckingInfo(version = 1,
> inferredType = ...)}}. Methods that do not satisfy the rule get no annotation.
> # *Reader.* When a call resolves to a precompiled non-overridable method
> carrying {{@TypeCheckingInfo}}, decode it with
> {{SignatureCodecFactory.inferredReturnOrDeclared(version, signature, loader,
> declared)}} and store the result as the method's inferred return type. The
> loader must be the compile classpath loader (as the deleted reader used
> {{getSourceUnit().getClassLoader()}}), not the transform loader, so the
> encoded class names resolve.
> h2. Contract
> ||Method||Same unit||Written to bytecode||Read from bytecode||
> |private / static / final / in final class|inferred
> type|{{@TypeCheckingInfo}} added|decoded type|
> |any other|declared type|no annotation|declared type, payload unread|
> |constructor|n/a|never (existing rule)|n/a|
> A reader never fails compilation on the payload. An unknown version falls
> back to the declared type (GROOVY-12445). A corrupt payload of a known
> version is treated the same way: catch the {{GroovyRuntimeException}} from
> {{decode}}, use the declared type, and continue.
> The inferred type written and read is the one {{SignatureCodecVersion1}}
> produces after GROOVY-12446: generics preserved, unions widened to a stable
> lowest upper bound or {{Object}}. The type checker treats it exactly as it
> treats a declared return type at that call site; no extra {{checkcast}}
> beyond what the declared/inferred type already implies.
> h2. What this gives
> * Callers of {{def}}-style methods in a statically compiled library get the
> types the library's own compiler proved, with no API change by the library
> author. Multi-module builds and frameworks consuming Groovy jars benefit most.
> * Fewer casts and redundant declared types in consumer code; type errors name
> the real type instead of {{Object}}.
> * {{@CompileStatic}} can emit a direct call on the real type rather than
> going through {{Object}} and casting.
> * groovy-lsp hover/completion on such calls shows the inferred type rather
> than {{Object}} (it already runs STC).
> h2. Touch points
> * {{StaticTypeCheckingVisitor}}: a {{isNonOverridable(MethodNode)}}
> predicate; the call-site hook that triggers {{silentlyVisitMethodNode}} / the
> reader for a resolved target; the end of {{startMethodInference}} for the
> writer. Verify which call-site path computes the return type for a given
> call: {{getType(MethodNode)}} already prefers the stored inferred type, but
> {{inferReturnTypeGenerics}} starts from {{method.getReturnType()}} and may
> need to start from the inferred type when one is stored.
> * {{SignatureCodecFactory.inferredReturnOrDeclared}} (from GROOVY-12445):
> gains the corrupt-payload fallback, or a wrapper does.
> * {{groovy.transform.TypeChecked.TypeCheckingInfo}} Javadoc: currently says
> the annotation "is added by @TypeChecked on methods which have type checking
> turned on"; make it say *non-overridable* methods and name the version rule.
> * {{StaticCompilationVisitor}}: confirm the stored inferred type is what the
> static compiler uses for the call's return type and {{checkcast}}.
> * Documentation: the "Type checking" section of the language guide gets a
> paragraph on inferred return types and the non-overridable rule.
> * {{COMPATIBILITY.md}}: note that {{@TypeCheckingInfo}} is written again (it
> was last written by Groovy 2.1.x) and only for non-overridable methods.
> h2. Tests
> * Same unit: {{def}} private/static/final methods and methods in a final
> class resolve to their inferred type at call sites, in both {{@TypeChecked}}
> and {{@CompileStatic}}; a non-final instance method still resolves to the
> declared type. The GROOVY-6401 example compiles and runs correctly.
> * Writer: a precompiled class carries {{@TypeCheckingInfo}} exactly on its
> non-overridable methods, none on constructors or overridable methods
> ({{ConstructorsSTCTest.testTypeCheckingInfoShouldNotBeAddedToConstructor}}
> stays green).
> * Reader: reinstate the shape of the deleted {{PrecompiledClassSTCTest}}: a
> precompiled {{static def foo() { new Date() }}} lets {{long t =
> foo().getTime()}} compile; a non-annotated {{bar()}} still fails with
> {{Cannot find matching method java.lang.Object#getTime()}}.
> * Reader robustness: an unknown version and a corrupt version-1 payload both
> fall back to the declared type without an error.
> * Generics survive the round trip ({{List<String>}}); a union return becomes
> its lowest upper bound (GROOVY-12446).
> * Static compilation emits no {{checkcast}} to a type the declared/inferred
> contract does not justify (bytecode check on the GROOVY-6401 example).
> h2. Staged delivery
> Each stage is independently useful and can land separately:
> # Same-unit propagation under the non-overridable rule.
> # Writer, under the same rule.
> # Reader, with the fallbacks.
> h2. Scope
> Overridable methods stay on the declared type; a scheme that records a
> covariant contract for overriders is a separate discussion. Traits and
> interface default methods are excluded. Closure return inference is unchanged.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
