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

[Incomplete] Use RMiner to identify similar nodes on renaming #79

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

Conversation

Arthurlpgc
Copy link
Collaborator

No description provided.

@Arthurlpgc
Copy link
Collaborator Author

Figuring out now why travis is failing, also will remove the debugs soon..

@Arthurlpgc
Copy link
Collaborator Author

Among others, the case 18 of the https://gaabs.github.io/s3m-logs-web/ isn't entering handleSingleRenamings function (even on master branch, without any changes)

@pauloborba
Copy link
Collaborator

Among others, the case 18 of the https://gaabs.github.io/s3m-logs-web/ isn't entering handleSingleRenamings function (even on master branch, without any changes)

@jvcoutinho could you please confirm that?

@jvcoutinho
Copy link
Collaborator

It's true, the method terminates before the call to handleSingleRenamings is executed:

oi

Although, I don't think Travis' error was caused by this. I believe RMiner was compiled with a different Java version than the one the project operates (Java 8). Can you check this, @Arthurlpgc?

@pauloborba
Copy link
Collaborator

pauloborba commented Dec 27, 2018 via email

@jvcoutinho
Copy link
Collaborator

Yes, @pauloborba, I've tested with the master branch. I've tried to test with @Arthurlpgc's branch too, but it failed (same error of Travis).

@pauloborba
Copy link
Collaborator

Yes, @pauloborba, I've tested with the master branch. I've tried to test with @Arthurlpgc's branch too, but it failed (same error of Travis).

So it is a bug in our code, not in Arthur's code. @jvcoutinho, @gaabs and @guilhermejccavalcanti any idea of what's causing that and when it was introduced? it might be an issue with the methods that detect changed/added nodes.

@guilhermejccavalcanti
Copy link
Owner

guilhermejccavalcanti commented Dec 28, 2018

Pessoal, neste exemplo mencionado, o 18, há um renaming mútuo (left e right modificam a assinatura do mesmo método). Deste modo, ambas novas versões aparecem como novos nós de left e right (em MergeContext.addedRightNodes e MergeContext.addedLeftNodes), e o método original da base é considerado como nó apagado tanto por left quanto por right (em MergeContext.nodesDeletedByLeft e MergeContext.nodesDeletedByRight).

A lista de possíveis nós renomeados( MergeContext.possibleRenamedLeftNodes e MergeContext.possibleRenamedRightNodes) fica corretamente vazia de acordo com a implementação atual pois um nó só entre nessa lista quando há um matching de left ou right com a base, e neste caso não há pois ambos mudaram a assinatura, sã0 novos nós afinal. Então, corretamente, handleSingleRenaming não vai ser executado pois suas condições não são satisfeitas (MergeContext.possibleRenamedLeftNodes ou MergeContext.possibleRenamedRightNodes não-vazias).

Quanto ao handleMutualRenaming, para que ele reporte conflito 3 condições precisam ser satisfeitas, como ilustrado abaixo:
image

No entanto, para o exemplo em questão, RenamingUtils.haveSameBody(left, right) retorna falso e o conflito não é reportado.

@pauloborba haviamos discutido que sempre que houvesse renaming mútuo deveria ser reportado conflito. Neste exemplo, ao ver estas 3 condições, percebi a real limitação disso: como saber que dois nós são de fato versões renomeadas de uma mesma base? Atualmente usamos a igualdade do corpo dos métodos. Teriamos que usar similaridade textual com o risco de introduzir falso positivo, pois sempre vai haver a possibilidade de dois nós conceitualmente distintos, serem similares e como tal serem casados pelo handler.

Quanto ao problema do travis, não tem relação com o exemplo, e sim com um mismatch entre a versão de java do S3M e do RMMiner:
image

https://stackoverflow.com/a/29907195 (no nosso caso, o s3m é java 8, e a do plugin é java 10)

@pauloborba
Copy link
Collaborator

Pessoal, neste exemplo mencionado, o 18, há um renaming mútuo (left e right modificam a assinatura do mesmo método). Deste modo, ambas novas versões aparecem como novos nós de left e right (em MergeContext.addedRightNodes e MergeContext.addedLeftNodes), e o método original da base é considerado como nó apagado tanto por left quanto por right (em MergeContext.nodesDeletedByLeft e MergeContext.nodesDeletedByRight).

@guilhermejccavalcanti observa que, no código testado por @jvcoutinho, o handleSingleRenaming nem é chamado. acho que @gaabs que extraiu o if. essa extração é um refactoring (olha mais abaixo a evidência de que não é)?

A lista de possíveis nós renomeados( MergeContext.possibleRenamedLeftNodes e MergeContext.possibleRenamedRightNodes) fica corretamente vazia de acordo com a implementação atual pois um nó só entre nessa lista quando há um matching de left ou right com a base, e neste caso não há pois ambos mudaram a assinatura, sã0 novos nós afinal.

@guilhermejccavalcanti a intenção não é de que MergeContext.possibleRenamedLeftNodes englobe tanto nós possivelmente renomeados quanto nós possivelmente deletados?

Então, corretamente, handleSingleRenaming não vai ser executado pois suas condições não são satisfeitas (MergeContext.possibleRenamedLeftNodes ou MergeContext.possibleRenamedRightNodes não-vazias).

perfeito. mas como esse teste parece ter sido extraído, nem chamado handleSingleRenaming é. a bronca maior é que handleMutualRenaming também nem chamado será! entendeu @guilhermejccavalcanti?

Quanto ao handleMutualRenaming, para que ele reporte conflito 3 condições precisam ser satisfeitas, como ilustrado abaixo:
image

No entanto, para o exemplo em questão, RenamingUtils.haveSameBody(left, right) retorna falso e o conflito não é reportado.

@pauloborba haviamos discutido que sempre que houvesse renaming mútuo deveria ser reportado conflito. Neste exemplo, ao ver estas 3 condições, percebi a real limitação disso: como saber que dois nós são de fato versões renomeadas de uma mesma base? Atualmente usamos a igualdade do corpo dos métodos.

@guilhermejccavalcanti igualdade desconsiderando whitespace?

Teriamos que usar similaridade textual com o risco de introduzir falso positivo, pois sempre vai haver a possibilidade de dois nós conceitualmente distintos, serem similares e como tal serem casados pelo handler.

então dois desenvolvedores renomearem o mesmo método, com nomes diferentes, e pelo menos um deles mudar o corpo é um potencial FN adicionado do s3m. é bom registrar isso.

precisamos rever o algoritmo para refinar melhor as formas de distinção entre renomeação e deleção.

Quanto ao problema do travis, não tem relação com o exemplo, e sim com um mismatch entre a versão de java do S3M e do RMMiner:
image

https://stackoverflow.com/a/29907195 (no nosso caso, o s3m é java 8, e a do plugin é java 10

@Arthurlpgc você pode verificar isso, por favor?

@guilhermejccavalcanti
Copy link
Owner

guilhermejccavalcanti commented Dec 28, 2018

@guilhermejccavalcanti observa que, no código testado por @jvcoutinho, o handleSingleRenaming nem é chamado. acho que @gaabs que extraiu o if. essa extração é um refactoring (olha mais abaixo a evidência de que não é)?

handleSingleRenaming é chamado sim mas como a as condições das listas não-vazias falha, ele não prossegue:
image

@guilhermejccavalcanti a intenção não é de que MergeContext.possibleRenamedLeftNodes englobe tanto nós possivelmente renomeados quanto nós possivelmente deletados?

MergeContext.possibleRenamedLeftNodes armazena nós de left renomeados ou apagados por right. Isso implica que houve um matching de left com a base na superimposição. No exemplo em questão, como há um renaming mútuo, superimposição não faz essa matching pois os 3 métodos (left, base e right) tem assinaturas diferentes, e portanto não insere nessa lista, apenas insere em MergeContext.nodesDeleted...

perfeito. mas como esse teste parece ter sido extraído, nem chamado handleSingleRenaming é. a bronca maior é que handleMutualRenaming também nem chamado será! entendeu @guilhermejccavalcanti?

handleMutualRenaming foi chamado sim:
image

@guilhermejccavalcanti igualdade desconsiderando whitespace?

Sim.

então dois desenvolvedores renomearem o mesmo método, com nomes diferentes, e pelo menos um deles mudar o corpo é um potencial FN adicionado do s3m. é bom registrar isso.

Sim.

Resumindo, na minha concepção, o S3M funcionou corretamente no exemplo, pelos menos como está implementado para funcionar.

precisamos rever o algoritmo para refinar melhor as formas de distinção entre renomeação e deleção.

Ai vem aquela tabela que discutimos com @gaabs mostrando todos os tipo de interações relacionadas ao renaming pra vermos o que tá sendo feito, o que falta, e se tá correto.

@pauloborba
Copy link
Collaborator

pauloborba commented Dec 28, 2018 via email

@guilhermejccavalcanti
Copy link
Owner

@guilhermejccavalcanti olha a imagem postada por joão. você está olhando outro código.

Da resposta #79 (comment) ? É o mesmo código! Método handleSingleRenamings(MergeContext context) da classe MethodAndConstructorRenamingAndDeletionHandler.

@pauloborba
Copy link
Collaborator

@guilhermejccavalcanti olha a imagem postada por joão. você está olhando outro código.

Da resposta #79 (comment) ? É o mesmo código! Método handleSingleRenamings(MergeContext context) da classe MethodAndConstructorRenamingAndDeletionHandler.

tem razão! achei que era o método que chama tanto single quanto mutual, que a condicao tinha sido extraida para esse nivel. sendo assim, nao era para executar mesmo o resto do método de single, @Arthurlpgc!

@Arthurlpgc
Copy link
Collaborator Author

Bom ponto, eu me enganei achando que ele iria chamar o HandleSingleRenaming, vou dar uma olhada para retestar isso, e parar compilar o RMiner com java correto, devo olhar esse issue domingo, aviso aqui os updates, Obrigado pelo feedback (:

@Arthurlpgc
Copy link
Collaborator Author

Fixed the build, will update the PR with the changes to make the code cover the different refactoring not covered before(As I had dealt with renamings only, and not changes to the signature)

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.

4 participants