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

feat: forc-test predicate logging #6996

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

zees-dev
Copy link
Contributor

@zees-dev zees-dev commented Mar 5, 2025

Description

This PR adds support for logging in predicates via the use of the ECAL opcode.
In the ECAL implementation we simply replicate the functionality of the LOGD instruction - but for predicates.

Notes:

  • To make this work we needed to create and propagate the enable_predicate_logs attribute in the BuildProfile, BuildConfig, and Context structs; this needed to be propagated down deep into the ir_generation so that the __log compiler intrinsic could branch based on the enable_predicate_logs attribute and the predicate program Kind.
  • With this change; the forc test --logs and forc test --raw-logs commands will now output logs for predicates.
  • forc build will compile the predicate (which contains logs) in debug mode successfully
  • forc build will NOT compile the predicate (which contains logs) in release mode
  • scripts, contracts, libraries should remain unaffected by this change

Example

Predicate

predicate;

pub struct CallParams {
    pub coins: u64,
    pub gas: u64,
}

fn main() -> bool {
    let call_params = CallParams {
        coins: 7,
        gas: 8,
    };
    log(call_params);
    false
}

#[test]
fn test_meaning_of_life() {
    log("test_meaning_of_life");
    assert(main() == false);
}

> forc test --logs

Screenshot 2025-03-06 at 11 02 55 AM

> forc test --raw-logs

Screenshot 2025-03-06 at 11 03 34 AM

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@zees-dev zees-dev requested review from a team as code owners March 5, 2025 22:08
@zees-dev zees-dev self-assigned this Mar 5, 2025
@zees-dev zees-dev added forc forc-test Everything related to the `forc-test` lib and `forc test` command. team:tooling Tooling Team compiler: ir IRgen and sway-ir including optimization passes compiler General compiler. Should eventually become more specific as the issue is triaged language feature Core language features visible to end users labels Mar 5, 2025
Copy link

codspeed-hq bot commented Mar 5, 2025

CodSpeed Performance Report

Merging #6996 will not alter performance

Comparing feat/forc-test-predicate-logging (4ae3ab1) with master (d46f08a)

Summary

✅ 22 untouched benchmarks

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.

Nice one! This looks good to me in general we should probably make log_generation flag true in debug build profile by default which i left as a comment as well.

Also we should consider renaming the this flag in my opinion. Because logs are already being generated for other program kinds other than predicates without this flag. And users might end up mixing this with --logs in forc-test flags. enable_predicate_logs or something like this might be more on point (while being a little verbose, maybe we can find a common ground between this and enable_logs)

@zees-dev
Copy link
Contributor Author

zees-dev commented Mar 5, 2025

Nice one! This looks good to me in general we should probably make log_generation flag true in debug build profile by default which i left as a comment as well.

Also we should consider renaming the this flag in my opinion. Because logs are already being generated for other program kinds other than predicates without this flag. And users might end up mixing this with --logs in forc-test flags. enable_predicate_logs or something like this might be more on point (while being a little verbose, maybe we can find a common ground between this and enable_logs)

Agreed; renamed: 3ec072f

@kayagokalp
Copy link
Member

I think this should be a breaking change? by default logs in predicates were a compile error but if the user does not pass in --release it will compile fine now.

What do you think @FuelLabs/sway-compiler? Are you guys fine with enable_predicate_logs being on by default in debug builds but not in release builds?

Copy link
Contributor

@alfiedotwtf alfiedotwtf left a comment

Choose a reason for hiding this comment

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

Apart from my changes and question, looks good.

alfiedotwtf
alfiedotwtf previously approved these changes Mar 6, 2025
alfiedotwtf
alfiedotwtf previously approved these changes Mar 6, 2025
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.

I think we need the docs updated before merging with the build profile flag (enable predicate logging) and general limitations around predicates and logging such as:

  1. https://docs.fuel.network/guides/intro-to-predicates/debugging-with-scripts/#debugging-with-scripts
  2. https://docs.fuel.network/docs/forc/manifest_reference/#the-build-profile-section

For first one we might need to contact @calldelegation to see where these docs are hosted.

@zees-dev zees-dev force-pushed the feat/forc-test-predicate-logging branch from 85f7f72 to 64d5b69 Compare March 7, 2025 01:49
@zees-dev
Copy link
Contributor Author

zees-dev commented Mar 7, 2025

Should we have a test for a project that has logging triggered from a sway unit test?

addressed: 64d5b69

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

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.

Nice one!

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

Yeap release mode will fail to compile (throws typical error - Using intrinsic "log" in a predicate is not allowed.). You can explicitly override this though (even in release mode):

[build-profile.release]
enable-predicate-logs = true

Okay, this needs to be clearer in my opinion, we should have the error direct the user to a solution (either removing the logs or turning on this option) and making it clear what's happening here.

At the least the error should be updated to tell the truth (that it's allowed under certain circumstances).

@IGI-111
Copy link
Contributor

IGI-111 commented Mar 7, 2025

I also think we should discuss eliding log predicate invocation in release builds by default instead of erroring out. I'm not sure that's the best behavior, but we should consider it versus having the user modify their source code when building for release. That would likely prove annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: ir IRgen and sway-ir including optimization passes compiler General compiler. Should eventually become more specific as the issue is triaged forc forc-test Everything related to the `forc-test` lib and `forc test` command. language feature Core language features visible to end users team:tooling Tooling Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants