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

issue w/ for statement #64

Closed
mattiaerre opened this issue Oct 16, 2018 · 5 comments
Closed

issue w/ for statement #64

mattiaerre opened this issue Oct 16, 2018 · 5 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@mattiaerre
Copy link
Member

Description

missing a semicolon if for statement body is not included in braces.

Source

  function count(string[] data) public pure returns (uint) {
    uint result;
    for(uint i = 0; i < data.length; i++)
      result += 1;
    return result;
  }

Prettified

  function count(string[] data) public pure returns(uint) {
    uint result;
    for (uint i = 0; i < data.length; i++) result += 1
    return result;
  }

N.B. there is a missing ; at the end of the for body

@mattiaerre mattiaerre added bug Something isn't working help wanted Extra attention is needed labels Oct 16, 2018
@mattiaerre mattiaerre added this to the alpha launch milestone Oct 16, 2018
@fvictorio
Copy link
Member

Just repeating what we've already said elsewhere. The problem seems to be here:

case 'ExpressionStatement': {
const addSemicolon = path.getParentNode().type !== 'ForStatement';
return concat([
node.expression ? path.call(print, 'expression') : '',
addSemicolon ? ';' : ''
]);
}

where we add a semicolon only if the parent node is not a for statement. But if we don't do this, then an extra semicolon is added to the loop condition, because that is an ExpressionStatement too.

An extremely ugly fix would be to mutate the node so that the print function knows if it should add the semicolon. I don't like this, but it could work.

Another idea is to never add a semicolon in the ExpressionStatement printer, and make that a responsibility of the parent node.

@mattiaerre
Copy link
Member Author

mattiaerre commented Oct 17, 2018

I like the second option much more but if we can have something that still fixes the problem I really don't mind if it's really terrible tbh; happy if you want to create a PR that fixes this issue btw

@fvictorio
Copy link
Member

The more I think about this, the more convinced I am that this is a problem upstream, so I opened federicobond/solidity-parser-antlr#39 to see if it can be changed. But this may be a breaking change, so I don't know if it will be modified/fixed.

I say we do this: if the parser is modified and that allows us to do a proper fix, we do that. If not, let's do the easy/mutating approach, and revisit it later if needed.

@mattiaerre
Copy link
Member Author

shall we decorate our parser so that it returns the structure we think it's more beneficial for us? that be also another option what do you think?

@fvictorio
Copy link
Member

That's a great idea. Let's do that for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants