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

[BREAKING] Disallow trailing dot not followed by number #4172

Merged
merged 1 commit into from
May 30, 2018

Conversation

leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented May 22, 2018

Closes #3210.

The trick here is that version tokens are also read as numbers, so there it should be fine to have other stuff after a .. So if the parser sees something like 1., 1 is returned as a number token and parsing goes on with ..

@leonardoalt
Copy link
Member Author

Missing syntax tests

@axic
Copy link
Member

axic commented May 23, 2018

The trick here is that version tokens are also read as numbers, so there it should be fine to have other stuff after a .

Why should versions like 0. be valid?

@@ -768,8 +768,14 @@ Token::Value Scanner::scanNumber(char _charSeen)
scanDecimalDigits(); // optional
if (m_char == '.')
{
// A '.' has to be followed by a number.
if (!isDecimalDigit(m_source.get(1)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should also use m_source.isPastEndOfInput. To ensure it is tested, please add a test case of contract C { uint a = 2.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests to the SolidityScanner file too.

@leonardoalt
Copy link
Member Author

Versions like 0. shouldn't be valid, but what I meant were things like 0.*

@leonardoalt leonardoalt changed the base branch from 050 to develop May 28, 2018 15:49
@leonardoalt
Copy link
Member Author

Rebased

@@ -99,8 +99,6 @@ void SyntaxTestTool::printContract() const
for (auto const& error: m_test->errorList())
if (error.locationStart >= 0 && error.locationEnd >= 0)
{
assert(static_cast<size_t>(error.locationStart) <= source.length());
assert(static_cast<size_t>(error.locationEnd) <= source.length());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails if the error is in the end of the file, that is, if the contract is incomplete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still be good to have these. What are the exact numbers? This could be a bug in the parser or scanner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know what happened, but I just ran it now to get the numbers and it didn't crash 😕

@@ -99,8 +99,6 @@ void SyntaxTestTool::printContract() const
for (auto const& error: m_test->errorList())
if (error.locationStart >= 0 && error.locationEnd >= 0)
{
assert(static_cast<size_t>(error.locationStart) <= source.length());
assert(static_cast<size_t>(error.locationEnd) <= source.length());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still be good to have these. What are the exact numbers? This could be a bug in the parser or scanner.

@leonardoalt
Copy link
Member Author

Squashed and rebased.

scanner.reset(CharStream(".5"), "");
BOOST_CHECK_EQUAL(scanner.currentToken(), Token::Number);
scanner.reset(CharStream(".5e10"), "");
BOOST_CHECK_EQUAL(scanner.currentToken(), Token::Number);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these above should have another expectation for EOS

@axic axic changed the title [BREAKING][050]Disallow trailing dot not followed by number [BREAKING] Disallow trailing dot not followed by number May 30, 2018
@axic axic dismissed chriseth’s stale review May 30, 2018 14:25

Original issue is gone.

@axic
Copy link
Member

axic commented May 30, 2018

@chriseth please review

@axic
Copy link
Member

axic commented May 30, 2018

There are optimiser test failures, not sure they are just a CI issue, so restarted it.

@chriseth
Copy link
Contributor

@axic can you come to https://meet.jit.si/SolidityWeekly, please?

@chriseth chriseth merged commit 0a1a8bf into develop May 30, 2018
@axic axic deleted the trailing_dot branch June 3, 2018 23:27
Xanewok added a commit to Xanewok/slang that referenced this pull request Mar 9, 2024
See ethereum/solidity#4172.

This unblocks parsing member access expressions where the primary
expression can't consume the dot in order to parse the `.member` postfix
expression.
github-merge-queue bot pushed a commit to NomicFoundation/slang that referenced this pull request Mar 19, 2024
…it since 0.5.0 (#891)

See ethereum/solidity#4172.

This unblocks parsing member access expressions where the primary
expression can't consume the dot in order to parse the `.member` postfix
expression.
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.

3 participants