Create api for diagnostics ensure with no need for lowering group

[wawel37]

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 

[orizi]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mkaput and @wawel37)

a discussion (no related file):
please explain more - and not that there are some missing diagnostics if this is skipped.

---

[maciektr]

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.

[orizi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @maciektr and @mkaput)

a discussion (no related file):

Previously, maciektr (maciektr) wrote…

@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.

can you try setting up the db with - "add_withdraw_gas" flag as false? i think that is the cause for the extra time.

---

[maciektr]

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? 🤔 I may be wrong, but it just don't seem like an enough reason to do so.

[mkaput]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @maciektr and @orizi)

a discussion (no related file):

Previously, maciektr (maciektr) wrote…

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? 🤔 I may be wrong, but it just don't seem like an enough reason to do so.

yeah I think being able to skip LoweringGroup should be fine. scarb doc is only an example of a tool from a class of stuff that do not need deep semantic analysis

---

[mkaput]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

[orizi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wawel37)

---

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

    /// Ignores all the diagnostics related to LoweringGroup.
    /// Returns `Err` if diagnostics were found.
    pub fn ensure_without_lowering_group(

i don't like this name - or the concept - i think you should just have a configuration of which level of diagnostics you want to get - and just get diagnostics up to that point.
the "without" part is weird.

[maciektr]

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?

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciektr)

---

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

Previously, maciektr (maciektr) wrote…

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?

no - it is all about the queries you actually call.
if you won't call the lowering-diags queries - they won't be called.

[wawel37]

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

---

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

Previously, orizi wrote…

no - it is all about the queries you actually call.
if you won't call the lowering-diags queries - they won't be called.

But is it a good idea to use the LoweringGroup within the scarb doc, even if we don't need any of the queries from the group? It will be added there just to make diagnostics run with only LoweringGroup (and make it a little bit cleaner code on the diagnostic api side). What do you think?

[orizi]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @maciektr, @mkaput, and @wawel37)

---

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

Previously, wawel37 (Mateusz Kowalski) wrote…

But is it a good idea to use the LoweringGroup within the scarb doc, even if we don't need any of the queries from the group? It will be added there just to make diagnostics run with only LoweringGroup (and make it a little bit cleaner code on the diagnostic api side). What do you think?

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

in general - if you only want top level items cairo-lang-defs should be enough. only parser level diagnostics should actually be of any issue to you.

[maciektr]

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

[orizi]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @maciektr, @mkaput, and @wawel37)

---

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

Previously, maciektr (maciektr) wrote…

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

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).

[maciektr]

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? 🤔

[wawel37]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @maciektr, @mkaput, and @orizi)

---

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

Previously, maciektr (maciektr) wrote…

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

Do you mean we should run queries, and if any of them fails, only then call the ensure?

[orizi]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @maciektr, @mkaput, and @wawel37)

---

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

Previously, wawel37 (Mateusz Kowalski) wrote…

Do you mean we should run queries, and if any of them fails, only then call the ensure?

Exactly

[mkaput]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciektr and @wawel37)

[wawel37]

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

---

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

Previously, orizi wrote…

Exactly

As I tried to make it run only on queries error, i noticed that sometimes queries will run without an error even with wrong code, for example:

/// test comment
fn main() {
    println!("Hello world!");
    wrong code
}

And if we skip the ensure when there aren't any errors from queries, we won't display any errors. Running cargo doc with same code results in diagnostics error. I don't think that generating docs with the wrong code is a good idea. Is it guaranteed for queries to be correct after queries haven't thrown any errors but the diagnostics have?

[orizi]

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

---

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

Previously, wawel37 (Mateusz Kowalski) wrote…

As I tried to make it run only on queries error, i noticed that sometimes queries will run without an error even with wrong code, for example:

/// test comment
fn main() {
    println!("Hello world!");
    wrong code
}

And if we skip the ensure when there aren't any errors from queries, we won't display any errors. Running cargo doc with same code results in diagnostics error. I don't think that generating docs with the wrong code is a good idea. Is it guaranteed for queries to be correct after queries haven't thrown any errors but the diagnostics have?

this is an inconsistency - either you don't care about diagnostics - and then just get the best result you can - and if you fail - return the diagnostics (that probably caused the issue).
or you care about diagnostics - and you should return all of them.

being under the assumption that someone exporting docs actually compiled their projects sounds ok with me.

[wawel37]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)

---

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

Previously, orizi wrote…

this is an inconsistency - either you don't care about diagnostics - and then just get the best result you can - and if you fail - return the diagnostics (that probably caused the issue).
or you care about diagnostics - and you should return all of them.

being under the assumption that someone exporting docs actually compiled their projects sounds ok with me.

Done. Made it configurable on the DiagnosticsReporter level.

[mkaput]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi and @wawel37)

---

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

    pub fn callback(
        callback: impl FnMut(FormattedDiagnosticEntry) + 'a,
        include_lowering_diagnostics: bool,

that's a bad pattern, this standalone true/false will be very unreadable in code; and also you can't pursue a notion of "default" value

also adding param here is a breaking API change

You should do a (pseudo-) builder pattern here:

DiagnosticsReporter::callback(..).skip_lowering_diagnostics()

[orizi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @wawel37)

---

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

                        self.check_diag_group(db.upcast(), group, ignore_warnings_in_crate);
                }
                if self.include_lowering_diagnostics {

i think flipping the flag is more sensible.
optionally instead - just make it into a DiagLevel enum - (Syntax, Semantic, Full)

Suggestion:

                if self.skip_lowering_diagnostics {
                    continue;
                }

[wawel37]

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

---

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

Previously, mkaput (Marek Kaput) wrote…

that's a bad pattern, this standalone true/false will be very unreadable in code; and also you can't pursue a notion of "default" value

also adding param here is a breaking API change

You should do a (pseudo-) builder pattern here:

DiagnosticsReporter::callback(..).skip_lowering_diagnostics()

Done.

---

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

Previously, orizi wrote…

i think flipping the flag is more sensible.
optionally instead - just make it into a DiagLevel enum - (Syntax, Semantic, Full)

Done. Will leave only the skip for lowering group for now, because no one will use another levels right now.

[orizi]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)

[mkaput]

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