[
https://issues.apache.org/jira/browse/GROOVY-12472?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18129465#comment-18129465
]
ASF GitHub Bot commented on GROOVY-12472:
-----------------------------------------
codecov-commenter commented on PR #2998:
URL: https://github.com/apache/groovy/pull/2998#issuecomment-6310122901
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2998?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `85.98726%` with `22 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 72.4559%. Comparing base
([`8778bce`](https://app.codecov.io/gh/apache/groovy/commit/8778bce1a0da05b5bffe27f02819262c849f9d56?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`e5b717a`](https://app.codecov.io/gh/apache/groovy/commit/e5b717ab4a691417191c1ab89ba05069fbbdc064?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2998?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...roovy/transform/stc/StaticTypeCheckingVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2998?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fstc%2FStaticTypeCheckingVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zdGMvU3RhdGljVHlwZUNoZWNraW5nVmlzaXRvci5qYXZh)
| 80.0000% | [6 Missing and 16 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2998?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2998?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2998 +/- ##
==================================================
+ Coverage 72.4290% 72.4559% +0.0269%
- Complexity 40622 40693 +71
==================================================
Files 1643 1643
Lines 142686 142840 +154
Branches 26799 26838 +39
==================================================
+ Hits 103346 103496 +150
+ Misses 29830 29815 -15
- Partials 9510 9529 +19
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2998?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[.../groovy/transform/sc/StaticCompilationVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2998?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fsc%2FStaticCompilationVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zYy9TdGF0aWNDb21waWxhdGlvblZpc2l0b3IuamF2YQ==)
| `84.3621% <100.0000%> (+0.0646%)` | :arrow_up: |
|
[...haus/groovy/transform/stc/TypeCheckingContext.java](https://app.codecov.io/gh/apache/groovy/pull/2998?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fstc%2FTypeCheckingContext.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zdGMvVHlwZUNoZWNraW5nQ29udGV4dC5qYXZh)
| `90.8497% <100.0000%> (+3.9338%)` | :arrow_up: |
|
[...roovy/transform/stc/StaticTypeCheckingVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2998?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2Fstc%2FStaticTypeCheckingVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9zdGMvU3RhdGljVHlwZUNoZWNraW5nVmlzaXRvci5qYXZh)
| `87.3974% <80.0000%> (+0.1982%)` | :arrow_up: |
... and [11 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2998/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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
(v8.20.10#820010)
