Skip to content

Conversation

@vlsi
Copy link
Contributor

@vlsi vlsi commented Jun 5, 2025

Overview

The latter is easier to read, and it forbids list.add(...) at the compile-time.

See https://jspecify.dev/docs/user-guide/#wildcard-bounds


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@vlsi
Copy link
Contributor Author

vlsi commented Jun 5, 2025

It looks like AssertJ can't assert on values like List<?> and List<? extends Object>:

List<?> values = List.of("1", 2)

assertThat(values).containsExactly("1", 2); // this does not compile
Assertions.<Object>assertThat(values).containsExactly("1", 2); // this compiles

Currently, the build fails because some of the JUnit tests attempt things like the following:

var values = readFieldValues(fields, new ClassWithFields());
assertThat(values).containsExactly("enigma", 3.14, "text", 2.5, null, 42, "constant", 99);

I'm not sure if AssertJ's assertThat is the reason to refrain from List<?>.

@vlsi
Copy link
Contributor Author

vlsi commented Jun 5, 2025

A funny case is that an illegal .set was there in the test code:

// Modify local copy:
assertThrows(UnsupportedOperationException.class, () -> copy.set(0, "Boom!"));

The code fails to compile with List<?>, so it is a good case: it moves a runtime failure to a compile-time failure.

Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

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

Since this is a breaking change, I'm not convinced we should do it for public APIs. Could you please revert all changes except those in ReflectionUtils and its test class?

@vlsi vlsi force-pushed the null_list branch 2 times, most recently from 97bd188 to d717c72 Compare June 6, 2025 14:32
Comment on lines 453 to 455
@SuppressWarnings("unchecked")
List<Object> result = (List<Object>) ReflectionUtils.readFieldValues(fields, instance);
return result;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not fond of this cast, however, it seems to be the least evil as the added suppression holds for a single statement only rather than a method.

The latter is easier to read, and it forbids list.add(...) at the compile-time.

See https://jspecify.dev/docs/user-guide/#wildcard-bounds
@marcphilipp marcphilipp added this to the 6.0.0-M1 milestone Jun 7, 2025
@marcphilipp marcphilipp merged commit 2089504 into junit-team:main Jun 7, 2025
12 checks passed
@marcphilipp
Copy link
Member

Thanks, @vlsi! 👍

@cpovirk cpovirk mentioned this pull request Jun 9, 2025
@sbrannen sbrannen changed the title chore: replace List<@Nullable Object> with List<?> Replace List<@Nullable Object> with List<?> Jun 11, 2025
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.

2 participants