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

Supercharge traces #978

Merged
merged 10 commits into from
Jul 23, 2024
Merged

Supercharge traces #978

merged 10 commits into from
Jul 23, 2024

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jul 19, 2024

Preview

Default options

code result
trace @"my_label" image
trace @"my_label": @"foo", @"bar" image
trace @"my_label": [1, 2, 3], Some(#"00ff") image
trace (14, 42) image
trace fn(x) { x + 1 } image
trace @"my_label", @"foo", @"bar" image

--trace-level compact

code result
trace @"my_label" image
trace @"my_label": @"foo", @"bar" image
trace @"my_label": [1, 2, 3], Some(#"00ff") image
trace (14, 42) image
trace fn(x) { x + 1 } image
trace @"my_label", @"foo", @"bar" image

Changes

  • 📍 Allow variadic arguments in trace
    Although, doesn't do anything with them yet. The idea is to simplify
    the use of trace to make it a lot more useful than it currently is.

  • 📍 Type-check variadic traces & desugarize them.

  • 📍 Add 'diagnostic' to the prelude, as well as companion functions.
    This is not fully satisfactory as it pollutes a bit the prelude. Ideally, those functions should only be visible
    and usable by the underlying trace code. But for now, we'll just go with it.

  • 📍 Allow serialisable (Data-able) arguments to trace
    Somehow, we have to patch some function in gen_uplc because of the
    module name. I have to look further into this because it isn't normal.

  • 📍 Remove unnecessary code_gen patch.
    This is a little weird but, prelude functions are handled slightly
    differently.

  • 📍 re-introduce code-gen patch, but with a test.
    Actually, this has been a bug for a long time it seems. Calling any
    prelude functions using a qualified import would result in a codegen
    crash. Whoopsie.

    This is now fixed as shown by the regression test.

  • 📍 Rework 'compact' mode for traces

    • Trace-if-false are now completely discarded in compact mode.

    • Only the label (i.e. first trace argument) is preserved.

    • When compiling with tracing compact, the first label MUST unify to
      a string. This shouldn't be an issue generally speaking and would
      enforce that traces follow the pattern

      label: arg_0[, arg_1, ..., arg_n]
      

    Note that what isn't obvious with these changes is that we now support
    what the "emit" keyword was trying to achieve; as we compile now with
    user-defined traces only, and in compact mode to only keep event
    labels in the final contract; while allowing larger payloads with
    verbose tracing.

  • 📍 Display expected patterns/tokens in parse error when applicable.
    We've never been using those 'expected' tokens captured during
    parsing, which is lame because they contain useful information!

    This is much better than merely showing our infamous

    "Try removing it!"

  • 📍 Provide better parse errors in trace when using comma instead of colon.

  • 📍 Fill-in CHANGELOG

  Although, doesn't do anything with them yet. The idea is to simplify
  the use of trace to make it a lot more useful than it currently is.
@KtorZ KtorZ requested a review from a team as a code owner July 19, 2024 10:01
  This is not fully satisfactory as it pollutes a bit the prelude. Ideally, those functions should only be visible
  and usable by the underlying trace code. But for now, we'll just go with it.
  Somehow, we have to patch some function in gen_uplc because of the
  module name. I have to look further into this because it isn't normal.
  This is a little weird but, prelude functions are handled slightly
  differently.
  Actually, this has been a bug for a long time it seems. Calling any
  prelude functions using a qualified import would result in a codegen
  crash. Whoopsie.

  This is now fixed as shown by the regression test.
  - Trace-if-false are now completely discarded in compact mode.

  - Only the label (i.e. first trace argument) is preserved.

  - When compiling with tracing _compact_, the first label MUST unify to
    a string. This shouldn't be an issue generally speaking and would
    enforce that traces follow the pattern

    ```
    label: arg_0[, arg_1, ..., arg_n]
    ```

  Note that what isn't obvious with these changes is that we now support
  what the "emit" keyword was trying to achieve; as we compile now with
  user-defined traces only, and in compact mode to only keep event
  labels in the final contract; while allowing larger payloads with
  verbose tracing.
  We've never been using those 'expected' tokens captured during
  parsing, which is lame because they contain useful information!

  This is much better than merely showing our infamous

    "Try removing it!"
TypeIdent,
#[error("I found an unexpected identifier.")]
#[diagnostic(help("Try removing it!"))]
TermIdent,
Copy link
Member Author

Choose a reason for hiding this comment

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

dead-code

#[diagnostic(help(
"If no label is provided then only variables\nmatching a field name are allowed."
))]
RecordPunning,
Copy link
Member Author

Choose a reason for hiding this comment

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

dead-code

String::new()
} else {
module_name.clone()
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually a long-standing bug, see the corresponding gen_uplc test.

@@ -183,6 +183,6 @@ impl fmt::Display for Token {
Token::Validator => "validator",
Token::Via => "via",
};
write!(f, "\"{s}\"")
write!(f, "{s}")
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not 100% sure about the impact of this one. And also not sure why we were wrapping tokens in double-quotes when printing them. This was ugly in the error reporting so I removed them; but I am not certain that we don't show them in other places where the quotes might matter.

Anyway, we should add the quotes in-place where they're needed, and not from this function.

.delayed_if_then_else(constant_true, constant_false),
false,
)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: ☝️ This would crash prior to the fix in this PR.

@KtorZ KtorZ merged commit 967c264 into main Jul 23, 2024
12 checks passed
@KtorZ KtorZ deleted the supercharge_traces branch July 23, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ In Next Release
Development

Successfully merging this pull request may close these issues.

2 participants