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

Introduced queries for crate plugins in DefsGroup and SemanticGroup #6840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

integraledelebesgue
Copy link
Collaborator

@integraledelebesgue integraledelebesgue commented Dec 6, 2024

@reviewable-StarkWare
Copy link

This change is Reviewable

@integraledelebesgue integraledelebesgue force-pushed the spr/main/5dc187d6 branch 2 times, most recently from 864ea77 to 458e8d6 Compare December 6, 2024 13:04
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 r1, 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.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @integraledelebesgue)


crates/cairo-lang-defs/src/ids.rs line 359 at r1 (raw file):

}

impl Clone for MacroPluginLongId {

why can't this be derived?


crates/cairo-lang-defs/src/ids.rs line 369 at r1 (raw file):

impl PartialEq for MacroPluginLongId {
    fn eq(&self, other: &Self) -> bool {
        *self.0 == *other.0

just pointer equality should be actually enough for us. we already rely on it in LS :)

@integraledelebesgue integraledelebesgue changed the base branch from spr/main/57b41a03 to main December 9, 2024 21:24
@integraledelebesgue integraledelebesgue force-pushed the spr/main/5dc187d6 branch 3 times, most recently from 79ec121 to e6bd4c0 Compare December 9, 2024 21:46
Copy link
Collaborator 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: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @mkaput and @orizi)


crates/cairo-lang-defs/src/ids.rs line 359 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why can't this be derived?

It absolutely can, idk why I thought it couldn't :)


crates/cairo-lang-defs/src/ids.rs line 369 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

just pointer equality should be actually enough for us. we already rely on it in LS :)

Done, as we've concluded

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 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi and @piotmag769)

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.

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


crates/cairo-lang-defs/src/db.rs line 107 at r2 (raw file):

    #[salsa::input]
    fn override_crate_macro_plugins(&self, crate_id: CrateId) -> Vec<MacroPluginId>;

etc

Suggestion:

    #[salsa::input]
    fn default_macro_plugins(&self) -> Arc<[MacroPluginId]>;

    #[salsa::input]
    fn override_crate_macro_plugins(&self, crate_id: CrateId) -> Arc<[MacroPluginId]>;

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, 2 unresolved discussions (waiting on @integraledelebesgue, @mkaput, @orizi, and @piotmag769)


crates/cairo-lang-defs/src/db.rs line 107 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

etc

Apply it to other plugins too.


crates/cairo-lang-defs/src/db.rs line 119 at r2 (raw file):

    #[salsa::input]
    fn default_inline_macro_plugins(&self) -> OrderedHashMap<String, InlineMacroExprPluginId>;

Should this be wrapped in Arc? @mkaput

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.

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


crates/cairo-lang-defs/src/db.rs line 119 at r2 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Should this be wrapped in Arc? @mkaput

yes it should

Copy link
Collaborator 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: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @mkaput, @orizi, and @piotmag769)


crates/cairo-lang-defs/src/db.rs line 107 at r2 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Apply it to other plugins too.

Done

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:

Reviewed 1 of 6 files at r1, 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput, @orizi, and @piotmag769)

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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi and @piotmag769)

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.

:lgtm:

Reviewed 1 of 6 files at r1, 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue and @orizi)


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

    fn macro_plugins(&self) -> Vec<Arc<dyn MacroPlugin>>; // TODO: Delete in favour or [`default_macro_plugins`]
    #[salsa::input] // TODO: Delete in favour or [`default_inline_macro_plugins`]
    fn inline_macro_plugins(&self) -> Arc<OrderedHashMap<String, Arc<dyn InlineMacroExprPlugin>>>;

[nit] Move this lower to group with inline macro methods

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.

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

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 r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @integraledelebesgue, @orizi, and @wawel37)


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

/// Initializes the [`SemanticGroup`] database to a proper state.
pub fn init_semantic_group(db: &mut dyn SemanticGroup) {

you have to add calls to all these functions anywhere a database is created. we unfortuately have no compile-time checks for missing initializations, so you need to do a little witch hunt :(

Copy link
Collaborator 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: all files reviewed, 2 unresolved discussions (waiting on @orizi, @piotmag769, and @wawel37)


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

Previously, piotmag769 (Piotr Magiera) wrote…

[nit] Move this lower to group with inline macro methods

That's an old, already present code, that I removed in the last PR. I think it's no use touching it

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:

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

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 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @piotmag769 and @wawel37)

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.

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wawel37)

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: all files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue and @wawel37)


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

        plugins: Arc<[MacroPluginId]>,
    ) {
        let mut overrides = self.macro_plugin_overrides().as_ref().clone();

I know this is the way override_file_content is done but is it needed here? Cannot we just modify the arc? cc @mkaput @Draggu

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