Skip to content

NoOpImplementation improvements [3/3] RUM-7390 Add NoOp name collision detection and customName support#3470

Open
satween wants to merge 2 commits into
developfrom
tvaleev/feature/add-noop-name-collision-detection
Open

NoOpImplementation improvements [3/3] RUM-7390 Add NoOp name collision detection and customName support#3470
satween wants to merge 2 commits into
developfrom
tvaleev/feature/add-noop-name-collision-detection

Conversation

@satween
Copy link
Copy Markdown
Contributor

@satween satween commented May 25, 2026

What does this PR do?

Adds collision detection to the NoOp KSP processor and a new customName parameter to @NoOpImplementation. When two interfaces would generate the same NoOp* class name (e.g. Outer.Inner and OuterInner both produce NoOpOuterInner), the processor now emits a clear error and suggests using customName to resolve the conflict. The customName parameter lets callers override the generated class name entirely.

Motivation

Without collision detection, two distinct interfaces silently clobber each other's generated file, producing broken or incomplete NoOp implementations at build time with no diagnostic. The new guard catches this at KSP processing time with actionable error messages.

Additional Notes

  • Validates customName against a Kotlin identifier regex and reports a clear error for names like "My.Invalid.Name".
  • Detects both same-round name collisions (via an in-process generatedNames map) and pre-existing file conflicts (via FileAlreadyExistsException).
  • ThrowingCodeGeneratorProvider test helper retained because KSP 1.8.0 does not throw FileAlreadyExistsException for disk files from previous builds; two-pass compilation silently overwrites.
  • All 8 new test cases pass; no existing tests regressed.
  • generateNoOpImplementation refactored into resolveNoOpClassName, reportNameCollision, and writeNoOpFile helpers to satisfy detekt LongMethod / ReturnCount limits.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

Ref: RUM-7390

Copy link
Copy Markdown
Contributor Author

satween commented May 25, 2026

@datadog-official

This comment has been minimized.

@satween satween changed the title RUM-7390 Add NoOp name collision detection and customName support NoOpImplementation improvements [3/3] RUM-7390 Add NoOp name collision detection and customName support May 25, 2026
@satween satween marked this pull request as ready for review May 25, 2026 15:47
@satween satween requested review from a team as code owners May 25, 2026 15:47
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2943d3a56

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Base automatically changed from tvaleev/feature/guard-noop-generation-against-restricted to develop May 26, 2026 09:56
@satween satween marked this pull request as draft May 26, 2026 10:03
@satween satween force-pushed the tvaleev/feature/add-noop-name-collision-detection branch from b2943d3 to 86ce9a9 Compare May 26, 2026 12:55
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.18%. Comparing base (00897b2) to head (62ae506).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3470      +/-   ##
===========================================
- Coverage    72.20%   72.18%   -0.02%     
===========================================
  Files          964      964              
  Lines        35554    35554              
  Branches      5922     5922              
===========================================
- Hits         25669    25663       -6     
- Misses        8269     8282      +13     
+ Partials      1616     1609       -7     

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Detect when two interfaces would generate the same NoOp class name and
  report a KSP error instead of silently overwriting the file.
- Add customName parameter to @NoOpImplementation to let users resolve
  collisions by overriding the generated class name.
- Support NoOp generation for nested interfaces; guard against interfaces
  with restricted (private/protected) enclosing visibility.
- Fix NoSuchElementException caused by KSP2 omitting default annotation
  args from KSAnnotation.arguments (workaround via safeAnnotationArg;
  see google/ksp#2356).
- Fix return-type and property initializer paths to use the effective
  NoOp name (respecting customName) when referencing other annotated
  interfaces, preventing compilation failures.
- Extract annotation utilities into NoOpAnnotationExt.kt.

Ref: RUM-7390
@satween satween force-pushed the tvaleev/feature/add-noop-name-collision-detection branch from b06a347 to d98a5c1 Compare May 26, 2026 14:42
@satween satween marked this pull request as ready for review May 26, 2026 14:43
Remove needless double blank line caught by ktlint.

Ref: RUM-7390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants