starkware-libs / cairo

Cairo is the first Turing-complete language for creating provable programs for general computation.
Apache License 2.0
1.56k stars 479 forks source link

Create api for diagnostics ensure with no need for lowering group #6440

Open wawel37 opened 1 day ago

wawel37 commented 1 day ago

We decided to create a diagnostics' api, that doesn't require a LoweringGroup db, as without it, the diagnostic performence is much more better. For more information, click here.


This change is Reviewable

maciektr commented 1 day ago
Previously, orizi wrote…
please explain more - and not that there are some missing diagnostics if this is skipped.

@orizi It is expected to be used by scarb-doc which does not need LoweringGroup. The intention is to be able to calculate diagnostics without adding lowering group just for this purpose.

maciektr commented 20 hours ago
Previously, orizi wrote…
can you try setting up the db with - "add_withdraw_gas" flag as `false`? i think that is the cause for the extra time.

Hmm , I have tried setting

        let add_withdraw_gas_flag_id = FlagId::new(db.upcast(), "add_withdraw_gas");
        db.set_flag(
            add_withdraw_gas_flag_id,
            Some(Arc::new(Flag::AddWithdrawGas(false))),
        );

        db.set_optimization_config(Arc::new(
            OptimizationConfig::default().with_inlining_strategy(InliningStrategy::Avoid),
        ));

and did not see any real difference in benchmarks.

To be honest, the time penalty is a one thing, but would you advise adding the LoweringGroup to the database only in order to calculate diagnostics? :thinking: I may be wrong, but it just don't seem like an enough reason to do so.

maciektr commented 9 hours ago

crates/cairo-lang-compiler/src/diagnostics.rs line 249 at r1 (raw file):

i think you should just have a configuration of which level of diagnostics you want to get

Wouldn't we still be forced to add the LoweringGroup to the database, just to make type system happy?

maciektr commented 9 hours ago

crates/cairo-lang-compiler/src/diagnostics.rs line 249 at r1 (raw file):

it is already dependent on it assuming we use the cairo-lang-compiler crate

Scarb doc is not doing that. It only relies on queries from DocGroup (and dependant groups, up to the semantic one) - https://github.com/software-mansion/scarb/blob/0366ff08f58e5b554aed840b952cc3f7637347a3/extensions/scarb-doc/src/db.rs#L22-L29

maciektr commented 8 hours ago

crates/cairo-lang-compiler/src/diagnostics.rs line 249 at r1 (raw file):

Previously, orizi wrote…
so you can probably just run it without the `ensure` part - and it would still be fine. if for some reason it fails - only then call the diagnostics. everything would be much faster that way (in the good case).

Hmm, good point! By only then call diagnostics you still mean with check or something else? :thinking: