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

derive: fix compilation error if user define its own Result type #20

Merged

Conversation

abernardeau-wallix
Copy link
Contributor

When using the TryFromValue and TryToValue macros in a file where a custom Result is defined (in my case type Result<T> = std::result::Result<T, Box<dyn Error + Send + Sync>>;) the macro doesn't work anymore and create a compilation error.

The Result seems to be common pattern and I think it should not break the macrso (https://doc.rust-lang.org/rust-by-example/error/result/result_alias.html )

@@ -145,7 +145,7 @@ pub fn try_from_value(input: TokenStream) -> TokenStream {

let impl_block = quote! {
impl #impl_generics #dxr::TryFromValue for #name #ty_generics #where_clause {
fn try_from_value(value: &#dxr::Value) -> Result<#name #ty_generics, #dxr::DxrError> {
fn try_from_value(value: &#dxr::Value) -> std::result::Result<#name #ty_generics, #dxr::DxrError> {
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this with ::std::result::Result, similar to other std imports?

@@ -258,7 +258,7 @@ pub fn try_to_value(input: TokenStream) -> TokenStream {

let impl_block = quote! {
impl #impl_generics #dxr::TryToValue for #name #ty_generics #where_clause {
fn try_to_value(&self) -> Result<#dxr::Value, #dxr::DxrError> {
fn try_to_value(&self) -> std::result::Result<#dxr::Value, #dxr::DxrError> {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing applies here.

Copy link
Member

@decathorpe decathorpe left a comment

Choose a reason for hiding this comment

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

Thank you, this looks mostly good to me, with two minor issues.

(Imagine somebody redefining std, it would cause the same problem as the custom Result.)

@decathorpe
Copy link
Member

Great, thanks! Let's see what the CI says.

@abernardeau-wallix
Copy link
Contributor Author

abernardeau-wallix commented Jul 11, 2024

I updated the PR with your wise remarks.

(Imagine somebody redefining std, it would cause the same problem as the custom Result.)

This is a terrifying thought 😱.

@abernardeau-wallix
Copy link
Contributor Author

It seems that the nightly jobs is failing to the time crate used here not being compatible with the nightly toolchain.
Locally running a cargo update seems to be fixing the issue but it updates a bunch of dependencies.

@decathorpe
Copy link
Member

Yeah, the failure with time is unrelated. I've pushed a change that updates the lockfile. Once the CI is green here, can you rebase your PR? https://github.com/ironthree/dxr/actions/runs/9905812150

@decathorpe
Copy link
Member

Awesome. Thank you for the contribution!

@decathorpe decathorpe merged commit 79512f5 into ironthree:main Jul 12, 2024
5 checks passed
@decathorpe
Copy link
Member

Do you need a new release in the v0.6 branch with this fix? Or would it be OK if this just ends up in 0.7.0?

@abernardeau-wallix
Copy link
Contributor Author

If it's not too much work I would be happy to have a release with the fix but I can implement a workaround in my code and can wait for the 0.7.0 version.
Thank you for the nice library and your reactivity.

@abernardeau-wallix abernardeau-wallix deleted the fix-custom-result-type branch July 12, 2024 12:40
@decathorpe
Copy link
Member

I'm working on some refactors for 0.7.0 still, so I'll cherry-pick your change onto the v0.6 release series and publish a new version.

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