Skip to content

Conversation

@ghokun
Copy link
Member

@ghokun ghokun commented Dec 12, 2025

Context

Enum.valueOf method usage may lead to runtime exceptions when the provided argument is not checked against all possible values of method owner.

Example:

class Test {
  enum A {
    ONE, TWO, THREE
  }
  
  enum B {
    ONE, TWO, THREE, FOUR
  }
  
  // Will throw a runtime exception if b is equal to FOUR.
  A convertUnsafe(B b) {
    return A.valueOf(b.name());
  }
}

Scope

Of course it is not able to capture all possible checks on b before supplying it into A.valueOf. Therefore scope will be limited to following checks.

// BUG: Raw string
A convertUnsafe(String raw) {
  return A.valueOf(raw);
}

// BUG: From invalid string constant
A convertUnsafe() {
  return A.valueOf("FOUR");
}

// BUG: Where B has values that A do not have
A convertUnsafe(B b) {
  return A.valueOf(b.name());
}

// BUG: Value from case is not present in A
A convertUnsafe(B b) {
  return switch(b) { case FOUR -> A.valueOf(b.name()); };
}

// NOT BUG: From valid string constant
A convertSafe() {
  return A.valueOf("ONE");
}

Above checks should also consider the polymorphic variant of valueOf:

public static <T extends Enum<T>> T valueOf(Class<T> enumClass, String name)

@github-actions
Copy link

github-actions bot commented Dec 12, 2025

Suggested commit message:

Add `EnumValueOfUsage` bug pattern for unchecked `valueOf` usage (#2031)

Introduce an Error Prone check that flags `Enum#valueOf` invocations
with unchecked names, including raw strings and values from enums that
define additional constants.

Handle both overloads and account for switch filters to avoid
unchecked values reaching `valueOf`.

@github-actions
Copy link

  • Surviving mutants in this change: 3
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfSuperSet 3 7

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie self-requested a review December 17, 2025 11:53
@rickie rickie added this to the 0.28.0 milestone Dec 17, 2025
@github-actions
Copy link

  • Surviving mutants in this change: 2
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfSuperSet 2 7

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@github-actions
Copy link

  • Surviving mutants in this change: 2
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfSuperSet 2 7

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@ghokun ghokun force-pushed the ghokun/enum-value-of-superset branch from c1ce1f0 to 918a7b1 Compare December 22, 2025 13:23
@github-actions
Copy link

  • Surviving mutants in this change: 12
  • Killed mutants in this change: 16
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfUsage 12 16

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

"",
" void convertSafe(String raw, B b) {",
" A.valueOf(\"ONE\");",
" var a = switch(b) { case TWO -> A.valueOf(b.name()); default -> null;};",
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to traverse back to switch case statement somehow to capture possible values of b.name(). If they are filtered by the switch case this should not be a bug.

Question: Should we do a similar check for possible if-else statements? I think scope will be too large to capture everything.

}

// Match unchecked String values
if (nameArgument instanceof IdentifierTree) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is enough to check if it is just a passed String.

@ghokun ghokun marked this pull request as ready for review December 22, 2025 15:45
@github-actions
Copy link

  • Surviving mutants in this change: 6
  • Killed mutants in this change: 24
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfUsage 6 24

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

.filter(
argument ->
Optional.ofNullable(ASTHelpers.getType(argument))
.filter(type -> type.toString().equals(String.class.getCanonicalName()))

Choose a reason for hiding this comment

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

[javac] reported by reviewdog 🐶
[TypeToString] TypeMirror#toString shouldn't be used as it is expensive and fragile.

@github-actions
Copy link

  • Surviving mutants in this change: 12
  • Killed mutants in this change: 33
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfUsage 12 33

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@sonarqubecloud
Copy link

@github-actions
Copy link

  • Surviving mutants in this change: 12
  • Killed mutants in this change: 33
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.EnumValueOfUsage 12 33

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@ghokun
Copy link
Member Author

ghokun commented Dec 23, 2025

I will try to resolve mutation test reports when I have the time.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants