Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

feat: forc-test predicate logging#6996

Open
zees-dev wants to merge29 commits intomaster
base:master
Choose a base branch
Loading
fromfeat/forc-test-predicate-logging

Conversation

zees-dev
Copy link
Contributor

@zees-devzees-dev commentedMar 5, 2025
edited
Loading

Description

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

Notes:

  • To make this work we needed to create and propagate theenable_predicate_logs attribute in theBuildProfile,BuildConfig, andContext structs; this needed to be propagated down deep into their_generation so that the__log compiler intrinsic could branch based on theenable_predicate_logs attribute and the predicate programKind.
  • With this change; theforc test --logs andforc test --raw-logs commands will now output logs for predicates.
  • forc build will compile the predicate (which contains logs) indebug mode successfully
  • forc build willNOT compile the predicate (which contains logs) inrelease mode
  • scripts, contracts, libraries should remain unaffected by this change

Example

Predicate

predicate;pubstructCallParams {pubcoins:u64,pubgas:u64,}fnmain()->bool {letcall_params=CallParams {coins:7,gas:8,    };log(call_params);false}#[test]fntest_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 necessaryBreaking* orNew Feature labels where relevant.
  • I have done my best to ensure that my PR adheres tothe Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@zees-devzees-dev requested review froma team ascode ownersMarch 5, 2025 22:08
@zees-devzees-dev self-assigned thisMar 5, 2025
@zees-devzees-dev added forc forc-testEverything related to the `forc-test` lib and `forc test` command. team:toolingTooling Team compiler: irIRgen and sway-ir including optimization passes compilerGeneral compiler. Should eventually become more specific as the issue is triaged language featureCore language features visible to end users labelsMar 5, 2025
@codspeed-hqCodSpeed HQ
Copy link

codspeed-hqbot commentedMar 5, 2025
edited
Loading

CodSpeed Performance Report

Merging#6996 willnot alter performance

Comparingfeat/forc-test-predicate-logging (4ae3ab1) withmaster (d46f08a)

Summary

✅ 22 untouched benchmarks

Copy link
Member

@kayagokalpkayagokalp left a comment
edited
Loading

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 makelog_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 reacted with thumbs up emoji
@zees-dev
Copy link
ContributorAuthor

Nice one! This looks good to me in general we should probably makelog_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 withenable_predicate_logs being on by default indebug builds but not inrelease builds?

Copy link
Contributor

@alfiedotwtfalfiedotwtf 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 changesMar 6, 2025
alfiedotwtf
alfiedotwtf previously approved these changesMar 6, 2025
Copy link
Member

@kayagokalpkayagokalp 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 reacted with thumbs up emoji
@zees-dev
Copy link
ContributorAuthor

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

addressed:64d5b69

Copy link
Member

@sdankelsdankel 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

@kayagokalpkayagokalp 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

@JoshuaBattyJoshuaBatty 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-111IGI-111 left a comment
edited
Loading

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

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.

zees-dev reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@IGI-111IGI-111IGI-111 requested changes

@JoshuaBattyJoshuaBattyJoshuaBatty approved these changes

@kayagokalpkayagokalpkayagokalp approved these changes

@sdankelsdankelsdankel approved these changes

@alfiedotwtfalfiedotwtfalfiedotwtf left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@zees-devzees-dev

Labels
compiler: irIRgen and sway-ir including optimization passescompilerGeneral compiler. Should eventually become more specific as the issue is triagedforcforc-testEverything related to the `forc-test` lib and `forc test` command.language featureCore language features visible to end usersteam:toolingTooling Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@zees-dev@kayagokalp@IGI-111@alfiedotwtf@JoshuaBatty@sdankel

[8]ページ先頭

©2009-2025 Movatter.jp