Add @NoEscape marker annotation and apply it to SubSequence and Maybe - #12368
Add @NoEscape marker annotation and apply it to SubSequence and Maybe#12368dougqh wants to merge 6 commits into
Conversation
Documentation-and-tooling marker for transient, zero-copy view types (e.g. a substring view sharing its parent String's backing array) meant to live only within a single call frame -- holding one beyond that frame pins the shared backing object alive. No application to a real type yet and no checker -- just the annotation type, following the Strategy/StrategyConsumer marker convention (APMLP-1787).
Broadens the motivating-shapes list to cover escape-analysis-dependent value types (constructed, consumed, and discarded within one call frame by design) alongside shared-backing views, ahead of applying the annotation to Maybe in addition to SubSequence.
SubSequence shares its parent String's backing array, so holding one past its call frame pins the parent alive. Maybe is designed to scalar-replace under escape analysis and becomes a real, permanent allocation if stored anywhere longer-lived than the call that produced it. Both are exactly the shape the annotation exists to flag; no checker yet (APMLP-1787).
dougqh
added
comp: core
"Live only within a single call frame" wrongly implied a NoEscape value couldn't be returned or passed to a callback -- both Maybe (returned, chained through update()/getOrNull()) and SubSequence (passed to callbacks, returned from subSequence()) do exactly that. The actual constraint is storage: never a field, collection, or cache, however far the value otherwise travels through ordinary calls. Also adds a "Checker contract" section to the annotation's javadoc (trigger / not-a-trigger / violation / compliant examples) so an AI reviewer can check compliance without reading the prose.
Adds a deterministic-lint entry pointing at NoEscape's new self-contained checker contract, and cross-references it from J7 (SubSequence's existing retention-trap entry) so the perf-review skill can flag violations from the diff alone.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ded4603e54
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * </ul> | ||
| */ | ||
| @Documented | ||
| @Retention(RetentionPolicy.SOURCE) |
There was a problem hiding this comment.
When the promised checker analyzes a Gradle module that consumes :internal-api, it sees SubSequence and Maybe from the compiled artifact rather than their source files. SOURCE retention removes @NoEscape from those class files, so the checker cannot determine that a dependent module's field uses an annotated type and such violations silently evade enforcement. Use CLASS retention, which remains unavailable to runtime reflection, or require the checker to index all source trees globally.
Useful? React with 👍 / 👎.
| * backing array, holding one anywhere longer-lived than the call that produced it (a field, a | ||
| * cache, a collection) pins the entire parent string alive for as long as the view survives. | ||
| */ | ||
| @NoEscape |
There was a problem hiding this comment.
Applying @NoEscape here immediately contradicts its checker contract: the next line declares the static field SubSequence EMPTY, while the contract explicitly triggers on any instance or static field whose declared type is annotated. A checker implementing the documented rule will therefore report this class itself even though the singleton only retains an empty string; remove the field or define and implement a narrow safe-singleton exception so the rule does not begin with a known false positive.
Useful? React with 👍 / 👎.
| - a field (instance or static, directly or as a generic type argument) declared with an | ||
| `@NoEscape`-annotated type (`datadog.trace.api.function.NoEscape`) — the annotation's own | ||
| javadoc carries a self-contained "Checker contract" section (trigger / not-a-trigger / | ||
| violation example / compliant example) written so this can be checked from the diff alone, |
There was a problem hiding this comment.
Until the mechanical checker lands, placing this entry under Deterministic-lint candidates (DON'T spend AI budget) conflicts with the entry's claim that an AI reviewer should check it and with the annotation's statement that it is not yet enforced. When the required /perf-review runs during this interim period, the reviewer can follow the section instruction and skip these field violations entirely; keep this in the active review checks until the lint exists, or explicitly require the AI to enforce it meanwhile.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
Retention was framed as an absolute prohibition, but a deliberate, reviewed exception (e.g. a container retaining a SubSequence as a precaution) is legitimate as long as it's called out at the retention site rather than done silently. The checker contract's trigger now requires an explanatory comment to be compliant, rather than treating every match as an automatic violation.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new rule marks SubSequence.EMPTY as a violation because it stores an annotated type without a comment. Add a justification at the field or define an exemption.
🤖 Datadog Autotest · Commit f20b296 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| * backing array, holding one anywhere longer-lived than the call that produced it (a field, a | ||
| * cache, a collection) pins the entire parent string alive for as long as the view survives. | ||
| */ | ||
| @NoEscape |
There was a problem hiding this comment.
The rule creates a false finding for its first annotated type and reduces review accuracy.
Assertion details
- Input: Run the new
@NoEscapefield-storage check againstSubSequence.EMPTY. - Expected: Add a justification comment to
EMPTY, or define an explicit exemption for safe singleton fields. - Actual: The new review rule flags the safe
SubSequence.EMPTYstatic field because it stores an annotated type without a justification comment.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
1 participant
What Does This Do
Adds
datadog.trace.api.function.NoEscape, a documentation-and-tooling marker (no runtime behavior,SOURCEretention) for types that must never be retained -- never assigned to a field, put into a collection, or cached -- though they may otherwise flow normally through returns, callbacks, and chained calls. Follows the existing@Strategy/@StrategyConsumermarker convention.Applies it to two existing types that already match the shape:
SubSequence-- shares its parentString's backing array, so storing one pins the parent alive for as long as it's retained.Maybe-- deliberately shaped to scalar-replace under escape analysis; assigning an instance to a field or collection forces the JIT to materialize a real, permanent allocation.Also adds a "Checker contract" section to the annotation's own javadoc (trigger / not-a-trigger / violation example / compliant example / v1 scope) and wires it into the perf-review skill's
checks.mdas a deterministic-lint entry, so an AI reviewer can flag violations directly from a diff without needing a mechanical checker yet.Motivation
This is the first step of a 3-phase plan for a set of new perf-review annotations: define the annotation → apply it to its motivating use cases → build enforcement. No checker lands in this PR (tracked separately); the goal here is establishing the marker and its first real wearers, with a rule specific enough that either a human or an AI reviewer can already check compliance by hand.
Additional Notes
@NoEscapeisSOURCE-retention and carries no runtime semantics.@NoEscapevalue can be returned, passed to a callback, or chained through further calls; the only violation is parking it in a field/collection/cache.Contributor Checklist
type:andcomp:labels in addition to any other useful labelsclose/fix/linking keywords when referencing an issueJira ticket: APMLP-1787
🤖 Generated with Claude Code