Skip to content

[jnigen] Propagate Javadoc deprecation messages#3475

Open
Siya-05 wants to merge 4 commits into
dart-lang:mainfrom
Siya-05:jnigen-javadoc-deprecation
Open

[jnigen] Propagate Javadoc deprecation messages#3475
Siya-05 wants to merge 4 commits into
dart-lang:mainfrom
Siya-05:jnigen-javadoc-deprecation

Conversation

@Siya-05

@Siya-05 Siya-05 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Description

This PR is a follow-up to #3429 and addresses part of #2796.

The previous PR generated Dart @Deprecated annotations for Java methods annotated with java.lang.Deprecated, using a generic deprecation message.

This PR propagates the Java @deprecated Javadoc message into the generated Dart @Deprecated(...) annotation when bindings are generated with the doclet backend, providing developers with the original deprecation guidance from the Java source.

When Javadocs are unavailable (for example, when using the ASM backend), the generator continues to fall back to the generic "This Java method is deprecated." message.

Changes

  • Add a JavaDocComment.deprecatedMessage helper to extract the @deprecated Javadoc message.
  • Generate Dart @Deprecated(...) annotations using the extracted Javadoc message when available.
  • Preserve the existing generic fallback message when Javadocs are unavailable.
  • Update the simple_package test fixture with a Javadoc deprecation message.

Related Issues

Addresses part of #2796.

PR Checklist

  • I’ve reviewed the [contributor guide](https://github.com/dart-lang/native/blob/main/CONTRIBUTING.md) and applied the relevant portions to this PR.
  • I've run dart tool/ci.dart --all locally and resolved all issues identified. This ensures the PR is formatted, has no lint errors, and ran all code generators. This applies to the packages part of the toplevel pubspec.yaml workspace.
  • All existing tests are passing.
  • The PR addresses the requested follow-up by propagating the Java @deprecated Javadoc message into generated Dart @Deprecated(...) annotations.
  • I have updated CHANGELOG.md for the relevant packages. (Not needed for small changes such as doc typos.)
  • I have updated the pubspec package version if necessary.

@liamappelbe
liamappelbe self-requested a review July 21, 2026 01:01
@github-actions

github-actions Bot commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?

This check can be disabled by tagging the PR with skip-breaking-check.

liamappelbe
liamappelbe reviewed Jul 21, 2026
View reviewed changes

@liamappelbe liamappelbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the slow review. I was pretty swamped last week. Since this is hard to cover with integration tests, and deprecatedMessage is a pretty fiddly little function, it's a good candidate for a unit test. You can just add a new test file directly in pkgs/jnigen/test. Construct a bunch of synthetic JavaDocComment objects, and make sure the deprecatedMessage returns the right answer across all edge cases.

@liamappelbe

liamappelbe commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

To fix this error, you'll either need to:

  • output single quotes (maybe move the jsonEncode step inside deprecatedMessage and fix the quotes in there, so you can unit test it?)
  • or add an ignore for this analysis error to the list of all ignores printed at the top of each jnigen file (this is more work than it seems because you then have to regen all the bindings in this monorepo)

@liamappelbe

liamappelbe commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Can you also add a CHANGELOG entry? I forgot to ask for this in the previous PR.

@Siya-05

Siya-05 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! I've added the unit tests, moved the jsonEncode logic into deprecatedMessage, and added a CHANGELOG entry.

liamappelbe
liamappelbe reviewed Jul 23, 2026
View reviewed changes

@liamappelbe liamappelbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should have

Comment thread pkgs/jnigen/lib/src/elements/elements.dart Outdated
final message = messageLines.join('\n').trim();
if (message.isEmpty) return null;

final encoded = jsonEncode(message);

@liamappelbe liamappelbe Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably roll our own Dart string escaper, rather than relying on this JSON encoding trick. It's sort of a coincidence that JSON uses mostly the same escape characters as Dart. In fact there are some small differences in the escaping rules that are leading to test failures like this. It'd be better to write (and unit test) a dedicated function for this.

You may also need to regenerate the bindings for that test, to fix this failure. Try tool/regenerate_all_bindings.dart. That tool can be a bit problematic to run, so let me know if you have issues with it (eg errors, or spurious diffs) and I can regerenate the bindings for you on my machine.

@Siya-05

Siya-05 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! I'll implement a dedicated Dart string escaper and add unit tests.

Just to make sure I match your expectations: do you have a preferred set of escape rules in mind (or an existing helper in the repo I should follow), or should I implement a minimal escaper that emits valid single-quoted Dart string literals and expand it if tests require additional cases?

@liamappelbe

liamappelbe commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Just to make sure I match your expectations: do you have a preferred set of escape rules in mind (or an existing helper in the repo I should follow), or should I implement a minimal escaper that emits valid single-quoted Dart string literals and expand it if tests require additional cases?

I think it's just \b, \n, \f, \v, \r, \t, ', ", \, $.

@Siya-05

Siya-05 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the help! I moved the escaping logic into dart_generator.dart with a dedicated escapeDartString helper and added unit tests covering the escaping behavior. I regenerated the existing Jackson test bindings instead of running regenerate_all_bindings.dart to keep the diff focused, since the full regeneration produced many unrelated changes.

@liamappelbe

liamappelbe commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@Siya-05 I don't see any new changes. Did you forget to push?

@Siya-05

Siya-05 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Sorry about that! I missed the push error on my end yesterday. I've pushed the latest changes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamappelbe liamappelbe liamappelbe approved these changes

Assignees

No one assigned

Labels

package:jnigen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Siya-05 @liamappelbe