mozilla / rust-code-analysis

Library to analyze and collect metrics on source code
https://mozilla.github.io/rust-code-analysis/
262 stars 46 forks source link

Make metric traits public #1046

Closed Luni-4 closed 1 year ago

Luni-4 commented 1 year ago

This PR makes all metric traits public. Code changes in this code have the following reasons:

We have decided to make all trait public in order to allow external users to compute metrics on single nodes since all arguments of the compute function are already public

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.01 :warning:

Comparison is base (92f00bc) 74.18% compared to head (9b6a26f) 74.18%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1046 +/- ## ========================================== - Coverage 74.18% 74.18% -0.01% ========================================== Files 64 64 Lines 15923 15915 -8 ========================================== - Hits 11813 11807 -6 + Misses 4110 4108 -2 ``` | [Impacted Files](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla) | Coverage Δ | | |---|---|---| | [src/metrics/abc.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvYWJjLnJz) | `94.37% <ø> (-0.01%)` | :arrow_down: | | [src/metrics/cognitive.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvY29nbml0aXZlLnJz) | `99.28% <ø> (-0.01%)` | :arrow_down: | | [src/metrics/cyclomatic.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvY3ljbG9tYXRpYy5ycw==) | `96.96% <ø> (+0.20%)` | :arrow_up: | | [src/metrics/exit.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvZXhpdC5ycw==) | `96.04% <ø> (+0.34%)` | :arrow_up: | | [src/metrics/halstead.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvaGFsc3RlYWQucnM=) | `93.38% <ø> (+0.18%)` | :arrow_up: | | [src/metrics/loc.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvbG9jLnJz) | `98.98% <ø> (+0.03%)` | :arrow_up: | | [src/metrics/mi.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvbWkucnM=) | `87.14% <ø> (ø)` | | | [src/metrics/nargs.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvbmFyZ3MucnM=) | `97.86% <ø> (ø)` | | | [src/metrics/nom.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3Mvbm9tLnJz) | `95.70% <ø> (ø)` | | | [src/metrics/npa.rs](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla#diff-c3JjL21ldHJpY3MvbnBhLnJz) | `97.25% <ø> (-0.01%)` | :arrow_down: | | ... and [4 more](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla) | | ... and [1 file with indirect coverage changes](https://codecov.io/gh/mozilla/rust-code-analysis/pull/1046/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mozilla)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

marco-c commented 1 year ago

I'm OK with these changes, except the last two commits that do Add a macro that implements in a fake way unimplemented code metrics traits. What is the benefit of this?

Luni-4 commented 1 year ago

Without that macro, we should add {} to the trait definition not to have a compiler error on the implementations. In this way instead:

marco-c commented 1 year ago

Are you sure we don't achieve the same two results if we stop at commit 232f502b60017f8eeea4a799434425470cc92e66? Isn't impl Abc for PythonCode {} already giving us an error if a language doesn't implement a trait?

Luni-4 commented 1 year ago

Are you sure we don't achieve the same two results if we stop at commit 232f502? Isn't impl Abc for PythonCode {} already giving us an error if a language doesn't implement a trait?

Yes, we already have an error but because of this line, since we require to have Abc trait implemented for PythonCode in order to allow Parser<PythonCode>. To better explain my position: using a trait with a default implementation does not create a clear documentation and leads to lose the distinctions among metric implementations for each language

marco-c commented 1 year ago

Could you show me the difference in generated documentation? I want to understand the result better before accepting the second part of the changes (I'm happy to merge the first part right away if you create that as a separate PR).

Luni-4 commented 1 year ago

Yep, loading the two documentations to show the main differences

Documentation without the second part

Screenshot from 2023-05-05 08-47-00

Documentation with the second part

Screenshot from 2023-05-05 08-49-40

The main differences are:

For what concerns the error I was talking about instead:

Commenting PythonCode implementation on master for abc metric

error[E0277]: the trait bound `langs::PythonCode: metrics::abc::Abc` is not satisfied
   --> src/macros.rs:225:45
    |
225 |               impl _private::CodeMetricsT for $code { }
    |                                               ^^^^^ the trait `metrics::abc::Abc` is not implemented for `langs::PythonCode`
    |
   ::: src/langs.rs:8:1
    |
8   | / mk_langs!(
9   | |     // 1) Name for enum
10  | |     // 2) Language description
11  | |     // 3) Display name
...   |
126 | |     )
127 | | );
    | |_- in this macro invocation
    |
    = help: the following other types implement trait `metrics::abc::Abc`:
              langs::CcommentCode
              langs::CppCode
              langs::JavaCode
              langs::JavascriptCode
              langs::KotlinCode
              langs::MozjsCode
              langs::PreprocCode
              langs::RustCode
            and 2 others
note: required by a bound in `CodeMetricsT`
   --> src/traits.rs:43:83
    |
42  |     pub trait CodeMetricsT:
    |               ------------ required by a bound in this trait
43  |         Cognitive + Cyclomatic + Exit + Halstead + NArgs + Loc + Nom + Mi + Wmc + Abc + Npm + Npa
    |                                                                                   ^^^ required by this bound in `CodeMetricsT`
    = note: this error originates in the macro `mk_code` which comes from the expansion of the macro `mk_langs` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `rust-code-analysis` due to previous error

Commenting PythonCode implementation on this PR for abc metric

error[E0277]: the trait bound `langs::PythonCode: Abc` is not satisfied
   --> src/macros.rs:207:38
    |
207 |                           let parser = $parser::new(source, &path, pr);
    |                                        ^ the trait `Abc` is not implemented for `langs::PythonCode`
    |
   ::: src/langs.rs:8:1
    |
8   | / mk_langs!(
9   | |     // 1) Name for enum
10  | |     // 2) Language description
11  | |     // 3) Display name
...   |
126 | |     )
127 | | );
    | |_- in this macro invocation
    |
    = help: the following other types implement trait `Abc`:
              langs::CcommentCode
              langs::CppCode
              langs::JavaCode
              langs::JavascriptCode
              langs::KotlinCode
              langs::MozjsCode
              langs::PreprocCode
              langs::RustCode
            and 2 others
note: required by a bound in `parser::Parser`
   --> src/parser.rs:34:11
    |
29  | pub struct Parser<
    |            ------ required by a bound in this struct
...
34  |         + Abc
    |           ^^^ required by this bound in `Parser`
    = note: this error originates in the macro `mk_action` which comes from the expansion of the macro `mk_langs` (in Nightly builds, run with -Z macro-backtrace for more info)

The main difference is that the error is now associated to the Parser struct and not to the CodeMetricsT trait implementation