Update grammar functionDefinition parser rule and rename NumberUnit lexer rule
#14066
Conversation
| /** | ||
| * Scanned but not used by any rule, i.e, disallowed. | ||
| */ | ||
| OctalNumber: '0' DecimalDigits ('.' DecimalDigits)?; |
There was a problem hiding this comment.
Technically, 8 and 9 are not valid octal numbers, but are present in DecimalDigits (at least I'd hope, I haven't looked :D). So something like 091 would not be a valid octal number in the first place, but the parser error would indicate that octal numbers are not supported, which could be a bit confusing if someone makes a typo, e.g. 091 instead of 91. I think we might have to introduce OctalDigits as well if we want to be fully correct, at least I'm assuming that's something @ekpyron or @cameel would say.
There was a problem hiding this comment.
I think we can leave it as is because solc's parser behaves the same way. I.e. 091 results in
Error: Octal numbers not allowed.
Perhaps we should change that in solc, but that's a separate matter - and it would still be an error, just a different one (what is even 091?).
In any case, we should definitely have a test case for something like 091 to make sure both the grammar and the parser at least both treat it as an error.
There was a problem hiding this comment.
Fair enough. But I can tell you what 091 isn't. An octal number :D
There was a problem hiding this comment.
We could at least add a note about this weirdness in the docstring above.
There was a problem hiding this comment.
I was mainly focused in mirroring solc parser, which seems to treat anything starting with a 0 (and not followed by .) as octal...
I am going to add the test case regarding 091.
There was a problem hiding this comment.
Also added the note in the docstring.
| /** The definition of a free function. | ||
| * | ||
| */ | ||
| freeFunctionDefinition: |
There was a problem hiding this comment.
I feel like this and the NumberUnit changes should be separate PRs, since it's a bit confusing this way, especially since the tests only cover the NumberUnit changes.
There was a problem hiding this comment.
Yeah, would not mind having those as separate PRs, though I'm also fine with one because we already have quite a few suffix-related PRs :)
There was a problem hiding this comment.
I felt that, since we already tested (and cameel reviewed) the changes in the literal suffix PR, it would be easier to just push this as a single one. Plus, not too many changes.
But, no problem splitting it, if we consider that should be done.
matheusaaguiar
force-pushed
the
update_grammar
branch
2 times, most recently
from
c30e530
to
1f45ef6
Compare
test/libsolidity/syntaxTests/literals/invalid_octal_string_no_whitespace.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good. Please squash review fixes into the original commits and we can merge.
Also, you should add a changelog entry for some of the grammar changes. The renamings and splits are just refactors but we also have some actual changes:
- a fix of the discrepancy between the grammar and our parser for numbers followed by an identifier
- grammar is now more strict for free function definitions and will no longer accept some things that are only allowed for contract functions
matheusaaguiar
force-pushed
the
update_grammar
branch
2 times, most recently
from
f379c99
to
9343b81
Compare
This comment was marked as resolved.
This comment was marked as resolved.
matheusaaguiar
force-pushed
the
update_grammar
branch
2 times, most recently
from
77f675e
to
9dac87e
Compare
|
I was not sure if the changes should go in their own section titled |
|
By the way, just FYI, you don't have to squash it into a single commit. In this case it would be even nicer to have the independent changes in separate commits. There should just be no commits that change things from other commits from the same PR. Other than that it mostly matters that each commit is a single logical change. A good rule is that if it's hard to describe the set of changes in one short sentence, it's probably doing more than one thing and could be split. |
…d number followed by identifier with no whitespace.
matheusaaguiar
force-pushed
the
update_grammar
branch
from
1015452
to
0158de6
Compare
Good to know that. I'll keep in mind for the future. Thanks. |
|
Let's leave it as is so that we can merge it :) |
While working in literal suffixes, some problematic cases not covered by tests were found and motivated this.