XMage é um aplicativo cliente / servidor para jogar Magic: The Gathering (MTG). O XMage começou a evoluir no início de 2010. Durante esse tempo, 182 lançamentos foram lançados, todo um exército de colaboradores se reuniu e o projeto ainda está em desenvolvimento ativo. Uma excelente oportunidade de participar também do seu desenvolvimento! Portanto, hoje o unicórnio do PVS-Studio irá verificar a base de código XMage e, quem sabe, pode colidir com alguém na batalha.
Resumidamente sobre o projeto
O XMage vem se desenvolvendo ativamente há 10 anos. Seu objetivo é fazer uma versão online gratuita e de código aberto do jogo de cartas Magic: the Gathering original .
Recursos do aplicativo:
- acesso a ~ 19.000 cartões exclusivos emitidos ao longo dos 20 anos de história da MTG;
- controle automático e aplicação de todas as regras existentes do jogo;
- ;
- (AI);
- (Standard, Modern, Vintage, Commander );
- , .
Me deparei com o trabalho de alunos da Delft University of Technology 2018 (curso de Mestrado em Arquitetura de Software ). Consistia no fato de que os caras participavam ativamente de projetos de código aberto, que deviam ser bastante complexos e de desenvolvimento ativo. Durante um período de oito semanas, os alunos estudaram o curso e os projetos de código aberto para compreender e descrever a arquitetura do software selecionado.
Então é isso. Neste trabalho, os caras analisaram o projeto XMage, e um dos aspectos do trabalho deles foi a obtenção de várias métricas usando SonarQube (número de linhas de código, complexidade ciclomática, duplicação de código, cheiros de código, erros, vulnerabilidades, etc.).
Minha atenção foi atraída pelo fato de que na época de 2018 a varredura SonarQube mostrava 700 defeitos (bugs, vulnerabilidades) por 1.000.000 de linhas de código.
Depois de pesquisar a história dos caras contribuintes, descobri que, a partir do relatório recebido com avisos, eles fizeram uma solicitação de pull para corrigir cerca de 30 defeitos da categoria "Bloqueador" ou "Crítico". O que acontece com o resto dos avisos é desconhecido, mas espero que eles não tenham sido esquecidos.
Já se passaram 2 anos desde então e a base de código cresceu cerca de 250.000 linhas de código - um bom motivo para ver como as coisas estão indo.
Sobre análise
Para análise, peguei o lançamento do XMage - 1.4.44V0 .
Tive muita sorte com o projeto. Construir o XMage usando Maven acabou sendo muito simples (como estava escrito na documentação):
mvn clean install -DskipTestsNada mais foi exigido de mim. Legal?
Não houve problemas com a integração do plug-in PVS-Studio no Maven: tudo está como na documentação .
Após a análise, foram recebidos 911 avisos, dos quais 674 foram para avisos de 1 e 2 níveis de confiança. Para os fins deste artigo, não considerei avisos de nível 3, pois geralmente há uma alta porcentagem de falsos positivos. Gostaria de chamar a atenção para o fato de que ao usar um analisador estático em uma batalha real, você não pode ignorar tais avisos, pois eles também podem indicar defeitos significativos no código.
Além disso, não considerei os avisos de algumas das regras pelo motivo de que são mais bem considerados por aqueles que estão familiarizados com o projeto do que por mim:
- V6022, /. 336 .
- V6014, , . 73 .
- V6021, , . 36 .
- V6048, , . 17 .
Além disso, várias regras de diagnóstico produziram cerca de 20 falsos positivos óbvios do mesmo tipo. Gravado em todo!
Como resultado, se subtrairmos tudo, cerca de 190 positivos chegam a mim para consideração.
Ao revisar os gatilhos, muitos pequenos defeitos do mesmo tipo foram identificados, os quais estavam relacionados à depuração ou verificação ou operação sem sentido. Além disso, muitos aspectos positivos foram associados a um trecho de código muito estranho que implorava por refatoração.
Como resultado, para este artigo, identifiquei 11 regras de diagnóstico e analisei um dos gatilhos mais interessantes.
Vamos dar uma olhada no que aconteceu.
Aviso N1
V6003 O uso do padrão 'if (card! = Null) {...} else if (card! = Null) {...}' foi detectado. Existe uma probabilidade de presença de erro lógico. TorrentialGearhulk.java (90), TorrentialGearhulk.java (102)
@Override
public boolean apply(Game game, Ability source) {
....
Card card = game.getCard(....);
if (card != null) {
....
} else if (card != null) {
....
}
....
}Tudo é simples aqui: o corpo da segunda declaração condicional if (card! = Null) na construção if-else-if nunca será executado, já que o programa não alcançará este ponto, ou card! = Null sempre será falso .
Aviso N2
V6004 A instrução 'then' é equivalente à instrução 'else'. AsThoughEffectImpl.java (35), AsThoughEffectImpl.java (37)
@Override
public boolean applies(....) {
// affectedControllerId = player to check
if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) {
return applies(objectId, source, playerId, game);
} else {
return applies(objectId, source, playerId, game);
}
}Um erro comum que frequentemente ocorria em minha prática de verificar projetos de código aberto. Copiar colar? Ou eu estou esquecendo de alguma coisa? Vou assumir que você ainda precisa retornar false no branch else . PS Se houver alguma coisa, não há nenhuma chamada recursiva se aplica (....) , uma vez que esses são métodos diferentes. Acionamento semelhante:
- V6004 A instrução 'then' é equivalente à instrução 'else'. GuiDisplayUtil.java (194), GuiDisplayUtil.java (198)
Aviso N3
V6007 Expression 'filter.getMessage (). ToLowerCase (Locale.ENGLISH) .startsWith ("Each")' é sempre falso. SetPowerToughnessAllEffect.java (107)
@Override
public String getText(Mode mode) {
StringBuilder sb = new StringBuilder();
....
if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
sb.append(" has base power and toughness ");
} else {
sb.append(" have base power and toughness ");
}
....
return sb.toString();
}Os gatilhos de regra de diagnóstico V6007 são bastante populares para todos os projetos que estão sendo verificados. XMage não é exceção (79 peças). A atuação da regra, em princípio, tudo fica no case, mas muitos casos caem no debug, depois no resseguro, depois em outro. Em geral, é melhor para o autor do código observar esses aspectos positivos do que para mim.
Esse acionamento, entretanto, é definitivamente um erro. Dependendo do início da linha, filter.getMessage () para sbo texto "tem ..." ou "tem ..." é adicionado. Mas o erro é que os desenvolvedores verifiquem se a string começa com uma letra maiúscula, tendo convertido essa mesma string para minúscula antes disso. Opa. Como resultado, a linha adicionada sempre será "have ...". O resultado do defeito não é crítico, mas também desagradável: um texto composto de forma analfabeta aparecerá em algum lugar.
Os pontos positivos que achei mais interessantes:
- A expressão V6007 't.startsWith ("-")' é sempre falsa. BoostSourceEffect.java (103)
- A expressão V6007 'setNames.isEmpty ()' é sempre falsa. DownloadPicturesService.java (300)
- A expressão V6007 'existingBucketName == null' é sempre falsa. S3Uploader.java (23)
- Expressão V6007 '! LastRule.endsWith (".")' É sempre verdadeira. Effects.java (76)
- A expressão V6007 'subtypesToIgnore :: contains' é sempre falsa. VerifyCardDataTest.java (893)
- A expressão V6007 'notStartedTables == 1' é sempre falsa. MageServerImpl.java (1330)
Aviso N4
V6008 Dereferência nula de 'savedSpecialRares'. DragonsMaze.java (230)
public final class DragonsMaze extends ExpansionSet {
....
private List<CardInfo> savedSpecialRares = new ArrayList<>();
....
@Override
public List<CardInfo> getSpecialRare() {
if (savedSpecialRares == null) { // <=
CardCriteria criteria = new CardCriteria();
criteria.setCodes("GTC").name("Breeding Pool");
savedSpecialRares.addAll(....); // <=
criteria = new CardCriteria();
criteria.setCodes("GTC").name("Godless Shrine");
savedSpecialRares.addAll(....);
....
}
return new ArrayList<>(savedSpecialRares);
}
}O analisador reclama sobre o cancelamento da referência da referência nula savedSpecialRares quando a execução atinge o primeiro preenchimento da coleção.
A primeira coisa que vem à mente é simplesmente confundir saveSpecialRares == null com savedSpecialRares! = Null. Mas, nesse caso, o NPE pode acontecer no construtor de ArrayList quando a coleção é retornada do método, já que savedSpecialRares == null ainda é possível. Corrigir o código com a primeira solução que vier à mente não é uma boa opção. Tendo entendido um pouco o código, descobri que s avedSpecialRares é imediatamente definido por uma coleção vazia quando é declarada e não é reatribuída em nenhum outro lugar. Isso nos diz quesaveSpecialRares nunca será nulo, e a desreferenciação de uma referência nula, sobre a qual o analisador avisa, nunca acontecerá, pois nunca chegará à coleção. Como resultado, o método sempre retornará uma coleção vazia.
PS Para corrigir isso, você precisa substituir savedSpecialRares == null por savedSpecialRares.isEmpty () .
PPS: Infelizmente, ao jogar XMage, você não poderá obter cartas raras especiais para a coleção Labirinto do Dragão .
Outro caso de desreferenciação de uma referência nula:
- V6008 Dereferência nula de 'correspondência' TableController.java (973)
Aviso N5
V6012 O operador '?:', Independentemente de sua expressão condicional, sempre retorna um e o mesmo valor 'table.getCreateTime ()'. TableManager.java (418), TableManager.java (418)
private void checkTableHealthState() {
....
logger.debug(.... + formatter.format(table.getStartTime() == null
? table.getCreateTime()
: table.getCreateTime()) + ....);
....
}Aqui, o operador ternário ?: Retorna o mesmo valor, independentemente da condição table.getStartTime () == null . Eu acredito que o autocompletar código foi uma piada cruel com o desenvolvedor. Opção de correção:
private void checkTableHealthState() {
....
logger.debug(.... + formatter.format(table.getStartTime() == null
? table.getCreateTime()
: table.getStartTime()) + ....);
....
}Aviso N6
V6026 Este valor já está atribuído à variável 'this.loseOther'. BecomesCreatureTypeTargetEffect.java (54)
public
BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {
super(effect);
this.subtypes.addAll(effect.subtypes);
this.loseOther = effect.loseOther;
this.loseOther = effect.loseOther;
}String de atribuição duplicada. Parece que o desenvolvedor se empolgou um pouco com as teclas de atalho e não percebeu. Mas dado que o efeito possui um grande número de campos, o fragmento merece ser focado.
Aviso N7
V6036 O valor do opcional 'selectUser' não inicializado é usado. Session.java (227)
public String connectUserHandling(String userName, String password)
{
....
if (!selectUser.isPresent()) { // user already exists
selectUser = UserManager.instance.getUserByName(userName);
if (selectUser.isPresent()) {
User user = selectUser.get();
....
}
}
User user = selectUser.get(); // <=
....
}A partir do aviso do analisador, podemos concluir que selectUser.get () pode lançar uma NoSuchElementException.
Vamos dar uma olhada no que está acontecendo aqui.
Se você acredita que o comentário de que o usuário já existe, nenhuma exceção será lançada:
....
if (!selectUser.isPresent()) { // user already exists
....
}
User user = selectUser.get()
....Nesse caso, a execução do programa não entrará no corpo da instrução condicional. E tudo vai ficar bem. Mas então surge a pergunta: por que precisamos de um operador condicional com alguma lógica complexa, se ele nunca é executado?
Mas e se o comentário não for nada?
....
if (!selectUser.isPresent()) { // user already exists
selectUser = UserManager.instance.getUserByName(userName);
if (selectUser.isPresent()) {
....
}
}
User user = selectUser.get(); // <=
....Em seguida, a execução entra no corpo da instrução condicional e obtém novamente o usuário por meio de getUserByName (). O usuário é verificado novamente quanto à validade, o que sugere que o selectUser pode não ter sido inicializado. Não há outra ramificação para este caso, o que ainda levará a uma NoSuchElementException na linha de código em questão.
Aviso N8
V6042 A compatibilidade da expressão é verificada com o tipo 'A', mas é convertida para o tipo 'B'. CheckBoxList.java (586)
/**
* sets the model - must be an instance of CheckBoxListModel
*
* @param model the model to use
* @throws IllegalArgumentException if the model is not an instance of
* CheckBoxListModel
* @see CheckBoxListModel
*/
@Override
public void setModel(ListModel model) {
if (!(model instanceof CheckBoxListModel)) {
if (model instanceof javax.swing.DefaultListModel) {
super.setModel((CheckBoxListModel)model); // <=
}
else {
throw new IllegalArgumentException(
"Model must be an instance of CheckBoxListModel!");
}
}
else {
super.setModel(model);
}
}O autor do código está confuso sobre algo aqui: primeiro ele se certifica de que o modelo não é um CheckBoxListModel e, em seguida , converte explicitamente o objeto nesse tipo. Por causa disso, o método setModel lançará imediatamente uma ClassCastException quando chegar lá.
O arquivo CheckBoxList.java foi adicionado 2 anos atrás e esse bug permanece no código desde então. Aparentemente, não há testes para parâmetros incorretos, não há nenhum uso real desse método com objetos de tipos inadequados, então ele vive.
Se de repente alguém se conectar a este método e ler o Javadoc, eles irão esperar uma IllegalArgumentException , não uma ClassCastException... Não acho que alguém vá encontrar essa exceção deliberadamente, mas quem sabe.
Dada a documentação, o código provavelmente deve ser assim:
public void setModel(ListModel model) {
if (!(model instanceof CheckBoxListModel)) {
throw new IllegalArgumentException(
"Model must be an instance of CheckBoxListModel!");
}
else {
super.setModel(model);
}
}Aviso N9
V6060 A referência do 'jogador' foi utilizada antes de ser verificada em relação a nulo. VigeanIntuition.java (79), VigeanIntuition.java (78)
@Override
public boolean apply(Game game, Ability source) {
MageObject sourceObject = game.getObject(source.getSourceId());
Player player = game.getPlayer(source.getControllerId());
Library library = player.getLibrary(); // <=
if (player != null && sourceObject != null && library != null) { // <=
....
}
}O V6060 avisa o desenvolvedor que um objeto está sendo acessado antes de ser verificado quanto a nulo . Os gatilhos desta regra são frequentemente encontrados em artigos sobre a verificação de projetos de código aberto: normalmente, a razão para isso é a refatoração malsucedida ou a alteração de contratos de métodos. Se você prestar atenção à declaração do método getPlayer () , tudo se encaixará imediatamente:
// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);Aviso N10
V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'playerB' deva ser usada em vez de 'playerA'. SubTypeChangingEffectsTest.java (162), SubTypeChangingEffectsTest.java (158), SubTypeChangingEffectsTest.java (156), SubTypeChangingEffectsTest.java (160)
@Test
public void testArcaneAdaptationGiveType() {
addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U}
addCard(Zone.BATTLEFIELD, playerA, "Island", 3);
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
....
for (Card card : playerB.getGraveyard().getCards(currentGame)) {
if (card.isCreature()) {
Assert.assertEquals(card.getName() + " should not have ORC type",
false, card.getSubtype(currentGame).contains(SubType.ORC));
Assert.assertEquals(card.getName() + " should have CAT type",
true, card.getSubtype(currentGame).contains(SubType.CAT));
}
}
}Tendo visto que esse erro está nos testes, pode-se imediatamente desvalorizar o defeito encontrado, pensando: "Bom, são testes." Se sim, então eu discordo de você. Afinal, os testes desempenham um papel bastante importante no desenvolvimento (embora não tão perceptível quanto a programação), e quando um defeito aparece em uma versão, eles imediatamente começam a apontar o dedo aos testes / testadores. Portanto, testes defeituosos são insustentáveis. Por que então esses testes são necessários? Por que desperdiçar recursos com eles?
O método testArcaneAdaptationGiveType () testa o cartão "Adaptação Arcana". Cada jogador recebe cartas para uma área de jogo específica. E, graças ao copiar e colar, o jogador A obteve 2 cartas idênticas de "Leão Casaco Prateado " na área de jogo "Cemitério" , e o jogador Bentão nada aconteceu. Em seguida, um pouco de magia e teste de si mesmo.
Quando o teste chega ao "cemitério" do jogador B no rally atual, a execução do teste nunca entra no loop, porque não havia nada no "cemitério". Isso eu descobri com o bom e velho System.out.println () ao iniciar o teste .
Copiar e colar corrigido:
....
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion"); // <=
....Depois de ajustar o código, quando executei o teste, a verificação de criaturas no cemitério do player B começou a funcionar. Ave, System.out.println () !
O teste é verde antes e depois da correção, o que é muita sorte. Mas no caso de qualquer edição que altere a lógica de execução do programa, tal teste será um péssimo serviço, notificando-o da conclusão bem-sucedida, mesmo que haja erros.
Mesmo copiar e colar em outro lugar:
- V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'playerB' deva ser usada em vez de 'playerA'. PaintersServantTest.java (33), PaintersServantTest.java (29), PaintersServantTest.java (27), PaintersServantTest.java (31)
- V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'playerB' deva ser usada em vez de 'playerA'. SubTypeChangingEffectsTest.java (32), SubTypeChangingEffectsTest.java (28), SubTypeChangingEffectsTest.java (26), SubTypeChangingEffectsTest.java (30)
Aviso N11
V6086 Formatação de código suspeito. provavelmente está faltando a palavra-chave 'else'. DeckImporter.java (23)
public static DeckImporter getDeckImporter(String file) {
if (file == null) {
return null;
} if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) { // <=
return new DecDeckImporter();
} else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) {
return new MWSDeckImporter();
} else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) {
return new TxtDeckImporter(haveSideboardSection(file));
}
....
else {
return null;
}
}A regra de diagnóstico V6086 diagnostica formatação if-else-if incorreta , implicando na omissão de else .
Este trecho de código demonstra isso. Nesse caso, por causa da expressão nula de retorno , imprecisão na formatação não leva a nada, mas é legal encontrar tais casos, já que você não precisa.
Vamos considerar um caso em que omitir o else pode levar a um comportamento inesperado:
public SomeType smtMethod(SomeType obj) {
....
if (obj == null) {
obj = getNewObject();
} if (obj.isSomeObject()) {
// some logic
} else if (obj.isOtherSomething()) {
obj = calulateNewObject(obj);
// some logic
}
....
else {
// some logic
}
return obj;
}Agora, no caso de obj == null , o objeto em questão receberá algum valor, e o else ausente fará com que o objeto recém-atribuído comece a verificar ao longo da cadeia if-else-if , enquanto o objeto deveria retornar imediatamente de método.
Conclusão
Verificando o XMage é outro artigo que revela os recursos dos modernos analisadores estáticos. No desenvolvimento moderno, a necessidade deles só aumenta à medida que aumenta a complexidade do software. E não importa quantos lançamentos, testes, feedback do usuário você tenha: um bug sempre encontrará uma brecha para entrar em sua base de código. Então, por que não adicionar outra barreira à sua defesa?
Como você entende, os analisadores estão sujeitos a falsos positivos (incluindo PVS-Studio Java). Isso pode ser o resultado de uma falha óbvia e de um código muito confuso (infelizmente, o analisador não descobriu). Você precisa tratá-los com compreensão e cancelar imediatamente a inscrição sem hesitação , mas enquanto os falsos positivos aguardam sua correção, você pode usar um dos métodossuprimindo avisos.
Concluindo, sugiro que você "toque" pessoalmente no analisador, baixando -o de nosso website.
Se você deseja compartilhar este artigo com um público de língua inglesa, por favor, use o link de tradução: Maxim Stefanov. Verificando o código do XMage e por que você não conseguirá obter as cartas raras especiais da coleção Dragon's Maze .