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

Migrated the compiler to per-crate plugin queries #6843

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

integraledelebesgue
Copy link
Member

@integraledelebesgue integraledelebesgue commented Dec 6, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@integraledelebesgue integraledelebesgue force-pushed the spr/main/f0989263 branch 2 times, most recently from f500b20 to 2ff4cac Compare December 6, 2024 13:04
@integraledelebesgue integraledelebesgue changed the base branch from spr/main/d4abac73 to main December 9, 2024 21:24
@integraledelebesgue integraledelebesgue changed the title Migrated compiler to per-crate plugin queries Introduced per-crate plugin queries to the compiler Dec 9, 2024
@integraledelebesgue integraledelebesgue changed the base branch from main to spr/main/ffce04df December 9, 2024 21:24
@integraledelebesgue integraledelebesgue force-pushed the spr/main/f0989263 branch 2 times, most recently from 6c8700c to eb2d318 Compare December 9, 2024 21:37
@integraledelebesgue integraledelebesgue requested review from piotmag769 and removed request for wawel37 December 9, 2024 21:48
Copy link
Member Author

@integraledelebesgue integraledelebesgue 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 65 files reviewed, 1 unresolved discussion (waiting on @mkaput, @orizi, and @piotmag769)


a discussion (no related file):
I have two problems here which I am work on now:

  1. Fallback from "override" to "default" input (e.g. DefsGroup::default_macro_plugins and DefsGroup::override_crate_macro_plugins) is tricky to implement due to the way the input getters are implemented in Salsa: they panic. Sadly, I cannot catch the panic without constraining the DefsGroup (and, consequently, all its subtraits) to UnwindSafe.
  2. Tests related to circuits break because the compiler's input has Tuple<...> types instead of (...) structural types.

However, the rest of the stack is ready to be reviewed.

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 58 of 64 files at r3, all commit messages.
Reviewable status: 58 of 65 files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue, @orizi, and @piotmag769)


a discussion (no related file):

Previously, integraledelebesgue (Jan Smółka) wrote…

I have two problems here which I am work on now:

  1. Fallback from "override" to "default" input (e.g. DefsGroup::default_macro_plugins and DefsGroup::override_crate_macro_plugins) is tricky to implement due to the way the input getters are implemented in Salsa: they panic. Sadly, I cannot catch the panic without constraining the DefsGroup (and, consequently, all its subtraits) to UnwindSafe.
  2. Tests related to circuits break because the compiler's input has Tuple<...> types instead of (...) structural types.

However, the rest of the stack is ready to be reviewed.

Ad 1. I believe that's the reason file_overrides in FilesGroup are implemented as two queries: one, the parameterless input, keeps an HashMap of FileId -> override and the other query meant for broad use does the lookup and returns Option

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 1 of 65 files at r1, 6 of 64 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @integraledelebesgue, @orizi, and @piotmag769)


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

    /// containing it.
    pub fn is_phantom(&self, db: &dyn SemanticGroup) -> bool {
        let Some(module_file_id) = self.module_file_id(db) else {

I think this generalization of module_file_id for any TypeId is misleading a bit, and not really that useful outside this context. See my other comment down this file for an explanation of how I believe this should work.


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

                    .iter()
                    .any(|attr| id.has_attr(db, attr).unwrap_or_default()),
            },

I think this is the place where you should actually look for the crate and its declared phantom type attrs. Here, you know a concrete item that we're interested in, so we can always reliably ask for its crate.

Code quote:

            TypeLongId::Concrete(id) => match id {
                ConcreteTypeId::Struct(id) => phantom_type_attributes
                    .iter()
                    .any(|attr| id.has_attr(db, attr).unwrap_or_default()),
                ConcreteTypeId::Enum(id) => phantom_type_attributes
                    .iter()
                    .any(|attr| id.has_attr(db, attr).unwrap_or_default()),
                ConcreteTypeId::Extern(id) => phantom_type_attributes
                    .iter()
                    .any(|attr| id.has_attr(db, attr).unwrap_or_default()),
            },

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

            TypeLongId::Tuple(inner) => inner.iter().any(|ty| ty.is_phantom(db)),
            TypeLongId::FixedSizeArray { type_id, .. } => type_id.is_phantom(db),
            TypeLongId::Snapshot(_)

This is an example where generalized module_file_id might not be a good idea: we don't need it for snapshots, yet your code attempted to look for something.

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.

:lgtm:

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @integraledelebesgue, @orizi, and @piotmag769)

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 6 of 6 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)

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 53 of 53 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)

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 6 of 6 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)

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 2 of 6 files at r8, 47 of 53 files at r9, 6 of 6 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)

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 2 of 6 files at r8, 47 of 53 files at r9, 6 of 6 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)

@integraledelebesgue integraledelebesgue changed the base branch from spr/main/ffce04df to main February 3, 2025 12:51
@integraledelebesgue integraledelebesgue added this pull request to the merge queue Feb 3, 2025
Merged via the queue into main with commit a106b04 Feb 3, 2025
48 checks passed
integraledelebesgue added a commit that referenced this pull request Feb 3, 2025
integraledelebesgue added a commit that referenced this pull request Feb 3, 2025
@orizi orizi deleted the spr/main/f0989263 branch February 5, 2025 11:07
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