Closed duaraghav8 closed 7 years ago
@duaraghav8 Looks great to me. I've been meaning to get started on this.
Sorry for the late reply - got back from devcon and have been getting over the timezone change.
Interesting that ^
means not upward compatible. Seems opposite of npm versioning, IIRC.
Otherwise, looks great!
oh no problem, I'm sure you had fun! yeah, even I got puzzled because of the opposite use initially. Cool! I'm almost done with this, will submit the PR soon:)
Hi, anyone is working on this? I can contribute if needed.
Regarding the proposed AST Node I think the upward_compatible boolean field is wrong.
The documentation specify that the version follow the same rules of npm, so the expression ^0.4.0 means compatible with all 0.4.x versions, the expression ^1.0 means compatible with all 1.x.y, and also expressions like ~0.4.0
and >= 0.4.0
are supported, but also more complex expressions like >= 0.4.0 < 0.4.4
.
@mpolci Yes, I've implemented it (but not yet made a PR since I was testing my code) but if you have already written the code then please go ahead
Also, quoting you
^0.4.0 means compatible with all 0.4.x versions
- This I already understand
^0.4 means compatible with all 0.x.y, and also expressions like ~0.4.0 and >= 0.4.0 are supported
- This I didn't account for.
So thanks for pointing this out!
@duaraghav8 I'd love to help test this. Seems a lot of people are asking for this feature. Let me know if there's anything I can do.
@tcoulter thanks! I'll point you to my fork once I'm done implementing changes pointed out by @mpolci
@duaraghav8 What are the chances this can be finished early this week? I'm out Thursday onward to get married ( 🎉 ) and then I'm out for a two week honeymoon. I'd like to get people on solc 4.0 if I can, and I believe this is the only showstopper.
If you're booked for this week, do you have a branch I can work off of? Happy to try and pick up where you left off, or create an interim version that supports 90% of cases that'll get people compiling that we can polish later.
Hey congrats man! I've been travelling lately, so haven't been able to do much. Follow this link: https://github.com/duaraghav8/solparse/blob/master/solidity-parser.pegjs#L1267
(this fork is not entirely in sync with solidity-parser, some additional, untested features also exist. So just pay attention to the pragma statement parsing code)
also, just Ctrl+F "Pragma" and you'll see all the places where pragma statement-related code has been written
Right now, it only supports 2 types of statements:
pragma solidity ^0.4.0;
pragma solidity 0.4.0;
but given what we've been discussing, it requires us to fully understand how NPM versioning works and then write the parseing code
Thanks @duaraghav8! I'll take a look and see what I can do.
As far as Truffle is concerned, I'm going to do the bare minimum to get people compiling on 0.4.x, as there's there's huge interest there. We can patch things up later.
Regarding this statement:
it requires us to fully understand how NPM versioning works and then write the parseing code
I thought it worked differently than NPM's semver. Is there any documentation on how it's supposed to work?
@tcoulter yeah therein lies the confusion. The solidity pragma doc hasn't explicitly stated all the rules, says they're like NPM. However, the cap "^" is a special case where its the opposite of NPM's spec.
This is my interpretation of what's written in the doc. Maybe I'm wrong, please read and let me know?
I made a test with solc using the expressions I quoted in the previous post. They are all recognized.
Il 10 ott 2016 8:10 PM, "Raghav Dua" notifications@github.com ha scritto:
@tcoulter https://github.com/tcoulter yeah therein lies the confusion. The solidity pragma doc https://solidity.readthedocs.io/en/develop/layout-of-source-files.html#version-pragma hasn't explicitly stated all the rules, says they're like NPM. However, the cap "^" is a special case where its the opposite of NPM's spec.
This is my interpretation of what's written in the doc. Maybe I'm wrong, please read and let me know?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ConsenSys/solidity-parser/issues/13#issuecomment-252697289, or mute the thread https://github.com/notifications/unsubscribe-auth/AGUYT0cL3Ex3ukjzvw1WLc1JFJv4bvGIks5qyn96gaJpZM4KFkcK .
@mpolci Thanks for that.
@duaraghav8 I don't know any more than you do, but we can at least make a best guess. I'll edit the grammar to allow all prefix characters that NPM supports, just so we can get solidity-parser to stop erroring on the pragma
statement and let the compiler error instead. When we learn more we can then tighten up what's allowed.
As far as the AST is concerned, I'll have it report the prefix characters that were used, and allow anyone reading the AST to interpret their meaning. Once we understand the meaning ourselves we can then change the AST to better represent each available usage.
Is it required to have a semantic interpretation in the parser? The solidity grammar allow any expression https://github.com/ethereum/solidity/blob/develop/libsolidity/grammar.txt
I think the parser could accept any expression without interpretation if it isn't required for something else.
Il 10 ott 2016 8:23 PM, "Tim Coulter" notifications@github.com ha scritto:
@mpolci https://github.com/mpolci Thanks for that.
@duaraghav8 https://github.com/duaraghav8 I don't know any more than you do, but we can at least make a best guess. I'll edit the grammar to allow all prefix characters that NPM supports, just so we can get solidity-parser to stop erroring on the pragma statement and let the compiler error instead. When we learn more we can then tighten up what's allowed.
As far as the AST is concerned, I'll have it report the prefix characters that were used, and allow anyone reading the AST to interpret their meaning. Once we understand the meaning ourselves we can then change the AST to better represent each available usage.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ConsenSys/solidity-parser/issues/13#issuecomment-252700278, or mute the thread https://github.com/notifications/unsubscribe-auth/AGUYT7bIIxtv8kme0RmMyb3hOhzSa8E-ks5qyoKMgaJpZM4KFkcK .
Technically no. But the goal of the solidity-parser should be a consistent AST that's easy to use.
Thanks @mpolci @tcoulter sounds like a good plan! Should we also ask a Solidity core team member to read this thread? Maybe they'll clear the air once and for all?
@duaraghav8 Would love for them to read this! I'm definitely taking a littlest-information/quickest approach, so if we could get something better and more definitive I would love that!
Just added this commit: https://github.com/ConsenSys/solidity-parser/commit/9a2ab71d0794738f51515d2f022903a720befd0c
Tests are passing with these pragma statements: https://github.com/ConsenSys/solidity-parser/blob/master/test/doc_examples.sol#L4
I decided not to support version ranges. We can add support for ranges when people actually have a desire to use those later on.
Note that this is very permissive, as RelationalOperator
and EqualityOperator
allow for more operators than what's accepted by solc (I assume). We can tighten this up later as well.
PragmaStatement
AST looks like this:
{
type: "PragmaStatement",
version: version, // VersionLiteral, see below
start: location().start.offset,
end: location().end.offset
}
Note that VersionLiteral
has been revamped as well to include an operator. e.g.:
{
type: "VersionLiteral",
operator: ">=",
version: "x.x.x",
}
Give this a look over and let me know what you think. Remember, the goal is to be very permissive and get strict later as we gain more information. If it looks good I'll commit and push a new version tomorrow.
Oh, I also updated the imports parser to support pragma statements, but it simply ignores them as the point is to return only import statements.
@tcoulter yep go ahead!
Quick update:
It looks like the solidity dev team have advertised range inputs to the pragma statement. This means we need to support it.
I've added support for range inputs here: https://github.com/ConsenSys/solidity-parser/commit/66573961489541d5a1562458a86d2737ccada601
This changes the pragma statement's AST to the following:
{
type: "PragmaStatement",
start_version: start_version, // VersionLiteral, required,
end_version: end_version, // VersionLIteral, optional
start: location().start.offset,
end: location().end.offset
}
I'll be pushing out a new release once I've tested everything with Truffle.
FYI: Also added support for hex string literals:
hex"ab1248fe"
New version published: v0.1.1
.
I'm going to close this ticket. We can open new tickets and pull requests when we want to tighten up the allowed syntax. Thanks guys!
There's a new feature in Solidity - pragma statements
The statement looks like:
Proposed AST Node for the above:
upward_compatible
is false because of the presence of the cap '^' (as mentioned in the doc) which means the contract is not even compatible with v0.5.0 for example.It will be true if no cap is set, like:
@tcoulter thoughts?