[
https://issues.apache.org/jira/browse/GROOVY-12472?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133476#comment-18133476
]
ASF GitHub Bot commented on GROOVY-12472:
-----------------------------------------
paulk-asert commented on PR #2998:
URL: https://github.com/apache/groovy/pull/2998#issuecomment-6464333806
Thanks Daniel. Changed to cover the items you asked for:
1. **Mutual recursion:** added `testMutuallyRecursiveCallsUseDeclaredType`.
It's two private `def` methods calling each other, with a String base case,
under `@CompileStatic`. It runs in both declaration orders, so either method
can be the one checked first. In both orders, the call back into the method
being checked gets the declared `Object`, so nothing is published for either
method, and `.toUpperCase()` still fails.
2. **Closure-shared return:** added
`testClosureSharedVariableUsesDeclaredType`. A method returns a local variable
after a closure has assigned it `1`, and the call keeps the declared type.
3. **Package-private / protected under SC:** yes, that's intentional. Both
can be overridden, so even under static compilation there's no sole
implementation. I've added a sentence to the `hasSoleImplementation` Javadoc
saying so.
4. **Named LUBs:** agreed, this is deliberately conservative for a first
cut. For example, a `Number` local that holds an `Integer` and later a
`BigDecimal` gets `Object` rather than `Number`. If it's ever relaxed, a
positive test with a shared supertype is where I'd start.
5. **Other classes in the same compilation unit:** agreed. It's a known
limitation and `testMethodOfAnotherClassUsesDeclaredType` documents it. Happy
to track it as follow-up work.
6. **Nits:**
- A `@CompileStatic` plain enum behaves the same as the `@TypeChecked`
case: `A.m().toUpperCase()` compiles and runs.
- You're right about my wording. A closure in an instance method keeps
the declared type, but a closure in a static method still sees the inferred
type, because `this` there is the class.
7. **GROOVY-12445:** it has landed and is already in the branch's base. The
conflict I expected in `addTypeCheckingInfoAnnotation` didn't happen. The
reader keeps its own decoder rather than using `inferredReturnOrDeclared`.
Beyond the version check, it also has to handle an unreadable payload, reject
unions, mixed-type bounds and type parameters, and check that every named class
is on the classpath.
I also made one clarification to COMPATIBILITY.md: the annotation is written
only on methods that are type checked (`@TypeChecked` or `@CompileStatic`). A
dynamically compiled method records nothing, so its callers keep its declared
return type.
On the binary-contract point: I checked a statically compiled caller built
against a library whose `static def name()` returned `'x'`, then recompiled
only the library to return `42`. The caller has a `checkcast String` after the
call, so it fails with `ClassCastException` at the use site rather than a
`VerifyError`.
> 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)
