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

Breaking change from 2.1.3 to 2.2.1 due to new RuleType bound on Error types #693

Closed
dennis-hamester opened this issue Aug 14, 2022 · 9 comments · Fixed by #694
Closed
Labels

Comments

@dennis-hamester
Copy link

Describe the bug
In commit 5b807be, Error<R> was changed to Error<R: RuleType>, which is a breaking change. As far as I can see, this also extends to ErrorVariant.

To Reproduce
Compiles with Pest 2.1.3, but not with 2.2.1:

fn inspect_pest_error<R>(err: &pest::error::Error<R>) {
}

To make it compile with 2.2.1, a bound on R must be added:

fn inspect_pest_error<R: pest::RuleType>(err: &pest::error::Error<R>) {
}

Additional context
For me personally, this is not a huge deal. My code didn't require any bigger changes to make it compile again. But it can certainly be surprising.

@tomtau
Copy link
Contributor

tomtau commented Aug 14, 2022

@dennis-hamester thanks for the report.

@pest-parser/maintainers @pest-parser/triage any thoughts on how to resolve it? Revert that commit and release a new version? Or keep it as it is and be more careful next time (E.g. with https://crates.io/crates/cargo-semver-checks)? Or release it as v3.0.0 (despite it not being the "mythical pest3" with the new grammar etc.)?

@nathanielknight
Copy link

I think leaving it as is and adding a check is probably reasonable, perhaps with a note on the release?

@glyn
Copy link

glyn commented Aug 15, 2022

I could go either way. I agree it's not a huge deal. Maybe a comment on the offending type would help?

Note that, ultimately, we'll probably need to go to v3.0.0 before "mythical pest3", so the sooner we get that over and done with, the better?

@CAD97
Copy link
Contributor

CAD97 commented Aug 15, 2022

This is simple enough to fix, though. Just remove the trait bound from the type. Not bounding on the struct is best practice.

@CAD97
Copy link
Contributor

CAD97 commented Aug 16, 2022

5b807be also removes the fmt::Display impl for cfg(not(feature = "std")), so I think #694 is warranted to keep the best-effort no-std support.

@tomtau
Copy link
Contributor

tomtau commented Aug 16, 2022

The patch looks all right, so I guess we'll yank 2.2.1 and release 2.2.2 after merging that?

@CAD97
Copy link
Contributor

CAD97 commented Aug 16, 2022

👍 sounds accurate to me

@tomtau
Copy link
Contributor

tomtau commented Aug 16, 2022

Actually, the release may be 2.3.0 due to new features since 2.2.x. anyway, probably good to run cargo semver-checks check-release against 2.1.3 before going with it

@tomtau
Copy link
Contributor

tomtau commented Aug 19, 2022

FYI I haven't checked all crates, but pest on the current branch seems fine:

$ cargo semver-checks check-release --current current-all-features.json --baseline 2.1.3-all-features.json 
    Starting 18 checks, version 2.1.3 -> 2.2.1 (minor change)
        PASS [   0.029s]       major        auto_trait_impl_removed
        PASS [   0.018s]       major        derive_trait_impl_removed
        PASS [   0.002s]       major        enum_missing
        PASS [   0.000s]       major        enum_repr_c_removed
        PASS [   0.000s]       major        enum_repr_int_changed
        PASS [   0.000s]       major        enum_repr_int_removed
        PASS [   0.005s]       major        enum_variant_added
        PASS [   0.005s]       major        enum_variant_missing
        PASS [   0.001s]       major        function_missing
        PASS [   0.019s]       major        inherent_method_missing
        PASS [   0.005s]       major        sized_impl_removed
        PASS [   0.000s]       major        struct_marked_non_exhaustive
        PASS [   0.002s]       major        struct_missing
        PASS [   0.001s]       major        struct_pub_field_missing
        PASS [   0.000s]       major        struct_repr_c_removed
        PASS [   0.000s]       major        struct_repr_transparent_removed
        PASS [   0.000s]       major        unit_struct_changed_kind
        PASS [   0.005s]       major        variant_marked_non_exhaustive
     Summary [   0.092s] 18 checks run: 18 passed, 0 skipped
$ cargo semver-checks check-release --current current-default-features.json --baseline 2.1.3-default-features.json 
    Starting 18 checks, version 2.1.3 -> 2.2.1 (minor change)
        PASS [   0.023s]       major        auto_trait_impl_removed
        PASS [   0.016s]       major        derive_trait_impl_removed
        PASS [   0.002s]       major        enum_missing
        PASS [   0.000s]       major        enum_repr_c_removed
        PASS [   0.001s]       major        enum_repr_int_changed
        PASS [   0.000s]       major        enum_repr_int_removed
        PASS [   0.004s]       major        enum_variant_added
        PASS [   0.004s]       major        enum_variant_missing
        PASS [   0.001s]       major        function_missing
        PASS [   0.015s]       major        inherent_method_missing
        PASS [   0.005s]       major        sized_impl_removed
        PASS [   0.000s]       major        struct_marked_non_exhaustive
        PASS [   0.002s]       major        struct_missing
        PASS [   0.001s]       major        struct_pub_field_missing
        PASS [   0.000s]       major        struct_repr_c_removed
        PASS [   0.000s]       major        struct_repr_transparent_removed
        PASS [   0.000s]       major        unit_struct_changed_kind
        PASS [   0.004s]       major        variant_marked_non_exhaustive
     Summary [   0.078s] 18 checks run: 18 passed, 0 skipped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants