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

Add flag to reverse errors #4142

Merged
merged 40 commits into from
Jun 21, 2023

Conversation

gr00vytvniks
Copy link
Contributor

closes #3860

@gr00vytvniks
Copy link
Contributor Author

hey @kayagokalp I made the changes you requested, I tried running forc build after adding some errors to examples/counter and I got this error.

error
  --> /Users/mitchmackert/Code/sway/sway-lib-core/src/ops.sw:11:9
   |
 9 | 
10 |     fn add(self, other: Self) -> Self {
11 |         __add(self, other)
   |         ^^^^^ Identifiers cannot begin with a double underscore, as that naming convention is reserved for compiler intrinsics.
12 |     }
13 | }
   |
____

would you mind giving me steps to test this correctly? I think I am looking at this the wrong way

@kayagokalp
Copy link
Member

kayagokalp commented Feb 22, 2023

hey @gr00vytvniks thanks for the PR, the error looks odd. Can you make a rebase/merge master as you shouldn't have any lock file is the counter folder as we recently changed that folder to be a workspace, see. This type of core compilation errors can be seen with master forc depending on a released version of std/core. Can you make sure that in examples/counter/Forc.toml you actually depend on local std. You should have something like this in the Forc.toml:

[dependencies]
std = { path = "../../sway-lib-std" }

You should be able to test this with a template project by adding a couple of compile errors. Let me know if you are unsure/stuck 👍

@gr00vytvniks
Copy link
Contributor Author

hey @gr00vytvniks thanks for the PR, the error looks odd. Can you make a rebase/merge master as you shouldn't have any lock file is the counter folder as we recently changed that folder to be a workspace, see. This type of core compilation errors can be seen with master forc depending on a released version of std/core. Can you make sure that in examples/counter/Forc.toml you actually depend on local std. You should have something like this in the Forc.toml:

[dependencies]
std = { path = "../../sway-lib-std" }

You should be able to test this with a template project by adding a couple of compile errors. Let me know if you are unsure/stuck 👍

Hey @kayagokalp, The error came from a collision between an old fuelup binary and a cargo binary.

@gr00vytvniks
Copy link
Contributor Author

gr00vytvniks commented Feb 25, 2023

Hey @kayagokalp, I added a debug build profile like the book instructed but it is giving me this warning.

 ~/Code/sway/examples/counter/ [gr00vytvniks/reverse-errors*] forc build
  WARNING! unused manifest key: build-profiles

@gr00vytvniks
Copy link
Contributor Author

gr00vytvniks commented Feb 25, 2023

I went ahead and just changed the default in debug() to true and this was the output :) Should we leave the default as true, or leave it up to the user?

error
  --> /Users/mitchmackert/Code/sway/examples/counter/src/main.sw:23:42
   |
21 | 
22 |     #[storage(read, write)]
23 |     fn increment_counter(amount: u64) -> u8 {
   |                                          ^^ expected: u64 
found:    u8 
help:     The definition of this function must match the one in the ABI "TestContract" declaration.
24 |         let incremented = storage.counter + amount;
25 |         storage.counter = incremented;
   |
____

error
  --> /Users/mitchmackert/Code/sway/examples/counter/src/main.sw:17:42
   |
15 | 
16 |     #[storage(write)]
17 |     fn initialize_counter(value: u64) -> u32 {
   |                                          ^^^ expected: u64 
found:    u32 
help:     The definition of this function must match the one in the ABI "TestContract" declaration.
18 |         storage.counter = value;
19 |         value
   |
____

  Aborting due to 2 errors.
Error: Failed to compile counter

@gr00vytvniks gr00vytvniks marked this pull request as ready for review February 25, 2023 23:29
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

We should not revert it in any build profile imo. This is not something everyone would be using it is more niche command that some people may end-up using.

So we should not revert by default as it would be unexpected for the users. We should be able to use this feature with the flag forc build --reverse-errors etc.

examples/counter/Forc.lock Outdated Show resolved Hide resolved
@kayagokalp kayagokalp requested review from nfurfaro and a team March 15, 2023 15:15
nfurfaro
nfurfaro previously approved these changes Mar 15, 2023
Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

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

Thanks!

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-util/src/lib.rs Outdated Show resolved Hide resolved
@eureka-cpu
Copy link
Contributor

Looks like there's a problem with the formatting of the examples/counter/Forc.toml file. Also some of the should_fail e2e tests are failing incorrectly (not sure if it's related, may just need to re-run the job).

@eureka-cpu
Copy link
Contributor

Happy to approve this once merge conflicts are resolved and @kayagokalp gives his approval

@eureka-cpu
Copy link
Contributor

As a follow up PR, you could update the documentation so that developers will know how to use this feature. ie as part of a BuildProfile or just via --reverse-order(Not sure what name you guys landed on).

Copy link
Contributor

@eureka-cpu eureka-cpu left a comment

Choose a reason for hiding this comment

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

Haven't tested yet but there's one thing that seems out of place if you wouldn't mind addressing it. After that I'll pull this branch and test it out!

forc-pkg/src/manifest.rs Outdated Show resolved Hide resolved
forc-pkg/src/manifest.rs Outdated Show resolved Hide resolved
forc-pkg/src/manifest.rs Outdated Show resolved Hide resolved
eureka-cpu
eureka-cpu previously approved these changes Jun 8, 2023
Copy link
Contributor

@eureka-cpu eureka-cpu left a comment

Choose a reason for hiding this comment

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

🚢

@JoshuaBatty
Copy link
Member

Thanks for this. just some last merge conflicts @gr00vytvniks

@eureka-cpu eureka-cpu enabled auto-merge (squash) June 21, 2023 20:46
@eureka-cpu eureka-cpu merged commit 00982e4 into FuelLabs:master Jun 21, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to forc to reverse the order of printed errors
5 participants