Fix Pragma: disallow prefixing operators with version ranges

[eduardfina]

Fixes #13920

When pragma is using version ranges (-) it doesn't allow to use prefixing operators (<, <=, >=, >, ^).

[github-actions[bot]]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[matheusaaguiar]

You should also add some tests. If you haven't already, I would suggest reading our PR Review Checklist as well as the Contributing Guide

[r0qs]

I agree with @matheusaaguiar's comments about using ranges::find_if and doing the comparison directly (i.e. removing containPrefixingToken function) . To give you some directions about the tests, you will need to add some syntax tests at test/libsolidity/syntaxTests/pragma, see this one as example: https://github.com/ethereum/solidity/blob/develop/test/libsolidity/syntaxTests/pragma/invalid_range_conjunction_two_ranges.sol

[eduardfina]

Thank you so much for the feedback, I agree with all you said. I was thinking about including the = too, but I didn't know if it would be correct I'm new at Solidity development and I'm still understanding how it all works. I'll make the changes as soon as possible!

[NunoFilipeSantos]

It looks like this PR is still in progress, so we're converting it back to a draft.

[eduardfina]

I modified the code to match with the feedback, I had to adapt some things:

1- I deleted the function containPrefixingToken() and added an inline if to detect the prefix. 2- I evaluate the components prefix to validate before changing it to the range prefix at the function parseMatchExpression. 3- I couldn’t find an easy way to include the = to the invalid operators because at the function parseMatchComponent() the default value is Assign (=), making like if all the components without operator had an =. On the other hand I think it could not damage having the possibility of using = in a range (ex. pragma solidity =0.8.17 - 0.8.20;) 4- I left the function isPragmaOp(), we can’t use isCompareOp because it’ll wrongly compare != and == too. 5- I didn’t refactor the function isPragmaOp() to isAllowedInPragma() because the operator = is also allowed and is not part of the condition. 6- I added some tests 3 invalid and 2 valid.

[eduardfina]

Also I don't know if I should do it but I updated the code to the last develop version before the commit.

Waiting for feedback, thank you!

[cameel]

Also I don't know if I should do it but I updated the code to the last develop version before the commit.

Yeah, please keep it updated, but we use rebase for that. No need for merge commits that obscure history :)

Fixed issue: #13920

Please use the Fixes #13920 form. This way github will close the issue automatically once we merge the PR.

[NunoFilipeSantos]

Hi @eduardfina! 👋 are you planning to come back soon to this PR, or should we close it for now?

[NunoFilipeSantos]

Closing as stale.

[veniger]

I can take over this PR and issue and start working on it next week

[veniger]

I can take over this PR and issue and start working on it next week

[ekpyron]

I can take over this PR and issue and start working on it next week

@veniger are you still up for working on this on the side :-)?