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

More immutable classes. #1727

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

victorwss
Copy link

@victorwss victorwss commented Sep 27, 2021

I added some classes to the known immutable set.

  • java.math.Decimal does not exists (should be BigDecimal). And the package java.math already covers it, so I just removed it along with BigInteger.
  • Although the reflection objects can be altered with the setAccessible(boolean), there is already other checks covering that corner-case, so for every other case, they are immutable. Also, the side-effect of using that method is likely the intended one, and not the accidental side-effect that immutability seeks to prevent.
  • I am not sure if the java.lang.Module and java.lang.ModuleLayer should be considered immutable, but I left them out.
  • java.lang.Thread is effectively immutable. Stopping or suspending a thread is deprecated and was even removed from the newest JDKs. Only Thread.interrupt() remains to interfere with a Thread, which is already covered by other checks and is a very unlikely to be a way to attack mutability.
  • The other classes that I added should be ok.

Edit:

  • Sadly, Optional<T> is immutable if, and only if, T is immutable. But at this moment, it is too naive to know that.
  • Although, java.lang.Module and java.lang.ModuleLayer aren't 100% immutable, it is extremely unlikely that mutating them causes undesirable side-effects (most likely, only the intended ones). So, I added them to the list.

Edit 2:

  • Interfaces in java.util.function and annotations are immutable. However, sadly, for Thread, there are some stuff there that are too mutable to be safe.
  • If a Class is fair to be considered immutable, so is the ClassLoader.

@@ -86,7 +99,7 @@ public static boolean mutableSignature(String sig) {
private static final class ClassAnalysis {
private static final List<String> SETTER_LIKE_PREFIXES = Arrays.asList(
"set", "put", "add", "insert", "delete", "remove", "erase", "clear", "push", "pop",
"enqueue", "dequeue", "write", "append", "replace");
"enqueue", "dequeue", "write", "append", "replace", "prepend", "reset", "update");
Copy link
Member

Choose a reason for hiding this comment

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

I'm personally not so positive to edit these prefixes. Name methods and mutability should have no relation.
Then it is better to keep the current implementation as is, to keep backward compatibility and bring no surprise to users.

Copy link
Author

Choose a reason for hiding this comment

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

@KengoTODA This could be argued to every other previously existing member on this list. I am just building on what is already there.

In fact, I also really dislike the idea of looking the method name for common verbs in English to guess its behavior because it is very prone to go wrong in many ways. If it was for me, I would never make it behave that way to start with. However, considering that it is already there and is already coded that way, I am just adding a few more common prefixes for mutating methods.

@github-actions github-actions bot added the Stale label Mar 5, 2024
@github-actions github-actions bot closed this Apr 9, 2024
@hazendaz hazendaz reopened this Apr 12, 2024
@github-actions github-actions bot removed the Stale label Apr 14, 2024
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.

None yet

3 participants