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

Change macro syntax to be token tree based and fix legacy macros. #6388

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

gilbens-starkware
Copy link
Contributor

@gilbens-starkware gilbens-starkware commented Sep 18, 2024

commit-id:09f12638


This change is Reviewable

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.

Reviewed 3 of 41 files at r1, all commit messages.
Reviewable status: 3 of 41 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @mkaput, and @piotmag769)


crates/cairo-lang-language-server/tests/test_data/macro_expand/simple_inline.txt line 72 at r1 (raw file):

    core::result::ResultTrait::<
        (), core::fmt::Error
    >::unwrap(writeln!(__formatter_for_print_macros__, "some text"));

shouldn't it still expand?


crates/cairo-lang-lowering/src/borrow_check/test_data/borrow_check line 888 at r1 (raw file):

note: the variable needs to be dropped due to the potential panic here:
  --> lib.cairo:3:14
struct MyStruct {

mapping is broken.

Copy link
Collaborator

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

Reviewed 4 of 41 files at r1, all commit messages.
Reviewable status: 5 of 41 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @mkaput, and @orizi)


crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 269 at r1 (raw file):

    /// Receives a message from the server.
    async fn recv(&mut self) -> Result<Option<Message>, RecvError> {
        const TIMEOUT: Duration = Duration::from_secs(4 * 60);

Why?


crates/cairo-lang-language-server/tests/test_data/macro_expand/attribute.txt line 109 at r1 (raw file):

    core::result::ResultTrait::<
        (), core::fmt::Error
    >::unwrap(writeln!(__formatter_for_print_macros__, "a"));

It shouldn't have changed, this PR seems to break this features.


crates/cairo-lang-language-server/tests/test_data/macro_expand/simple_inline.txt line 72 at r1 (raw file):

Previously, orizi wrote…

shouldn't it still expand?

It should

Copy link
Collaborator

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

Reviewed 7 of 41 files at r2, all commit messages.
Reviewable status: 7 of 50 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @maciektr, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/ide/macros/expand.rs line 283 at r2 (raw file):

            content: config.content.into(),
            code_mappings: Default::default(),
            kind: match top_level_macro_kind {

@Draggu please verify this

Copy link
Collaborator

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 41 files at r2, all commit messages.
Reviewable status: 7 of 50 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, @maciektr, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/ide/macros/expand.rs line 283 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

@Draggu please verify this

This works

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.

Reviewed 5 of 41 files at r1, 13 of 41 files at r2, all commit messages.
Reviewable status: 22 of 50 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, @maciektr, and @mkaput)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 16 at r3 (raw file):

pub struct StableLocation {
    stable_ptr: SyntaxStablePtrId,
    inner_span: Option<(TextWidth, TextWidth)>,

doc - and especially explain the inner_span.

Code quote:

    stable_ptr: SyntaxStablePtrId,
    inner_span: Option<(TextWidth, TextWidth)>,

crates/cairo-lang-parser/src/macro_helpers.rs line 17 at r3 (raw file):

use crate::recovery::is_of_kind;

pub fn token_tree_as_wrapped_arg_list(

doc


crates/cairo-lang-parser/src/macro_helpers.rs line 54 at r3 (raw file):

}

pub trait AsLegacyInlineMacro {

doc all.


crates/cairo-lang-parser/src/parser.rs line 44 at r3 (raw file):

    /// An accumulating vector of pending skipped tokens diagnostics.
    pub(crate) pending_skipped_token_diagnostics: Vec<PendingParserDiagnostic>,
}

can't you add accessors for partial parsing instead of having pub(crate) all around?

Code quote:

pub struct Parser<'a> {
    pub(crate) db: &'a dyn SyntaxGroup,
    pub(crate) file_id: FileId,
    pub(crate) lexer: Lexer<'a>,
    /// The next terminal to handle.
    pub(crate) next_terminal: LexerTerminal,
    /// A vector of pending trivia to be added as leading trivia to the next valid terminal.
    pub(crate) pending_trivia: Vec<TriviumGreen>,
    /// The current offset, excluding the current terminal.
    pub(crate) offset: TextOffset,
    /// The width of the current terminal being handled (excluding the trivia length of this
    /// terminal).
    pub(crate) current_width: TextWidth,
    /// The length of the trailing trivia following the last read token.
    pub(crate) last_trivia_length: TextWidth,
    pub(crate) diagnostics: &'a mut DiagnosticsBuilder<ParserDiagnostic>,
    /// An accumulating vector of pending skipped tokens diagnostics.
    pub(crate) pending_skipped_token_diagnostics: Vec<PendingParserDiagnostic>,
}

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.

Reviewed 2 of 41 files at r2.
Reviewable status: 24 of 50 files reviewed, 8 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, @maciektr, and @mkaput)


crates/cairo-lang-runner/src/profiling_test_data/major_test_cases line 149 at r3 (raw file):

        0x54767f773cc172172c3afc5265bd0a76089c24cdef409635d27ac1a1fa96ca8, // calldata 7.
        0x65586264, // calldata 8.
    ].span();

this seems unrelated.


crates/cairo-lang-semantic/src/db.rs line 1606 at r3 (raw file):

                ));
            }
        }

Suggestion:

        diagnostics.add(SemanticDiagnostic::new(
            match plugin_diag.inner_span {
                None => StableLocation::new(plugin_diag.stable_ptr),
                Some(inner_span) => StableLocation::with_inner_span(plugin_diag.stable_ptr, inner_span),
            },
            SemanticDiagnosticKind::PluginDiagnostic(plugin_diag),
        )

crates/cairo-lang-semantic/src/diagnostic.rs line 46 at r3 (raw file):

        kind: SemanticDiagnosticKind,
    ) -> DiagnosticAdded;
    fn report_with_inner_span(

doc


crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros line 198 at r3 (raw file):


//! > diagnostics
error: Plugin diagnostic: Macro can not be parsed as legacy macro.

this seems like a degradation - probably requires more info in the diag.

Code quote:

lugin diagnostic: Macro can not be parsed as legacy macro.

Copy link
Collaborator

@wawel37 wawel37 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: 24 of 50 files reviewed, 9 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, @maciektr, and @mkaput)


crates/cairo-lang-parser/src/macro_helpers.rs line 26 at r3 (raw file):

    let mut lexer = Lexer::from_text(db, &node_text);
    let next_terminal = lexer.next().unwrap();
    // println!("Offset: {:?}", token_tree.stable_ptr().0.lookup(db).offset());

Leftover?

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 41 files at r2.
Reviewable status: 24 of 50 files reviewed, 9 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, and @maciektr)

@gilbens-starkware gilbens-starkware force-pushed the gil/macros branch 2 times, most recently from be8ac32 to 24c52fc Compare October 31, 2024 09:23
Copy link
Contributor Author

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

Reviewable status: 17 of 51 files reviewed, 8 unresolved discussions (waiting on @Arcticae, @Draggu, @maciektr, @orizi, and @piotmag769)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 16 at r3 (raw file):

Previously, orizi wrote…

doc - and especially explain the inner_span.

Done.


crates/cairo-lang-parser/src/macro_helpers.rs line 17 at r3 (raw file):

Previously, orizi wrote…

doc

Done.


crates/cairo-lang-parser/src/macro_helpers.rs line 54 at r3 (raw file):

Previously, orizi wrote…

doc all.

Done.


crates/cairo-lang-parser/src/parser.rs line 44 at r3 (raw file):

Previously, orizi wrote…

can't you add accessors for partial parsing instead of having pub(crate) all around?

Done.


crates/cairo-lang-semantic/src/db.rs line 1606 at r3 (raw file):

                ));
            }
        }

Done.


crates/cairo-lang-semantic/src/diagnostic.rs line 46 at r3 (raw file):

Previously, orizi wrote…

doc

Done.


crates/cairo-lang-semantic/src/expr/expansion_test_data/inline_macros line 198 at r3 (raw file):

Previously, orizi wrote…

this seems like a degradation - probably requires more info in the diag.

Done.


crates/cairo-lang-runner/src/profiling_test_data/major_test_cases line 149 at r3 (raw file):

Previously, orizi wrote…

this seems unrelated.

Done.

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.

Reviewed 8 of 15 files at r4, all commit messages.
Reviewable status: 25 of 51 files reviewed, 7 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, and @maciektr)


crates/cairo-lang-defs/src/plugin_utils.rs line 83 at r4 (raw file):

        "Macro can not be parsed as legacy macro.\n       Expected an argument list wrapped in \
         either parentheses, brackets, or braces."
            .to_string(),

Why the 2 tabs? this is not a code formatting.
probably use indoc! as well.

Suggestion:

    PluginDiagnostic::error(
        stable_ptr,
        "Macro can not be parsed as legacy macro.\nExpected an argument list wrapped in \
         either parentheses, brackets, or braces."
            .to_string(),

crates/cairo-lang-formatter/src/formatter_impl.rs line 796 at r4 (raw file):

    /// Should be called with a root syntax node to format a file.
    fn format_node(&mut self, syntax_node: &SyntaxNode) {
        if syntax_node.kind(self.db) == SyntaxKind::TokenTreeNode {

doc the hell out of this - as well as add a todo - as it WOULD break upon the first non arg_list conforming macro.


crates/cairo-lang-parser/src/parser.rs line 2960 at r4 (raw file):

    ///
    /// Returns the span of the skipped terminals, if any.
    pub(crate) fn skip_until(

why the pub(crate)s all around?

Code quote:

pub(crate)

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.

Reviewed 2 of 41 files at r1, 16 of 41 files at r2, 1 of 1 files at r3, 7 of 15 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, and @maciektr)


crates/cairo-lang-defs/src/plugin_utils.rs line 83 at r4 (raw file):

Previously, orizi wrote…

Why the 2 tabs? this is not a code formatting.
probably use indoc! as well.

just remove the \n you don't know what would format your error message.

Copy link
Contributor Author

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

Reviewable status: 47 of 51 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @maciektr, and @orizi)


crates/cairo-lang-defs/src/plugin_utils.rs line 83 at r4 (raw file):

Previously, orizi wrote…

just remove the \n you don't know what would format your error message.

Done.


crates/cairo-lang-formatter/src/formatter_impl.rs line 796 at r4 (raw file):

Previously, orizi wrote…

doc the hell out of this - as well as add a todo - as it WOULD break upon the first non arg_list conforming macro.

Done.


crates/cairo-lang-parser/src/parser.rs line 2960 at r4 (raw file):

Previously, orizi wrote…

why the pub(crate)s all around?

It is used in the macro helpers

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 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae and @maciektr)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @maciektr)

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 (waiting on @Arcticae, @gilbens-starkware, and @maciektr)

a discussion (no related file):
blocking - so won't be merged until additional parts are implemented.

consider moving to a feature branch.


@gilbens-starkware gilbens-starkware changed the base branch from main to high-level-inline-macros October 31, 2024 13:23
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.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @maciektr)

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.

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @maciektr)

Copy link
Member

@mkaput mkaput 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 7 of 41 files at r1, 24 of 41 files at r2, 1 of 1 files at r3, 10 of 15 files at r4, 3 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @maciektr)

Copy link
Collaborator

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 41 files at r1, 28 of 41 files at r2, 1 of 1 files at r3, 10 of 15 files at r4, 3 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, and @maciektr)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 49 at r7 (raw file):

    pub fn diagnostic_location(&self, db: &dyn DefsGroup) -> DiagnosticLocation {
        if self.inner_span.is_some() {
            println!("Diagnostic location: {:?}", self.inner_span);

remove


crates/cairo-lang-defs/src/diagnostic_utils.rs line 74 at r7 (raw file):

    ) -> DiagnosticLocation {
        if self.inner_span.is_some() {
            println!("Diagnostic location until: {:?}", self.inner_span);

remove

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 @Arcticae, @gilbens-starkware, and @maciektr)

Copy link
Contributor Author

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

Reviewable status: 50 of 51 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @maciektr, @mkaput, and @orizi)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 49 at r7 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

remove

Done.


crates/cairo-lang-defs/src/diagnostic_utils.rs line 74 at r7 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

remove

Done.

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.

Reviewed all commit messages.
Reviewable status: 50 of 51 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @maciektr, and @mkaput)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 22 at r8 (raw file):

    inner_span: Option<(TextWidth, TextWidth)>,
}
pub struct StableLocation {

remove new duplicates.

Copy link
Collaborator

@Draggu Draggu 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: 50 of 51 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, @maciektr, @mkaput, and @orizi)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 74 at r7 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Done.

Not done.

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.

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @gilbens-starkware, and @maciektr)

@gilbens-starkware gilbens-starkware force-pushed the gil/macros branch 2 times, most recently from 5269fc2 to b3281fa Compare January 7, 2025 20:44
Copy link
Contributor Author

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

Reviewable status: 50 of 52 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @maciektr, and @orizi)


crates/cairo-lang-defs/src/diagnostic_utils.rs line 74 at r7 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Not done.

Bad rebase, done.


crates/cairo-lang-defs/src/diagnostic_utils.rs line 22 at r8 (raw file):

Previously, orizi wrote…

remove new duplicates.

Done.

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.

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @maciektr)


corelib/src/test/language_features/for_test.cairo line 24 at r9 (raw file):

fn test_for_loop_array_tuples() {
    let mut i = 10;
    for (x, y) in array![(10, 10), (11, 11), (12, 12)] {

revert

Copy link
Collaborator

@Draggu Draggu 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 (waiting on @Arcticae, @gilbens-starkware, and @maciektr)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @gilbens-starkware, and @maciektr)

Copy link
Contributor Author

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @maciektr, and @orizi)


corelib/src/test/language_features/for_test.cairo line 24 at r9 (raw file):

Previously, orizi wrote…

revert

Done.

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.

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @maciektr)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Jan 9, 2025
Merged via the queue into high-level-inline-macros with commit d24ed19 Jan 9, 2025
45 checks passed
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.

6 participants