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

library: return required update for each crate #398

Merged
merged 44 commits into from
May 7, 2023

Conversation

MarcoIeni
Copy link
Contributor

@MarcoIeni MarcoIeni commented Feb 23, 2023

First step to add more info to the Report struct.
This is a small improvement towards #396

Probably in the future you don't even want to store the required_bump of each crate, because you could deduce it by iterating on the crate report errors. However, I decided to store it because it seemed like a good first iteration.

@MarcoIeni MarcoIeni marked this pull request as ready for review February 23, 2023 22:31
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Seems mostly okay, just a couple of minor points.

src/lib.rs Outdated
Comment on lines 407 to 414
let success = all_outcomes
.into_iter()
.fold_ok(true, std::ops::BitAnd::bitand)?;

Ok(Report { success })
Ok(Report {
crate_reports: all_outcomes?,
})
Copy link
Owner

Choose a reason for hiding this comment

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

This has a subtle behavior change: the old code runs all the checks even if some of them error out because it merges the Result values late, whereas the new code relies on .collect() which will bail at the first Err.

Since the check output is user-visible, this seems net-negative to me? But I could probably be persuaded otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I addressed this in 1445fb6

Unfortunately using a BTreeMap instead of a Vec resulted in uglier code :/

src/lib.rs Outdated Show resolved Hide resolved
@MarcoIeni
Copy link
Contributor Author

I suggest to view the changes from VSCode GitHub plugin or another tool. GitHub diff is confused by the different indentation I think.

@MarcoIeni MarcoIeni requested a review from obi1kenobi February 25, 2023 10:00
src/lib.rs Outdated
Comment on lines 323 to 324
// Create a report for each crate.
let all_outcomes: anyhow::Result<BTreeMap<String, anyhow::Result<CrateReport>>> =
Copy link
Owner

Choose a reason for hiding this comment

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

Two concerns here:

  • I feel like this code has gotten a bit too complex and intricate. It took me probably 10-15 minutes to figure out what exactly is going on, and that's when I was already familiar with what this code was doing. We're adding significant new complexity here — do you think there's a reasonable refactor we could do to improve the readability in the process?
  • The Result<Map<String, Result<CrateReport>>> type is quite confusing, especially because it isn't documented what becomes an inner error vs an outer error. It seems that failure to generate the rustdoc becomes an outer error? Is that right? And if so, should that be the case? I'm genuinely not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're adding significant new complexity here

I agree. But trying to address this is not easy, because you can't simply ? everywhere when you have a BTreeMap.

do you think there's a reasonable refactor we could do to improve the readability in the process?

I will try to think about it and change that big Result type.

@MarcoIeni MarcoIeni requested a review from obi1kenobi March 20, 2023 22:06
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Overall looks quite good! Sorry for the delay, I just got home from a family vacation.

I just have a small concern around naming / docs in the new CrateReport type. Otherwise this should be easy to merge and I can release a new version as soon as that happens.

src/lib.rs Outdated
Comment on lines 330 to 337
ScopeMode::DenyList(_) =>
match &self.current.source {
RustdocSource::Rustdoc(_) =>
vec!["the-name-doesnt-matter-here".to_string()],
_ => panic!("couldn't deduce crate name, specify one through the package allow list")
}
ScopeMode::AllowList(lst) => lst.clone(),
};
Copy link
Owner

Choose a reason for hiding this comment

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

This change seems to be only an indent and not necessary. Let's avoid it?

src/lib.rs Outdated
#[non_exhaustive]
#[derive(Debug)]
pub struct CrateReport {
required_bump: Option<RequiredSemverUpdate>,
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a bit confused here.

required_bump sounds like "what is the minimum bump that satisfies semver" but based on the Option type and new_successful() function, it seems like it might actually be "this change is semver-incompliant and needs to be instead"?

Is my understanding correct? If so, it would be good to do one more editing pass on the naming and docs for the sake of clarity, since this is public API after all.

Copy link
Contributor Author

@MarcoIeni MarcoIeni Apr 9, 2023

Choose a reason for hiding this comment

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

I took the "required" word of required_bump from the stdout of the binary.
image
So the naming should be ok, I guess 🤔 but I'll add more comments :)
I agree that the Option makes it confusing. I changed it to "Patch"

@MarcoIeni
Copy link
Contributor Author

Can you help me understand why CI is failing?

@obi1kenobi
Copy link
Owner

Sorry about the failing CI, @mgr0dzicki was tweaking something in the target project that corresponds to the failing job. The test case here was written quite defensively (in a good way!) but that just means it "noticed" the change. It just needs a small update and I'll merge the fix right away.

@obi1kenobi
Copy link
Owner

It's actually quite concerning that the Windows equivalent of the job passed while the regular Linux one failed. They are testing the same thing, and it makes me feel that the Windows one isn't actually testing it correctly...

@MarcoIeni MarcoIeni requested a review from obi1kenobi April 25, 2023 21:01
src/lib.rs Outdated
Comment on lines 438 to 440
/// Minimum bump required to respect semver.
/// For example, if the crate contains breaking changes, this is [`ReleaseType::Major`].
/// If no bump is required, this is [`Option::None`].
Copy link
Owner

Choose a reason for hiding this comment

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

Is this accurate?

Say we ran a semver-check over a crate where the detected bump is ActualSemverUpdate::Major. Would required_bump actually be Some(ReleaseType::Major)?

If no bump is required, the value will be None, but is that the only time? I think there are other situations where the value might be None.

Let's back this up with some tests? Consider making a new integration test file in the tests/ directory to pin down the behavior here.

@MarcoIeni
Copy link
Contributor Author

The tests I added on 6cf3622 works locally.
Can you help me understand how to pass CI?

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

The test crate's name in Cargo.toml didn't match its directory, which the test suite requires to run. I'm guessing your locally-generated rustdoc contains some older version of the file, and if you wipe your localdata directory and regenerate all the rustdoc, you'll see the same error locally.

test_crates/trait_missing_with_major_bump/new/Cargo.toml Outdated Show resolved Hide resolved
test_crates/trait_missing_with_major_bump/old/Cargo.toml Outdated Show resolved Hide resolved
MarcoIeni and others added 2 commits May 2, 2023 21:34
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
@MarcoIeni
Copy link
Contributor Author

Deleting the localdata folder helped. I was seeing weird errors like:


---- query::tests_lints::function_const_removed stdout ----
thread 'query::tests_lints::function_const_removed' panicked at 'Once instance has previously been poisoned', /Users/marcoieni/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lazy_static-1.4.0/src/inline_lazy.rs:30:16

Now I see the same error locally 👍
Consider adding instructions to delete the localdata folder to this error: Also, remember about running ./scripts/regenerate_test_rustdocs.sh when needed.

@MarcoIeni
Copy link
Contributor Author

MarcoIeni commented May 2, 2023

can you help me fix this last error? 😅
I don't know why the test fails

@obi1kenobi
Copy link
Owner

obi1kenobi commented May 2, 2023 via email

@MarcoIeni
Copy link
Contributor Author

I fixed that error thanks to your previous help 👍
now there's another error:

---- query::tests_lints::trait_unsafe_removed stdout ----
thread 'query::tests_lints::trait_unsafe_removed' panicked at '
Query trait_unsafe_removed produced incorrect output (./src/lints/trait_unsafe_removed.ron).

Expected output (./test_outputs/trait_unsafe_removed.output.ron):
{
    "./test_crates/trait_missing/": [
        {
            "name": String("TraitStopsBeingUnsafe"),
            "path": List([
                String("trait_missing"),
                String("TraitStopsBeingUnsafe"),
            ]),
            "span_begin_line": Uint64(6),
            "span_filename": String("src/lib.rs"),
            "visibility_limit": String("public"),
        },
    ],
    "./test_crates/trait_unsafe_removed/": [
        {
            "name": String("TraitBecomesSafe"),
            "path": List([
                String("trait_unsafe_removed"),
                String("TraitBecomesSafe"),
            ]),
            "span_begin_line": Uint64(2),
            "span_filename": String("src/lib.rs"),
            "visibility_limit": String("public"),
        },
    ],
}

Actual output:
{
    "./test_crates/trait_missing/": [
        {
            "name": String("TraitStopsBeingUnsafe"),
            "path": List([
                String("trait_missing"),
                String("TraitStopsBeingUnsafe"),
            ]),
            "span_begin_line": Uint64(6),
            "span_filename": String("src/lib.rs"),
            "visibility_limit": String("public"),
        },
    ],
    "./test_crates/trait_missing_with_major_bump/": [
        {
            "name": String("TraitStopsBeingUnsafe"),
            "path": List([
                String("trait_missing_with_major_bump"),
                String("TraitStopsBeingUnsafe"),
            ]),
            "span_begin_line": Uint64(6),
            "span_filename": String("src/lib.rs"),
            "visibility_limit": String("public"),
        },
    ],
    "./test_crates/trait_unsafe_removed/": [
        {
            "name": String("TraitBecomesSafe"),
            "path": List([
                String("trait_unsafe_removed"),
                String("TraitBecomesSafe"),
            ]),
            "span_begin_line": Uint64(2),
            "span_filename": String("src/lib.rs"),
            "visibility_limit": String("public"),
        },
    ],
}

Note that the individual outputs might have been deliberately reordered.

Also, remember about running ./scripts/regenerate_test_rustdocs.sh when needed.
', src/query.rs:400:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/071f14baae931e17a5a99366bf216e76384cc4f6/library/std/src/panicking.rs:[578](https://github.com/obi1kenobi/cargo-semver-checks/actions/runs/4865060452/jobs/8674882462#step:9:579):5
   1: core::panicking::panic_fmt
             at /rustc/071f14baae931e17a5a99366bf216e76384cc4f6/library/core/src/panicking.rs:67:14
   2: cargo_semver_checks::query::tests::check_query_execution
             at ./src/query.rs:400:13
   3: cargo_semver_checks::query::tests_lints::trait_unsafe_removed
             at ./src/query.rs:442:21
   4: cargo_semver_checks::query::tests_lints::trait_unsafe_removed::{{closure}}
             at ./src/query.rs:441:28
   5: core::ops::function::FnOnce::call_once
             at /rustc/071f14baae931e17a5a99366bf216e76384cc4f6/library/core/src/ops/function.rs:250:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/071f14baae931e17a5a99366bf216e7[638](https://github.com/obi1kenobi/cargo-semver-checks/actions/runs/4865060452/jobs/8674882462#step:9:639)4cc4f6/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    query::tests_lints::trait_missing
    query::tests_lints::trait_unsafe_added
    query::tests_lints::trait_unsafe_removed

test result: FAILED. 61 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.02s

@obi1kenobi
Copy link
Owner

obi1kenobi commented May 2, 2023 via email

@MarcoIeni
Copy link
Contributor Author

Thanks, I think I have done it 👍
Please, consider using something like insta or expect_test to improve the developer experience.

@obi1kenobi
Copy link
Owner

I'd happily look at a PR for either insta or expect_test, whichever you think is better! Last I looked, neither was a clear improvement since our tests aren't just snapshot but also require running the rustdoc generation script first, so it seemed like any effort dedicated to making the experience better would be equally (or better) spent on making the existing approach better.

Would love to be proven wrong on this!

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

The design looks good! Just a few tweaks to doc comments and this is ready to merge 🎉

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 450 to 458
match self.detected_bump {
// If user bumped the major version, any breaking change is accepted.
ActualSemverUpdate::Major => true,
ActualSemverUpdate::Minor => {
matches!(required_bump, ReleaseType::Minor | ReleaseType::Patch)
}
ActualSemverUpdate::Patch | ActualSemverUpdate::NotChanged => {
required_bump == ReleaseType::Patch
}
Copy link
Owner

Choose a reason for hiding this comment

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

Does this need updating for the changes to the semantics of the struct's fields? It seems to me like some of these could be assert instead of comparisons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I think it should never happen that the update of detected_bump is the same of required_bump, right?
Maybe we can add some warnings in this case. Or do you prefer to write an assert? Sounds scary 😅

Copy link
Owner

Choose a reason for hiding this comment

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

Let's assert. If our design behaves unexpectedly, it should fail loudly and quickly so we can figure out the failure mode and fix it.

I think required_bump should never be equal to or lesser than detected_bump in this design. If we're wrong about that, there's some serious issue we've overlooked and the outcome shouldn't be trusted anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the assert in 9cf72ee

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/query.rs Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure this file is necessary? I don't recall the exact setup of the test suite, but I think the output files are per-lint, not per-test-crate, and trait_missing_with_major_bump is a test crate, not a lint. So that means its data can appear in lints' output files, but I don't think anything will read this file since it doesn't correspond to any lint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 54042b5

MarcoIeni and others added 8 commits May 3, 2023 18:29
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks great, let's ship it! Thanks for all the work on this, and sorry reviews took a while. I'm hoping my schedule calms down a bit going forward.

Also -- this found an ICE in nightly Rust. @MarcoIeni would you care to report it?

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks great, let's ship it! Thanks for all the work on this, and sorry reviews took a while. I'm hoping my schedule calms down a bit going forward.

Also -- this found an ICE in nightly Rust. @MarcoIeni would you care to report it?

@obi1kenobi obi1kenobi merged commit 904c914 into obi1kenobi:main May 7, 2023
@MarcoIeni
Copy link
Contributor Author

would you care to report it?

somebody already reported it in rust-lang/rust#111320

I'm hoping my schedule calms down a bit going forward.

No problem at all, thanks for the very detailed review :)

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.

2 participants