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

Store plugins in the Crate structure #209

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

Conversation

integraledelebesgue
Copy link
Member

@integraledelebesgue integraledelebesgue commented Jan 21, 2025

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

src/project/scarb.rs Outdated Show resolved Hide resolved
src/project/scarb.rs Outdated Show resolved Hide resolved
acc
});

debug!("Plugins for {}: {:#?}", package.unwrap().id.repr, plugins);
Copy link
Contributor

Choose a reason for hiding this comment

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

Crates are all logged to debug here

debug!("updating crate roots from scarb metadata: {crates:#?}");

Copy link
Member Author

@integraledelebesgue integraledelebesgue Jan 23, 2025

Choose a reason for hiding this comment

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

This message is for debugging purposes. I added a comment to remind me to delete it before merging

src/project/scarb.rs Outdated Show resolved Hide resolved
src/project/crate_data.rs Outdated Show resolved Hide resolved
src/project/scarb.rs Outdated Show resolved Hide resolved
Comment on lines +41 to +43
/// Plugins required by the crate.
#[allow(dead_code)] // TODO: remove. The field is used later in the stack
pub plugins: PluginSuite,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Plugins required by the crate.
#[allow(dead_code)] // TODO: remove. The field is used later in the stack
pub plugins: PluginSuite,
/// Built-in plugins required by the crate.
#[allow(dead_code)] // TODO: remove. The field is used later in the stack
pub built_in_plugins: PluginSuite,

use cairo_lang_utils::{Intern, LookupIntern};
use smol_str::SmolStr;

use crate::lang::db::AnalysisDatabase;

/// A complete set of information needed to set up a real crate in the analysis database.
#[derive(Debug, PartialEq, Eq)]
#[derive(Debug)]
Copy link
Member

Choose a reason for hiding this comment

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

I would like to preserve the plain nature of this struct. I know we're not based on upstream changes to the compiler yet here, but do you think if it would be possible to keep just a set of plugin ids here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have plugin ids in this PR at all since they are never interned, only treated as an input

Copy link
Member Author

Choose a reason for hiding this comment

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

Storing IDs in Crate seems possible when we start using the new compiler API, but it would require passing a database to extract_crates, which can be tricky

Copy link
Member

Choose a reason for hiding this comment

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

@piotmag769 is right. we would need to have a custom ID mechanism (PackageIDs I guess) at this stage. nonetheless, this is a separate piece of work which is not needed urgently right now

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.

4 participants