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

Display for snapshots #4745

Merged
merged 1 commit into from
Jan 28, 2024
Merged

Display for snapshots #4745

merged 1 commit into from
Jan 28, 2024

Conversation

piotmag769
Copy link
Collaborator

@piotmag769 piotmag769 commented Jan 5, 2024

Came up with it after the previous PR was closed :/


This change is Reviewable

@mkaput mkaput requested a review from orizi January 5, 2024 13:08
Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)

a discussion (no related file):
I added this because without it snapshot implementations of Display required specific import of the concrete impl - example below

package structure:

src/
  lib.cairo
  abc.cairo

lib.cairo:

mod abc;
use core::fmt::{Display, Formatter, Error};

#[derive(Drop)]
struct Y {}

impl DisplayY of Display<Y> {
  fn fmt(self: @Y, ref f: Formatter) -> Result<(), Error> {
      Result::Ok(())
  }
}

impl DisplayYSnap of Display<@Y> {
  fn fmt(self: @@Y, ref f: Formatter) -> Result<(), Error> {
    Result::Ok(())
  }
}

abc.cairo

use super::Y;

fn func() {
  println!("{}", @Y{});
}

For this package scarb build fails. The fix is to add use super::DisplayYSnap; to abc.cairo. Is it a desired behaviour?


Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)

a discussion (no related file):

Previously, piotmag769 (Piotr Magiera) wrote…

I added this because without it snapshot implementations of Display required specific import of the concrete impl - example below

package structure:

src/
  lib.cairo
  abc.cairo

lib.cairo:

mod abc;
use core::fmt::{Display, Formatter, Error};

#[derive(Drop)]
struct Y {}

impl DisplayY of Display<Y> {
  fn fmt(self: @Y, ref f: Formatter) -> Result<(), Error> {
      Result::Ok(())
  }
}

impl DisplayYSnap of Display<@Y> {
  fn fmt(self: @@Y, ref f: Formatter) -> Result<(), Error> {
    Result::Ok(())
  }
}

abc.cairo

use super::Y;

fn func() {
  println!("{}", @Y{});
}

For this package scarb build fails. The fix is to add use super::DisplayYSnap; to abc.cairo. Is it a desired behaviour?

Clarification:
the error message is

error: Trait has no implementation in context: core::fmt::Display::<@uwu::Y>
 --> /Users/piotrmagiera/SWM/uwu/src/abc.cairo[println_macro][writeln_macro]:3:31
    match core::fmt::Display::fmt(__write_macro_arg0__, ref __formatter_for_print_macros__) {

and the part of the code

impl DisplayY of Display<Y> {
  fn fmt(self: @Y, ref f: Formatter) -> Result<(), Error> { 
    Result::Ok(()) 
  }
}

is not needed to reproduce


Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)

a discussion (no related file):

Previously, piotmag769 (Piotr Magiera) wrote…

Clarification:
the error message is

error: Trait has no implementation in context: core::fmt::Display::<@uwu::Y>
 --> /Users/piotrmagiera/SWM/uwu/src/abc.cairo[println_macro][writeln_macro]:3:31
    match core::fmt::Display::fmt(__write_macro_arg0__, ref __formatter_for_print_macros__) {

and the part of the code

impl DisplayY of Display<Y> {
  fn fmt(self: @Y, ref f: Formatter) -> Result<(), Error> { 
    Result::Ok(()) 
  }
}

is not needed to reproduce

Update: same thing happens when implementing Display for Array<Y>

error: Trait has no implementation in context: core::fmt::Display::<core::array::Array::<uwu::Y>>
 --> /Users/piotrmagiera/SWM/uwu/src/abc.cairo[println_macro][writeln_macro]:3:31
    match core::fmt::Display::fmt(__write_macro_arg0__, ref __formatter_for_print_macros__) {
                              ^*^

I've used Scarb depending on Cairo from the main branch (bumped the Cairo deps for Scarb locally and built it), so it uses the latest possible compiler version - both issues still occur.


Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @piotmag769)

a discussion (no related file):
I'm not sure we actually want this - do note that any user of this can actually just call this with adding a *.
The diagnostic itself indeed should be better mapped to the original user code.


Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @orizi)

a discussion (no related file):

do note that any user of this can actually just call this with adding a *

Isn't Copy trait required to use desnap operator though? It would be helpful for printing e.g. snapshot of a struct that has a field of a type Array<felt252>. Note that there is a similar generic implementation for Debug for snapshots in corelib already.

impl DebugSnapshot<T, +Debug<T>> of Debug<@T> {


@piotmag769 piotmag769 requested a review from orizi January 7, 2024 19:20
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion

a discussion (no related file):
Adding @ilyalesokhin-starkware for 2nd eye.


Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @piotmag769)

a discussion (no related file):
@ilyalesokhin-starkware for 2nd eye


Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi and @piotmag769)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Jan 28, 2024
Merged via the queue into starkware-libs:main with commit 58a4967 Jan 28, 2024
@piotmag769 piotmag769 deleted the display-for-snapshots branch January 29, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants