Our website uses cookies to enhance your browsing experience.
A computer game written in Java is rare but always interesting. That's why we couldn't miss the opportunity to check the XMage project using a static analyzer. Let's explore what PVS-Studio detected in its source code.
XMage is a free, open-source version of Magic: the Gathering. Players can battle using cards both locally with bots and online with friends or random players.
The project is written entirely in Java and has been in active development for over 10 years. Today, the repository contains more than 47,000 commits and 1.8 million code lines. Five years ago, our team has already analyzed XMage and even posted an article. Back then, the project was around 800,000 lines smaller, so we thought it was time for a fresh look.
What's important to mention is that the project uses various code quality tools, including static analyzers. So, we're doubly interested to see what our analyzer can detect. It's time to examine the findings!
The eternal question that tears developers' minds, "If tests control the code quality, then what controls the quality of the tests themselves?"
Let's examine detected errors in the XMage unit tests.
@Test
public void test_NamesEquals() {
....
// same names (empty names can't be same)
Assert.assertFalse(CardUtil.haveSameNames(
"",
""));
Assert.assertFalse(CardUtil.haveSameNames(
EmptyNames.FACE_DOWN_CREATURE.getObjectName(),
""));
Assert.assertFalse(CardUtil.haveSameNames(
EmptyNames.FACE_DOWN_CREATURE.getObjectName(),
EmptyNames.FACE_DOWN_CREATURE.getObjectName()));
Assert.assertFalse(CardUtil.haveSameNames(
EmptyNames.FACE_DOWN_TOKEN.getObjectNam(),
""));
Assert.assertFalse(CardUtil.haveSameNames(
EmptyNames.FACE_DOWN_TOKEN.getObjectNam(),
EmptyNames.FACE_DOWN_CREATURE.getObjectName())); // <=
....
}The PVS-Studio warning: V6072 Two similar code fragments were found. Perhaps, this is a typo and 'FACE_DOWN_TOKEN' variable should be used instead of 'FACE_DOWN_CREATURE'. AliasesApiTest.java 30
The analyzer points out that developers might have used FACE_DOWN_TOKEN instead of FACE_DOWN_CREATURE in the highlighted line.
The test fragment checks whether "empty names" are compared correctly. Logically, empty names should yield a negative result when compared to identity, regardless of whether they reference the same constant. That's what the comment above says as well.
It looks like a case of an odd copypasta. FACE_DOWN_CREATURE is compared with itself, but FACE_DOWN_TOKEN is not.
After altering the constant, the unit test starts to work as expected.
@Test
public void test_Simple_LongGame() {
....
addCard(Zone.LIBRARY, playerA, "Mountain", 10);
addCard(Zone.LIBRARY, playerA, "Forest", 10);
addCard(Zone.LIBRARY, playerA, "Lightning Bolt", 20);
addCard(Zone.LIBRARY, playerA, "Balduvian Bears", 10);
//
addCard(Zone.LIBRARY, playerB, "Mountain", 10);
addCard(Zone.LIBRARY, playerA, "Forest", 10); // <=
addCard(Zone.LIBRARY, playerB, "Lightning Bolt", 20);
addCard(Zone.LIBRARY, playerB, "Balduvian Bears", 10);
....
}The PVS-Studio warning: V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SimulationPerformanceAITest.java 60
I think when you see the arrow in the code snippet above, you understand why I've added it in this article :)
The test checks that AI vs AI matches are properly handled from a performance standpoint—though, not so fair. Gentleman A received more forest cards than Gentleman B. Oh again: it looks like a sloppy copypasta. But there is nothing serious: the test was passing before the fix and will remain so afterward.
The PVS-Studio warning: V6008 Null dereference of 'permanent'. ThreeTreeScribe.java 78
@Override
public boolean checkTrigger(GameEvent event, Game game) {
....
Permanent permanent = zEvent.getTarget();
return ( permanent != null
|| !permanent.isControlledBy(getControllerId())) // <=
&& (permanent.getId().equals(getSourceId())
|| permanent.isCreature(game));
}Here's an obvious invalid condition in this code snippet that's hard not to smile at a little bit. Inside return, developers have dereferenced permanent if it's null.
The next example is also related to V6008 diagnostic rule and also about conditions. However, this time, there are two warnings for one code fragment at once:
V6008 Potential null dereference of 'player'. DelifsCone.java 104
V6008 Potential null dereference of 'permanent'. DelifsCone.java 104
@Override
public boolean apply(Game game, Ability source) {
Player player = game.getPlayer(source.getControllerId());
Permanent
permanent = game.getPermanent(....);
if (player != null || permanent != null) {
player.gainLife(permanent.getPower().getValue(), game, source);
return true;
}
return false;
}Checking only one object for null while dereferencing both of them at once looks like not so right approach. Clearly, the condition should have used && instead of ||. Btw, we encounter such kind of typos quite often in various projects.
static {
for (int count = 1; count <= 6; count++) {
FilterCard filter = new FilterCreatureCard(
"creature card that's exactly " +
CardUtil.numberToText(count) +
" color" + (count > 0 ? "s" : "")
);
filter.add(new EvolvingDoorPredicate(count));
filterMap.put(count, filter);
}
}The PVS-Studio warning: V6007 Expression 'count > 0' is always true. EvolvingDoor.java 76
Indeed, it's a strange condition. Looking at the constructed string, we can see that regardless of the loop's value, the ending "s" is added to the word "color." If we replace 0 with 1 in the code, grammar will be correct, and the loop will work as intended.
The PVS-Studio warning: V6060 The 'table' reference was utilized before it was verified against null. TableController.java 725
private void startGame(UUID choosingPlayerId) throws GameException {
try {
....
} catch (Exception ex) {
logger.fatal("Error starting game table: " + table.getId(), ex);
if (table != null) {
managerFactory.tableManager().removeTable(table.getId());
}
....
}
}It's a good deal to check for null—but all in good time. Well, developers use table first and only then check it for null.
Another file catches the same warning:
V6060 The 'enchantment' reference was utilized before it was verified against null. PublicEnemy.java 79
@Override
public boolean applies(Permanent permanent, Ability source, Game game) {
Permanent
enchantment = game.getPermanent(source.getSourceId());
Permanent
enchantedCreature = game.getPermanent(enchantment.getAttachedTo());
if (enchantment == null || enchantedCreature == null) {
return false;
}
if (permanent.isControlledBy(enchantedCreature.getControllerId())) {
return false;
}
return true;
}Developers use the enchantment object to create enchantedCreature, and only then check for null.
The PVS-Studio warning: V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'card'. SkyclaveApparition.java 108
@Override
public boolean apply(Game game, Ability source) {
....
Set<UUID> owners = new HashSet<>();
int totalCMC = exile
.getCards(game)
.stream()
.filter(Objects::nonNull)
.map(card -> owners.add(card.getOwnerId()) ? card : card) // <=
.mapToInt(MageObject::getManaValue)
.sum();
....
return true;
}Here's a rather odd code fragment that can easily bewilder someone who is unfamiliar with this class. Either devs would like to do something with the card object in one of the branches of the ternary operator but forgot—or they just wanted to add all stream objects to the owners instance of the Set class. If it's the second case, developers can use the peek method instead of map.
peek returns the input data stream while additionally performing some actions on its elements.
map returns the data stream with the output of applying the specified method to the input data stream.
Let's train our eyes and try to do a little interprocedural analysis on our own.
Look at this code fragment:
@Override
public boolean apply(Game game, Ability source) {
....
Card card = getCard(player, toReveal, game);
....
player.moveCards(card, Zone.BATTLEFIELD, source, game);
....
Permanent creature = game.getPermanent(card.getId()); // <=
....
}The PVS-Studio warning: V6008 Potential null dereference of 'card'. AspiringChampion.java 84
Now take a peek at the getCard method where card is obtained. One of the method execution paths may return null:
private static Card getCard(Player player, Cards toReveal, Game game) {
for (Card card : player.getLibrary().getCards(game)) {
toReveal.add(card);
if (card.isCreature(game)) {
return card;
}
}
return null;
}In the implementations of the moveCards method, which uses the card object, this is taken into account:
@Override
public boolean moveCards(Card card, ....) {
Set<Card> cardList = new HashSet<>();
if (card != null) {
cardList.add(card);
}
return moveCards(cardList, toZone, source, game,
tapped, faceDown, byOwner, appliedEffects);
}If we go back to the original code snippet, we can see that in the highlighted line, card is accessed without checking whether it might be null.
The PVS-Studio warning: V6013 Strings 'referenceName' and '""' are compared by reference. Possibly an equality comparison was intended. UrzasHotTub.java 112
private boolean sharesWordWithName(String str) {
if (referenceName == null || referenceName == "") {
return false;
}
. . . .
}It's a quite common error encountered in various projects—especially ancient ones. Strings aren't primitives, so == compares object references, not objects themselves. Even with Java's String Pool, there's no guarantee that two different string variables will reference the same object. The behavior is non-deterministic—so is the result. We should use the equals method—and especially here, the isEmpty method.
The PVS-Studio warning: V6087 InvalidClassException may occur during deserialization. Non-serializable ancestor 'RoomImpl' is missing an accessible no-arg constructor. GamesRoomImpl.java 32
public class GamesRoomImpl extends RoomImpl implements GamesRoom, Serializable {
....
}The class is serializable, but its first non-serializable superclass doesn't have a public no-arg constructor:
public abstract class RoomImpl implements Room {
....
public RoomImpl(ChatManager chatManager) {
roomId = UUID.randomUUID();
chatId = chatManager.createChatSession("Room " + roomId);
}
....
}Why is this a problem?
We've previously published an article where we took a brief look under the hood of Java native serialization and deserialization (click here). I think it'll be useful if I show a small quote from it:
The deserializable object is created not via its constructor but via the constructor of its first non-serializable superclass. This makes sense because the deserializable object and its superclasses that support serialization are restored from the byte stream. It's meaningless to execute code from constructors and initializers at this point. We're left only with non-serializable superclasses, so to initialize them, we call the constructor of the first non-serializable superclass of the restored object.
If the required constructor is missed during deserialization, an InvalidClassException exception will be thrown with the no valid constructor message.
So, as the saying goes: "Call yourself Serializable—then have a superclass with a public no-arg constructor."
The PVS-Studio warning: V6102 Inconsistent synchronization of the 'continuousRuleModifyingEffects' field. Consider synchronizing the field on all usages. ContinuousEffects.java 43
public class ContinuousEffects {
....
private ContinuousEffectsList<....>
continuousRuleModifyingEffects = new ContinuousEffectsList<>();
....
public synchronized void removeInactiveEffects(Game game) {
....
continuousRuleModifyingEffects.removeInactiveEffects(game);
....
}
....
public synchronized void removeEndOfCombatEffects() {
....
continuousRuleModifyingEffects.removeEndOfCombatEffects();
....
}
....
public boolean preventedByRuleModification(....) {
for (.... effect : continuousRuleModifyingEffects) { // <=
if (!effect.checksEventType(event, game)) {
continue;
}
....
}
....
}
....
public synchronized void addEffect(....) {
....
switch (effect.getEffectType()) {
....
case CONTINUOUS_RULE_MODIFICATION:
continuousRuleModifyingEffects.addEffect(....);
break;
....
}
}
....
}I've shown only a couple of spots in the example, but in the code, the continuousRuleModifyingEffects field is used within a synchronized block in eight methods, while in the ninth, it is not. I don't include each of the code fragments to avoid clutter.
Since the methods through which the field is interacted with are marked as synchronized, it's assumed that the interaction with the object takes place in a multithreaded block. However, interaction with the continuousRuleModifyingEffects field in the preventedByRuleModification method is unsynchronized in any way.
The collection is modified in one thread in one of the synchronized methods and read in another in a non-synchronized method. This can lead to the following problems:
ConcurrentModificationException exception may be thrown because this collection inherits from ArrayList, and in the unsynchronized method iterating over it is done via for-each;To avoid such problems, access to the collection should be synchronized. This can be done either through synchronized external invocations or through the use of collections that are already thread-safe.
The equals method is overridden, but hashCode is not.
public class CounterView implements Serializable {
. . . .
@Override
public boolean equals(Object other) {
if (other == this) {
return true;
}
if (other == null) {
return false;
}
if (!(other instanceof CounterView)) {
return false;
}
CounterView oth = (CounterView) other;
return
(count == oth.count) &&
(name.equals(oth.name));
}
}The PVS-Studio warning: V6049 Classes that define 'equals' method must also define 'hashCode' method. Class: 'CounterView'. CounterView.java 10
Personally, I feel that the rule about overriding equals and hashCode is etched in every Java developer's mind. Yet, there are several warnings in the project indicating violations of their override contract. That's why, I think it's worth repeating this information. Besides, it's possible that our articles may be read not only by seasoned developers but also by Java beginners. So, let's go over the basics once again.
There is a set of rules for overriding the equals and hashCode methods. I won't mention all of them here, only hashCode:
If two objects are equal according to the equals (Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.
To comply with this rule, developers need to override the equals and hashCode methods in a pair. This is also what the documentation for the equals method says:
It is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.
What is the purpose of such a rule? It ensures obtaining valid results when interacting with data structures that use a hash table under the hood. For example, HashMap uses the hashCode and equals methods to check if an object exists in a structure and then retrieve it. Without the overridden hashCode method, the class can duplicate objects in HashSet, which we definitely don't want.
Even if the object isn't intended to be used immediately in hash-based structures, it doesn't mean the situation won't change. The best option is to follow the documentation recommendations. So, developers can save time searching for bugs if they avoid problems right away.
The PVS-Studio warning: V6114 The 'ComputerPlayerMCTS' class contains the 'threadPoolSimulations' Closeable field, but the resources that the field is holding are not released inside the class. ComputerPlayerMCTS.java 39
public class ComputerPlayerMCTS extends ComputerPlayer {
....
private ExecutorService threadPoolSimulations = null;
....
protected void applyMCTS(....) {
if (this.threadPoolSimulations == null) {
....
this.threadPoolSimulations = new ThreadPoolExecutor(....);
}
....
try {
List<Future<Boolean>>
runningTasks = threadPoolSimulations.invokeAll(tasks,
thinkTime,
TimeUnit.SECONDS);
....
}
....
}
....
}The warning indicates that the Closable object is not closed. Since when did ExecutorService become Closable? Starting with Java 19 , ExecutorSerivce implements the AutoClosable interface, which allows ExecutorService to be seamlessly used in try-with-resource.
Note that the project is minimally supported on Java 8. I've built and tested it on version 21, which is why the warning about Closable appears :) This means the project itself doesn't assume ExecutorService implements Closeable. Nevertheless, its resources should be properly shut down, and that's not happening here. This class documentation clearly states this:
An unused ExecutorService should be shut down to allow reclamation of its resources.
The same documentation has examples of how we can correctly shut down an instance of ExecutorService.
After reviewing a large amount of code, we get a little tired. And maybe everyone wants to chill and enjoy something. The next couple of snippets struck me as entertaining, so I think I can share them with you :)
public class Session {
....
// TODO: enableafter full research
private static final boolean
SUPER_DUPER_BUGGY_AND_FASTEST_ASYNC_CONNECTION = false;
....
public void fireCallback(final ClientCallback call) {
....
try {
if (valid && callBackLock.tryLock(50, TimeUnit.MILLISECONDS)) {
....
boolean sendAsync = SUPER_DUPER_BUGGY_AND_FASTEST_ASYNC_CONNECTION
&& call.getMethod().getType().canComeInAnyOrder(); // <=
....
}
....
}
....
}
....
}The PVS-Studio warning: V6019 Unreachable code detected. It is possible that an error is present. Session.java 438
The warning indicates that the code fragment is unreachable. When I looked at it, I couldn't help but smile. I don't encounter such expression like SUPER_DUPER_BUGGY_AND_FASTEST_ASYNC_CONNECTION every day. However, in the same class, developers have an extensive discussion about implementing asynchronous data transfer from the server to the client during gameplay. A huge wall of comments outlines the pros and cons of integrating this into the game. The comment "enable after full research" next to the field declaration refers exactly to that :)
// TODO: investigate why we just discard sourceId and provide source directly?
public ZoneChangeGroupEvent(...., UUID sourceId, Ability source, ....) {
super(....);
this.fromZone = fromZone;
this.toZone = toZone;
this.cards = cards;
this.tokens = tokens;
this.source = source;
}The PVS-Studio warning: V6022 Parameter 'sourceId' is not used inside constructor body. ZoneChangeGroupEvent.java 23
When a team works on a large project, it's easy for developers to never come across some parts of the code their colleagues write. The situation above feels exactly like that.
The analyzer has detected that sourceId isn't used in the constructor. Interestingly, one of the developers had already noticed this and left a TODO comment about it. Honestly, that little note made me smile he-he.
I hope you found this the XMage exploration as interesting as I did—even if only a little bit. I'm glad that this long-running project is still going strong. I'm also happy that there are such fascinating projects in Java that are full-fledged games.
By the way, we've published an article where we've dived into bugs detected by the analyzer in a Minecraft mod and analyzed a game engine written in Java.
If you're curious about what the analyzer can do and want to give this tool a try, I'll leave a link for you.
That's all for now. See ya soon!
0
0
0
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
1056
0
0