Fix Pragma: disallow prefixing operators with version ranges #13979

eduardfina · 2023-02-16T16:14:08Z

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

github-actions · 2023-02-16T16:14:22Z

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 · 2023-02-16T21:17:52Z

liblangutil/SemVerHandler.cpp

+
+bool SemVerMatchExpressionParser::containPrefixingToken() const
+{
+	if (std::find_if(m_tokens.begin(), m_tokens.end(), TokenTraits::isPragmaOp) != m_tokens.end())


Suggested change

if (std::find_if(m_tokens.begin(), m_tokens.end(), TokenTraits::isPragmaOp) != m_tokens.end())

if (find_if(m_tokens.begin(), m_tokens.end(), TokenTraits::isPragmaOp) != m_tokens.end())

We already have using namespace std. On the other hand, we could use the range-v3 library, which has find_if working with the container itself, rather than its iterators. That would make the expression more easily readable.
Also, it would be better if this method directly returned the result of the comparison of the if block. Maybe, we could not have it at all and then just use the comparison directly.

matheusaaguiar · 2023-02-16T21:25:13Z

liblangutil/Token.h

 		op == Token::Add || op == Token::Mul || op == Token::Equal || op == Token::NotEqual; }
 	constexpr bool isArithmeticOp(Token op) { return Token::Add <= op && op <= Token::Exp; }
 	constexpr bool isCompareOp(Token op) { return Token::Equal <= op && op <= Token::GreaterThanOrEqual; }
+	constexpr bool isPragmaOp(Token op) { return (Token::LessThan <= op && op <= Token::GreaterThanOrEqual) || op == Token::BitXor; }


I think that this should also include token =. Additionally, the naming of the method could be something like isAllowedInPragma or similar.

Probably, this function would never be used anywhere else... in that case, it would be better to not have it here. We could just use isCompareOp combined with the equality check for Token::BitXor as a lambda function in the local context where needed.

matheusaaguiar

I think fundamentally you are in the right direction, but I think that the current implementation does not correctly solve the problem.
I haven't checked, but I expect the proposed solution would throw an error when ranges are combined with a disjunction, for example:

pragma solidity 0.5.0 - 0.55 || >=0.7 <0.9;

That's because it is checking all of the pragma tokens (m_tokens), instead of only those in the components of the range (range.components).

matheusaaguiar · 2023-02-16T22:11:47Z

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 · 2023-02-17T05:03:34Z

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 · 2023-02-17T09:38:59Z

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 · 2023-02-21T11:09:05Z

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

Add 3 invalid tests and 2 valid tests

eduardfina · 2023-02-21T19:44:07Z

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 · 2023-02-21T19:46:05Z

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

The PR is missing a changelog entry.

cameel · 2023-02-23T16:19:54Z

liblangutil/SemVerHandler.cpp

+		if (TokenTraits::isPragmaOp(range.components[0].prefix) || TokenTraits::isPragmaOp(range.components[1].prefix))
+		{
+			solThrow(
+				SemVerError,
+				"You cannot use operators (<, <=, >=, >, ^) with version ranges (-)."
+			);
+		}


I don't think it's a good idea to check only for specific prefixes. For example currently this condition misses ~, which is also a prefix you can use here. Ranges can't have prefixes anyway so we should just error out here on any of them. I'd also make the message just that:

Components of version ranges cannot be prefixed with operators.

Might be also a good idea to show which token it was. You can use TokenTraits::friendlyName(token) for that.

How could I detect if it the range has a prefix?

At this point after calling the function parseMatchComponent() the range.components[0].prefix and range.components[1].prefix will at least have the default prefix (Assign) making impossible to know if it was assigned because it really has the prefix = or is the default one.

I think I could change it but it would modify the function parseMatchComponent(), a bigger change at the code.

On the other hand I could just add the prefix ~ at the condition.

As you decide, I await a reply! :)

Good point, but it's a separate problem. Your current code has it too. It does not discern between = and having no prefix either. It's actually wrong, which I did not notice until you pointed it out, because it will treat =0.0 - =1.0 as a valid range (and by the way, a corner case like this should definitely have test case, whichever way it works).

As for how to distinguish them, I think we should change prefix to optional<Token> and stop setting a default in parseMatchComponent(). Then handle nullopt explicitly in matches().

cameel · 2023-02-23T16:22:55Z

test/libsolidity/syntaxTests/pragma/invalid_range_operator_range_first.sol

@@ -0,0 +1,3 @@
+pragma solidity <0.8.17 - 0.8.20;
+// ----
+// ParserError 1684: (0-33): Invalid version pragma. You cannot use operators (<, <=, >=, >, ^) with version ranges (-).


Please configure your editor to ensure that there is always a newline at the end of the file (github's diff is showing that one is missing here and in some other files). Makes for less random noise in diffs.

cameel · 2023-02-23T16:25:27Z

test/libsolidity/syntaxTests/pragma/invalid_range_operator_range_first.sol

@@ -0,0 +1,3 @@
+pragma solidity <0.8.17 - 0.8.20;


There are not that many options to test (>, >=, <, <=, =, ^, ~, no operator) so it would be good to test them all, at both positions.

You can check multiple cases at the same time by using a multi-file test. E.g.:

==== Source: A ==== pragma solidity <0.8.17 - 0.8.20; ==== Source: B ==== pragma solidity <=0.8.17 - 0.8.20; ==== Source: C ==== pragma solidity 0.8.17 - >0.8.20;

cameel · 2023-02-23T16:27:37Z

test/libsolidity/syntaxTests/pragma/invalid_range_operator_range_second.sol

@@ -0,0 +1,3 @@
+pragma solidity 0.8.17 - >0.8.20;


Please also add a case without whitespace or with shorter versions. This sometimes makes a difference. For example 0.0 - =1.0 was valid, while 0.0-=1.0 was not. We should make sure that both cases are handled.

cameel · 2023-02-23T16:30:44Z

test/libsolidity/syntaxTests/pragma/valid_range_operator_two_ranges.sol

@@ -0,0 +1,2 @@
+pragma solidity <0.8.15 || 0.8.17 - 0.8.20;


This test uses a pragma with only one range, but the name says it has two. It should be called something like valid_range_operator_range_second_prefixed_version.sol.

cameel · 2023-02-23T16:31:44Z

test/libsolidity/syntaxTests/pragma/invalid_range_operator_range_second.sol

@@ -0,0 +1,3 @@
+pragma solidity 0.8.17 - >0.8.20;


Please add a case where the range is prefixed with something that's not one of the currently allowed operators. The PR does not change that case but I don't think we have that case already covered.

cameel · 2023-02-23T16:35:27Z

test/libsolidity/syntaxTests/pragma/valid_range_operator_two_ranges.sol

@@ -0,0 +1,2 @@
+pragma solidity <0.8.15 || 0.8.17 - 0.8.20;


We should also have a case where the prefix is next to || to make sure it is not handled differently:

pragma solidity 0.8.15 || <0.8.17 - 0.8.20;

cameel · 2023-02-23T16:39:37Z

test/libsolidity/syntaxTests/pragma/valid_range_operator.sol

@@ -0,0 +1,2 @@
+pragma solidity 0.8.17 - 0.8.20;


For positive cases please use a wider range of versions. This one will break in 0.8.21.

Other tests we already have use at least 0.9.0, but I think it would not hurt to use something much higher so that updating these tests is less annoying when we reach a breaking release.

cameel · 2023-02-23T16:41:32Z

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 · 2023-04-04T16:46:40Z

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

NunoFilipeSantos · 2023-07-24T11:35:33Z

Closing as stale.

veniger · 2023-07-24T12:37:16Z

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

veniger · 2023-07-26T08:41:47Z

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

ekpyron · 2023-12-13T14:40:10Z

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 :-)?

Fixed Prefixing Version with Range

82e27dc

github-actions bot added the external contribution ⭐ label Feb 16, 2023

Fixed spelling and coding style errors

3a4a504

cameel requested review from matheusaaguiar and r0qs February 16, 2023 18:04

matheusaaguiar reviewed Feb 16, 2023

View reviewed changes

NunoFilipeSantos marked this pull request as draft February 21, 2023 11:09

eduardfina added 2 commits February 21, 2023 20:20

Looking pragma operators per range and not per tokens.

ed7123c

Add 3 invalid tests and 2 valid tests

Merge branch 'develop' into FixPrefixVersionRangesDevelop

ea9a773

eduardfina marked this pull request as ready for review February 21, 2023 19:46

cameel requested changes Feb 23, 2023

View reviewed changes

cameel reviewed Feb 23, 2023

View reviewed changes

cameel marked this pull request as draft March 6, 2023 10:24

cameel mentioned this pull request Apr 4, 2023

Version pragma does not disallow prefixing version ranges with comparison operators #13920

Open

cameel added the takeover Can be taken over by someone other than label giver label Apr 17, 2023

NunoFilipeSantos closed this Jul 24, 2023

nikola-matic reopened this Jul 24, 2023

nikola-matic self-assigned this Jul 24, 2023

nikola-matic removed their assignment Jul 24, 2023

nikola-matic assigned veniger Jul 24, 2023

nikola-matic removed the takeover Can be taken over by someone other than label giver label Jul 24, 2023

	if (std::find_if(m_tokens.begin(), m_tokens.end(), TokenTraits::isPragmaOp) != m_tokens.end())
	if (find_if(m_tokens.begin(), m_tokens.end(), TokenTraits::isPragmaOp) != m_tokens.end())

Fix Pragma: disallow prefixing operators with version ranges #13979

Are you sure you want to change the base?

Fix Pragma: disallow prefixing operators with version ranges #13979

Uh oh!

Conversation

eduardfina commented Feb 16, 2023 • edited by cameel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 16, 2023

Uh oh!

matheusaaguiar Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusaaguiar Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusaaguiar Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusaaguiar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusaaguiar commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r0qs commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduardfina commented Feb 17, 2023

Uh oh!

NunoFilipeSantos commented Feb 21, 2023

Uh oh!

eduardfina commented Feb 21, 2023

Uh oh!

eduardfina commented Feb 21, 2023

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardfina Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

cameel Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

cameel commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

eduardfina commented Feb 16, 2023 •

edited by cameel

Loading

matheusaaguiar Feb 16, 2023 •

edited

Loading

matheusaaguiar Feb 16, 2023 •

edited

Loading

matheusaaguiar Feb 16, 2023 •

edited

Loading

matheusaaguiar left a comment •

edited

Loading

matheusaaguiar commented Feb 16, 2023 •

edited

Loading

r0qs commented Feb 17, 2023 •

edited

Loading

cameel Feb 23, 2023 •

edited

Loading

cameel Feb 23, 2023 •

edited

Loading

cameel Feb 23, 2023 •

edited

Loading

cameel Feb 23, 2023 •

edited

Loading

cameel Feb 23, 2023 •

edited

Loading

cameel commented Feb 23, 2023 •

edited

Loading