Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Safe-logging] Verify member reference type parameters when cast to functional interface #3052

Merged
merged 6 commits into from
Feb 26, 2025

Conversation

aldexis
Copy link
Contributor

@aldexis aldexis commented Feb 24, 2025

Before this PR

Passing a member reference as a lambda will currently not catch cases where the type it gets passed to has different requirements around safety of the return or parameter types.

After this PR

==COMMIT_MSG==
[Safe-logging] Verify member reference type parameters when cast to functional interface
==COMMIT_MSG==

This is done by comparing the safety between the method reference and the functional interface it gets cast into, and verifying that:

  • The interface's return type safety is less safe than the method reference
    • This means you can do e.g. Supplier<@Unsafe String> f = this::fun when fun returns @Safe, but not @Supplier<@Safe String> f = this::fun when fun returns @Unsafe, as in the latter case, you would then start using the unsafe result from fun as safe when calling f
  • The interface's argument's type safety is more safe than the method reference
    • This means you can do e.g. Consumer<@Safe String> f = this::fun when fun takes an @Unsafe, but not Consumer<@Unsafe String> f = this::fun when fun takes a @Safe, as otherwise calling f.accept(s) would let you pass an unsafe string to fun

To summarize:
Allowed

import com.palantir.logsafe.*;
import java.util.function.*;

class Test {
  Supplier<@Unsafe String> func = this::fun;

  @Safe
  String fun() {
    return "safe";
  }
}
import com.palantir.logsafe.*;
import java.util.function.*;

class Test {
  Consumer<@Safe String> func = this::fun;

  void fun(@Unsafe Object ob) {}
}

Forbidden

import com.palantir.logsafe.*;
import java.util.function.*;

class Test {
  Supplier<@Safe String> func = this::fun;

  @Unsafe
  String fun() {
    return "unsafe";
  }
}
import com.palantir.logsafe.*;
import java.util.function.*;

class Test {
  Consumer<@Unsafe String> func = this::fun;

  void fun(@Safe Object ob) {}
}

Possible downsides?

@aldexis aldexis force-pushed the ald/member-reference-check branch from 51556ee to 5c9dc78 Compare February 25, 2025 12:11
@aldexis aldexis changed the base branch from ald/safe-logging-lambda-argument to develop February 25, 2025 12:11
@changelog-app
Copy link

changelog-app bot commented Feb 25, 2025

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

[Safe-logging] Verify member reference type parameters when cast to functional interface

Check the box to generate changelog(s)

  • Generate changelog entry

@aldexis aldexis marked this pull request as ready for review February 25, 2025 12:43
@aldexis aldexis requested a review from carterkozak February 25, 2025 12:43
// If e.g. the cast type says it will return SAFE, but the reference returns UNSAFE, that's a problem
// On the other hand, if the cast returns UNSAFE and the reference returns SAFE, that's fine
if (!castTypeReturnSafety.allowsValueWith(referenceReturnSafety)) {
return buildDescription(tree)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use state.reportMatch(, we can report problems with the return type as well as parameters in the same evaluation. It's not really the standard way we like to use error-prone, but I do find it helpful to describe all the issues at once for the safelogging check specifically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea indeed!

@Override
public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) {
// This is the type the reference gets "cast" into, whether through assignment, return type, argument, etc
Type castType = state.getTypes().findDescriptorType(ASTHelpers.getType(tree));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if ASTHelpers.getType(tree) returns null or findDescriptorType returns null and exit early?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Fantastic work, I especially appreciate the inline comments!

@carterkozak
Copy link
Contributor

Feel free to add "merge-when-ready" if you're ready to release this

@bulldozer-bot bulldozer-bot bot merged commit 4066118 into develop Feb 26, 2025
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the ald/member-reference-check branch February 26, 2025 14:13
@autorelease3
Copy link

autorelease3 bot commented Feb 26, 2025

Released 6.16.0

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

Successfully merging this pull request may close these issues.

3 participants