Support rational constants

seanyoung commented 4 years ago

In Solidity, rationals are supported as long as they are compile-time constants. e.g. this is supported:

uint x = .5 * 8;

Also, fractions can be written with scientific notation: 1e-2 or 0.001e2.

Fractions can be added/subtracted at compiler time, but fractions cannot be added to integers, like:

uint x = 1;
uint y = x + 0.1;

This change requires:

Changes to the lexer to support lexing fractions
Add new type to AST Type::Rational
Add funtion to evaluate expression of rationals at compile time
Implicit casting should allow round rationals convert to integers
Tests for this

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 400.0 DAI (400.0 USD @ $1.0/DAI) attached to it.

If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
- Want to chip in? Add your own contribution here.
- Questions? Checkout Gitcoin Help or the Gitcoin Chat
- $841,395.19 more funded OSS Work available on the Gitcoin Issue Explorer

KiChjang commented 4 years ago

@W3F It looks like gitcoin's github integration bot is malfunctioning again, and didn't seem to post the bounty applicants to this issue and #282. Do you mind taking a look at it?

Web3Foundation commented 3 years ago

Ha! @KiChjang you are correct, the bot did not post to the issue. @seanyoung could you have a look at the two applicants and let us know who to approve: https://gitcoin.co/issue/hyperledger-labs/solang/284/100023827

seanyoung commented 3 years ago

@Web3Foundation please approve @janus thank you

KiChjang commented 3 years ago

@janus Are you still working on this bounty, or is it ok for others to work on it?

janus commented 3 years ago

@janus Are you still working on this bounty, or is it ok for others to work on it?

I didn't receive 'approval' message. Please approve it then.

janus commented 3 years ago

@Web3Foundation Please approve me.

gitcoinbot commented 3 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 265 years, 9 months from now. Please review their action plans below:

1) kichjang has applied to start work _(Funders only: approve worker | reject worker)_.

First, the compiler frontend will need to be extended so that it supports the regex for rationals. Next, a new AST enum variant for rationals will be created, and generated appropriately when the lexer finds a rational token. After that, codegen needs to evaluate and generate the appropriate LLVM bytecode to support rational arithmetic. Finally, we'll need tests to exercise the entire process and make sure rounded rationals gets implicitly cast into integers. 2) janus has been approved to start work.

I am currently participating in a language development. I have done a few toys language development works in the paste. I also participated in a language called Citrine. I like challenges and this task would lead me to study the core codebase of your language and from there participate in future development. The task as stated involved tweaking the tokenizer(assuming it is not already there) , and adding Rational Object and include its evaluation and casting.

I have working knowledge of C and Rust languages.

Learn more on the Gitcoin Issue Details page.

Web3Foundation commented 3 years ago

done @janus

janus commented 3 years ago

done @janus

Thanks

gitcoinbot commented 3 years ago

@janus Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] reminder (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 3 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@janus due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

janus commented 3 years ago

@gitcoinbot This is actively worked on....update coming soon.

gitcoinbot commented 3 years ago

@janus Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] reminder (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

janus commented 3 years ago

@seanyoung , @Web3Foundation , @KiChjang

I got the below error message with this command: cargo test. I have removed my cache.

error: failed to run custom build command for `llvm-sys v100.2.0`

Caused by:
  process didn't exit successfully: `/home/kaka/buildtool/job/amhello/gitcoin/solang/target/debug/build/llvm-sys-d6938d4c65a04c4e/build-script-build` (exit code: 101)
--- stdout
cargo:rerun-if-env-changed=LLVM_SYS_100_PREFIX
cargo:rerun-if-env-changed=LLVM_SYS_100_IGNORE_BLACKLIST
cargo:rerun-if-env-changed=LLVM_SYS_100_STRICT_VERSIONING
cargo:rerun-if-env-changed=LLVM_SYS_100_NO_CLEAN_CFLAGS
cargo:rerun-if-env-changed=LLVM_SYS_100_USE_DEBUG_MSVCRT
cargo:rerun-if-env-changed=LLVM_SYS_100_FFI_WORKAROUND

--- stderr
thread 'main' panicked at 'Failed to search PATH for llvm-config: Permission denied (os error 13)', /home/kaka/.cargo/registry/src/github.com-1ecc6299db9ec823/llvm-sys-100.2.0/build.rs:121:23
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

warning: build failed, waiting for other jobs to finish...
error: build failed

janus commented 3 years ago

@seanyoung , @Web3Foundation , @KiChjang

I got the below error message with this command: cargo test. I have removed my cache.

error: failed to run custom build command for `llvm-sys v100.2.0`

Caused by:
  process didn't exit successfully: `/home/kaka/buildtool/job/amhello/gitcoin/solang/target/debug/build/llvm-sys-d6938d4c65a04c4e/build-script-build` (exit code: 101)
--- stdout
cargo:rerun-if-env-changed=LLVM_SYS_100_PREFIX
cargo:rerun-if-env-changed=LLVM_SYS_100_IGNORE_BLACKLIST
cargo:rerun-if-env-changed=LLVM_SYS_100_STRICT_VERSIONING
cargo:rerun-if-env-changed=LLVM_SYS_100_NO_CLEAN_CFLAGS
cargo:rerun-if-env-changed=LLVM_SYS_100_USE_DEBUG_MSVCRT
cargo:rerun-if-env-changed=LLVM_SYS_100_FFI_WORKAROUND

--- stderr
thread 'main' panicked at 'Failed to search PATH for llvm-config: Permission denied (os error 13)', /home/kaka/.cargo/registry/src/github.com-1ecc6299db9ec823/llvm-sys-100.2.0/build.rs:121:23
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

warning: build failed, waiting for other jobs to finish...
error: build failed

Discard the above I have figured it out

janus commented 3 years ago

@seanyoung Please go through my code and comment. I would want to know if I followed your convention. WIP, https://github.com/hyperledger-labs/solang/pull/384

seanyoung commented 3 years ago

@janus your code looks fine, thank you.

You do need a "Signed-off-by:" in your commit message.

gitcoinbot commented 3 years ago

@janus Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] reminder (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Web3Foundation commented 3 years ago

@seanyoung Can we make a payout to @janus ?

janus commented 3 years ago

Not yet...I was unwell... I would add up more features this week please

On Wed, 17 Feb 2021, 09:58 W3F, notifications@github.com wrote:

@seanyoung https://github.com/seanyoung Can we make a payout to @janus https://github.com/janus ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hyperledger-labs/solang/issues/284#issuecomment-780406833, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABURWLYYRYBPDMXTZXG33S7OAKVANCNFSM4RTZ22JA .

janus commented 3 years ago

This issue is not really out: sudo chown -R $(whoami) /home/kaka/.cargo/ This resolved the issue before but not now. I also noticed this: sudo cargo build error: no override and no default toolchain set

aused by:
  process didn't exit successfully: `/home/kaka/buildtool/job/amhello/gitcoin/solang/target/debug/build/llvm-sys-34f8483076ad8ea7/build-script-build` (exit code: 101)
  --- stdout
  cargo:rerun-if-env-changed=LLVM_SYS_100_PREFIX
  cargo:rerun-if-env-changed=LLVM_SYS_100_IGNORE_BLACKLIST
  cargo:rerun-if-env-changed=LLVM_SYS_100_STRICT_VERSIONING
  cargo:rerun-if-env-changed=LLVM_SYS_100_NO_CLEAN_CFLAGS
  cargo:rerun-if-env-changed=LLVM_SYS_100_USE_DEBUG_MSVCRT
  cargo:rerun-if-env-changed=LLVM_SYS_100_FFI_WORKAROUND

  --- stderr
  thread 'main' panicked at 'Failed to search PATH for llvm-config: Permission denied (os error 13)', /home/kaka/.cargo/registry/src/github.com-1ecc6299db9ec823/llvm-sys-100.2.0/build.rs:121:23
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

Is there a permanent solution to this? I need help

seanyoung commented 3 years ago

@janus you need to install the llvm libraries as per the documentation: https://solang.readthedocs.io/en/latest/installing.html#installing-llvm-on-linux

error: no override and no default toolchain set is an issue with rustup. Run rustup default stable

janus commented 3 years ago

@seanyoung , I have added unit test (lexer) for decimal. Please, check if it is exhaustive.

janus commented 3 years ago

@janus you need to install the llvm libraries as per the documentation: https://solang.readthedocs.io/en/latest/installing.html#installing-llvm-on-linux

error: no override and no default toolchain set is an issue with rustup. Run rustup default stable

Thanks. I erroneously deleted llvm file.

seanyoung commented 3 years ago

@seanyoung , I have added unit test (lexer) for decimal. Please, check if it is exhaustive.

The lexer looks complete, but I haven't tested it.

janus commented 3 years ago

@seanyoung From the example above , it would only be unsigned, is that correct?

janus commented 3 years ago

@seanyoung , Is solidity.lalrprop machine generated? If yes, how do I have to add Rational token to it? How is solidity.rs related to the parser? I noticed that Type and Expression enums are defined in both pt.rs and ast.rs.

Where is tokens consumed?

seanyoung commented 3 years ago

@janus the lexer only lexes positive values, and lexes the minus as a separate token. The resolver (sema) combines the two.

solidity.lalrpop is an LR grammar, very much handwritten. The solidity.rs is generated from this file. See http://lalrpop.github.io/lalrpop/

gitcoinbot commented 3 years ago

@janus Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] reminder (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

janus commented 3 years ago

@seanyoung I opted to use bigdecimal::BigDecimal. I could also use bigInt for numerator , bigInt for denominator. But I feel that BigDecimal is a natural fit here. Please review my update and also solidity.lalrpop. Which command do you use to convert lalrpop file to rs?

seanyoung commented 3 years ago

@janus cargo build will generate src/lexer/solidity.rs if src/lexer/solidity.lalrpop has changed.

gitcoinbot commented 3 years ago

@janus Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] reminder (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

janus commented 3 years ago

@seanyoung Please check out bigfloat_to_expression function. It is a bit sloppy. I need your input.

gitcoinbot commented 3 years ago

@janus Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] reminder (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 3 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@janus due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Web3Foundation commented 3 years ago

@seanyoung Can we make a payout to @janus? Or what is the status with this?

seanyoung commented 3 years ago

@Web3Foundation the work is far from complete, unfortunately

janus commented 3 years ago

@seanyoung I asked for little direction but no response. I am ready to get back if you would just support me by way of answering one or two questions. Thsnks

seanyoung commented 3 years ago

@janus sorry about that, I missed that.

So I think you're on the right track. Only nitpicks I have is:

bigfloat_to_expression isn't a great name; might as well call it bigdecimal_to_expression
For rational numbers, there is no size limit. A limit only kicks if it is cast to a uint256 or uint32.
Possibly it is better represented as actual rationals like with the rational crate. This is up to you.

I'll keep an eye out so I don't miss anything again!

seanyoung commented 3 years ago

@janus please let me know if there is anything else I can do for you

janus commented 3 years ago

@seanyoung Thanks so much. I would get back to coding and if I have issues along the way I would get back to you.

janus commented 3 years ago

@seanyoung Should Rational be classified as primitive? That is returning true for is_primitive. I have a nasty error with num_rational, I have contacted the team.

seanyoung commented 3 years ago

@janus yes, a rational is a primitive in Solidity.

Out of interest, what is the issue you are having with num_rational?

janus commented 3 years ago

@seanyoung Thanks for the quick response.

There may be something out there I am not following.

 BigRational::new(BigInt::from_str(&significand).unwrap(), denominator.mul(exp))
      |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `num_bigint::bigint::BigInt`, found struct `BigInt`
      |
      = note: perhaps two different versions of crate `num_bigint` are being used?

seanyoung commented 3 years ago

I think the note: is likely the point to the issue. You may have a different version num_bigint in your Cargo.toml which differs what the version that num_bigint depends on.

janus commented 3 years ago

I think the note: is likely the point to the issue. You may have a different version num_bigint in your Cargo.toml which differs what the version that num_bigint depends on.

Thanks. I would investigate.

janus commented 3 years ago

@seanyoung

I got the below. It is strange to me.

$ cargo --version cargo 1.50.0 (f04e7fab7 2021-02-04)

$ rustc --version rustc 1.50.0 (cb75ad5db 2021-02-10)

error[E0658]: or-patterns syntax is experimental
    --> src/sema/contracts.rs:1043:13
     |
1043 |             pt::Visibility::Public(_) | pt::Visibility::External(_),
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information

error[E0658]: or-patterns syntax is experimental
    --> src/sema/contracts.rs:1044:13
     |
1044 |             pt::Visibility::Public(_) | pt::Visibility::External(_)
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information

error[E0658]: or-patterns syntax is experimental
    --> src/sema/expression.rs:1329:43
     |
1329 |             | (Mutability::Nonpayable(_), Mutability::Nonpayable(_) | Mutability::Payable(_))
     |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information

error[E0658]: or-patterns syntax is experimental
    --> src/sema/expression.rs:1331:37
     |
1331 |             | (Mutability::View(_), Mutability::View(_) | Mutability::Nonpayable(_) | Mutability::Payable(_))
     |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information

seanyoung commented 3 years ago

@janus you need to upgrade your rust to at least 1.53.0, sorry about that.

janus commented 3 years ago

@seanyoung Thanks so much. It worked Please review my code. And point out file I would work on next.

hyperledger-solang / solang

Support rational constants #284