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

Detects "Identical blocks of code" that are in no way identical #290

Open
pitaj opened this issue Dec 3, 2017 · 4 comments
Open

Detects "Identical blocks of code" that are in no way identical #290

pitaj opened this issue Dec 3, 2017 · 4 comments

Comments

@pitaj
Copy link

pitaj commented Dec 3, 2017

https://codeclimate.com/github/benchpressjs/benchpressjs/issues

If you scroll down to the two Identical blocks of code found in 2 locations. Consider refactoring. issues, you will see that it detects

  return Promise.try(() => {
    Benchpress.cache[template] = Benchpress.cache[template] || load(template);
    return Benchpress.cache[template];
  }).then((templateFunction) => {
    if (block) {

and

  return Promise.try(() => {
    const cached = compileRenderCache.get(hash);
    if (cached) {
      compileRenderCache.ttl(hash);
      return cached;

as identical when the only thing even similar about them is Promise.try(() => {.

This is super annoying. Is it some kind of diffing or hashing bug? Should I report it on the flay repository?

@wfleming
Copy link
Contributor

wfleming commented Dec 6, 2017

Thanks for the report @pitaj, and sorry for the issue.

I'm able to reproduce this, but I'm sorry to say I don't have a fix yet. With some debugging output using the dump_ast config option I was able to see that the engine seems to be incorrectly assigning the AST for the first snippet to both instances of the code. Flay, which this engine is built on, hashes ASTs internally for tracking, so right now I suspect there's a hash conflict here, but I haven't identified exactly where yet. We're still looking into this internally, just wanted you to know it was being investigated. Thanks.

@pitaj
Copy link
Author

pitaj commented Dec 6, 2017

Thanks for looking into it.

@zenspider
Copy link
Contributor

Just quickly glancing at it... I'd bet it is trying to report the then lambdas, which are identical. I don't know the JS AST so I don't know how it is structured off the top of my head, but I'd bet that the file/line info is screwed up at some level.

@zenspider
Copy link
Contributor

Anything?

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

No branches or pull requests

3 participants