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 default plugin suites to the RootDatabaseBuilder #6852

Open
wants to merge 1 commit into
base: spr/main/5dc187d6
Choose a base branch
from

Conversation

integraledelebesgue
Copy link
Collaborator

@integraledelebesgue integraledelebesgue commented Dec 9, 2024

@reviewable-StarkWare
Copy link

This change is Reviewable

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


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

    /// Sets macro, inline macro and analyzer plugins specified in the [`PluginSuite`] as default
    /// for all crates.
    /// ***

this hr was not idiomatic

Suggestion:

    ///

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

    /// Sets macro, inline macro and analyzer plugins present in the [`PluginSuite`] for a crate
    /// pointed to by the [`CrateId`], overriding the defaults for that crate.
    /// ***

Suggestion:

    ///

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


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

            .into_iter()
            .map(|plugin| self.intern_analyzer_plugin(AnalyzerPluginLongId(plugin)))
            .collect();

Can we deduplicate this logic? Also, make it a public api; it will be useful in LS, too.

Code quote:

        let PluginSuite { plugins, inline_macro_plugins, analyzer_plugins } = suite;

        let macro_plugins = plugins
            .into_iter()
            .map(|plugin| self.intern_macro_plugin(MacroPluginLongId(plugin)))
            .collect();

        let inline_macro_plugins = inline_macro_plugins
            .into_iter()
            .map(|(name, plugin)| {
                (name, self.intern_inline_macro_plugin(InlineMacroExprPluginLongId(plugin)))
            })
            .collect();

        let analyzer_plugins = analyzer_plugins
            .into_iter()
            .map(|plugin| self.intern_analyzer_plugin(AnalyzerPluginLongId(plugin)))
            .collect();

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: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)


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

Previously, Draggu (Piotr Figiela) wrote…

Can we deduplicate this logic? Also, make it a public api; it will be useful in LS, too.

I extracted it as another helper.
These helpers should already be public since they belong to a pub trait, mutually implemented on every type that is a SemanticGroup.

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: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @integraledelebesgue, @mkaput, @orizi, and @piotmag769)


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

        &mut self,
        suite: PluginSuite,
    ) -> (Vec<MacroPluginId>, OrderedHashMap<String, InlineMacroExprPluginId>, Vec<AnalyzerPluginId>)

Make struct for these.

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

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: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)


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

Previously, Draggu (Piotr Figiela) wrote…

Make struct for these.

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 11 of 12 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @integraledelebesgue, @orizi, and @piotmag769)


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

    /// [`DefsGroup::default_inline_macro_plugins`], and
    /// [`SemanticGroup::default_analyzer_plugins`].
    fn set_default_plugins_from_suite(&mut self, suite: PluginSuite) {

Maybe inverse this? up to you

Suggestion:

InternedPluginSuite

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

    /// [`DefsGroup::override_crate_inline_macro_plugins`], and
    /// [`SemanticGroup::override_crate_analyzer_plugins`].
    fn set_override_crate_plugins_from_suite(&mut self, crate_id: CrateId, suite: PluginSuite) {

vide supra

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

@integraledelebesgue integraledelebesgue force-pushed the spr/main/ffce04df branch 2 times, most recently from 4b6a57f to 8b84cb3 Compare December 13, 2024 07:13
@integraledelebesgue integraledelebesgue force-pushed the spr/main/5dc187d6 branch 2 times, most recently from 15418f8 to 0318f63 Compare December 13, 2024 07:29
@integraledelebesgue integraledelebesgue force-pushed the spr/main/ffce04df branch 2 times, most recently from 2a0cd52 to 5438ec5 Compare December 13, 2024 08:17
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 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @integraledelebesgue, @orizi, and @piotmag769)

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: 11 of 13 files reviewed, all discussions resolved (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)


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

Previously, Draggu (Piotr Figiela) wrote…

Maybe inverse this? up to you

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


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

        self.set_inline_macro_plugins(Arc::new(raw_inline_macro_plugins));
        self.set_analyzer_plugins(raw_analyzer_plugins);
        // ---

?

Code quote:

        // ---

crates/cairo-lang-semantic/src/plugin.rs line 84 at r5 (raw file):

    pub inline_macro_plugins: Arc<OrderedHashMap<String, InlineMacroExprPluginId>>,
    pub analyzer_plugins: Arc<[AnalyzerPluginId]>,
}

maybe just have these as the original PluginSuite and having the creator responsible for interning?

Code quote:

#[derive(Clone, Debug)]
pub struct InternedPluginSuite {
    pub macro_plugins: Arc<[MacroPluginId]>,
    pub inline_macro_plugins: Arc<OrderedHashMap<String, InlineMacroExprPluginId>>,
    pub analyzer_plugins: Arc<[AnalyzerPluginId]>,
}

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 and @piotmag769)


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

Previously, orizi wrote…

?

Invoking these old setters is required to keep the compiler working while shifting to the new logic with default plugins. In this PR, queries related to the default and crate-specific plugins are not introduced to the compiler yet.


crates/cairo-lang-semantic/src/plugin.rs line 84 at r5 (raw file):

Previously, orizi wrote…

maybe just have these as the original PluginSuite and having the creator responsible for interning?

It's just a helper, plain-data struct reducing the boilerplate in the methods responsible for setting per-crate plugins by wrapping those Arcs. I think that the current logic is nicer and more flexible - we first set up the suite, modify its contents depending on our needs and eventually intern its contents all at once before setting them as a default or for a 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.

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


crates/cairo-lang-semantic/src/plugin.rs line 84 at r5 (raw file):

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

It's just a helper, plain-data struct reducing the boilerplate in the methods responsible for setting per-crate plugins by wrapping those Arcs. I think that the current logic is nicer and more flexible - we first set up the suite, modify its contents depending on our needs and eventually intern its contents all at once before setting them as a default or for a crate.

this sounds like something for a testing framework - for the general purpose - i don't like this hiding of behaviour - you're going to have this interning in any case, so you should probably not have the un-interned data here.

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 11 of 12 files at r1, 2 of 2 files at r3, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-semantic/src/plugin.rs line 84 at r5 (raw file):

Previously, orizi wrote…

this sounds like something for a testing framework - for the general purpose - i don't like this hiding of behaviour - you're going to have this interning in any case, so you should probably not have the un-interned data here.

Agree with @orizi here

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 12 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @integraledelebesgue)

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