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

Handle expansion failure #66

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Handle expansion failure #66

wants to merge 11 commits into from

Conversation

eupn
Copy link
Owner

@eupn eupn commented Nov 1, 2021

Closes #65.

Please check with this branch, @Emoun. Should you get no issues, I'll merge this and release a new version.

@eupn eupn self-assigned this Nov 1, 2021
@Emoun
Copy link
Contributor

Emoun commented Nov 1, 2021

Thanks for working on #65. I'll test out this branch as soon as I can and report back here.

@Emoun
Copy link
Contributor

Emoun commented Nov 3, 2021

I found the following problems with expand_fail:

  • If there is no .expanded file, the one created automatically is empty. I would have expected it to contain the error message from the macro that panicked.
  • If there is a .expanded file, I always just get a <file path> - different! error showing me the contents of the .expanded file but nothing else. expand_fail always seems to fail, even when my expected error is identical to the actual error.

@eupn
Copy link
Owner Author

eupn commented Nov 3, 2021

@Emoun can you show me the code for failing macro? I'll add this macro as an example/test if it's small enough. Does manually calling cargo expand on this macro produce an error (with a non-zero exit code)?

@Emoun
Copy link
Contributor

Emoun commented Nov 3, 2021

I'm trying things on duplicate, but the specific call I'm trying is:

#[duplicate::duplicate(refs(T); [& T]; [T])]
fn from(x: refs(Bits<1, false>)) -> bool {
	x.value == 1
}

Expanding this will panic in duplicate because of wrong use.
You can try it out on the branch i'm using. The specific call is at tests/default_features/test/parameters_not_encapsulated.rs and the use of macrotest is at tests/default_features/mod.rs:46

I tried running cargo expand on a different project that used duplicate, where I added the wrong code, and I can see that cargo expand does return 0 as an exit code, even though it also shows that the macro panicked.

@Emoun
Copy link
Contributor

Emoun commented Nov 3, 2021

Btw, I have cargo-expand v1.0.9

@eupn
Copy link
Owner Author

eupn commented Nov 3, 2021

@Emoun, thank you! I'll explore this issue further.

@eupn
Copy link
Owner Author

eupn commented Nov 5, 2021

@Emoun Indeed, cargo outputs expansion errors (such as proc macro panics) into STDERR while returning zero exit code. I've made some changes to handle this, could you check again?

Potentially will close #67.

@Emoun
Copy link
Contributor

Emoun commented Nov 5, 2021

I'll have a look at it this weekend and report back.

@Emoun
Copy link
Contributor

Emoun commented Nov 6, 2021

I've given the update a try and, in short, it seems for me to work well enough to be usable for my use cases.

I found one issue that I think is critical, though I specifically don't use macrotest in this way. As such, it is not blocking to me specifically:

  • expand_fail: When I have a macro that succeeds, but I expect it to fail, and there is no .expanded file. A new .expanded file is created, with the expansion result, and success is reported. This is problematic since if you run the tests again, you will again get a success (since the contents of the .expanded match the output of the macro) however, the macro is not panicking, as it should when using expand_fail. This can trip-up anyone not paying attention to what the contents of the generated .expanded file are.

Below are some of the irregularities I have found. While I think they should be addressed, they are not blocking for my use cases and mostly affect usability.

  • expand_fail: When I have a macro that succeeds, but I expect it to fail, the error message I get gives me the difference between my expected error message, and the actual expansion output (i.e. a <file path> - Different! error). I would instead expect to get a message telling me the macro was expected to panic but didn't. E.g. <file path> - Didn't Panic!. I'm ambivalent to whether this could be followed by the macros actual expansion output.
  • expand: When run on a macro that panics, it correctly reports an error but the report doesn't include the initial <file path> - <error type>! message signifying what test failed. I would prefer if each panic report starts with (e.g.) <file path> - Unexpected Panic! followed by the macro's own error message.
  • expand_without_refresh/expand_without_refresh: When run without any .expanded file being present, correctly throws an error but there is no message. I would expect a message like <file path> - Missing '.expanded.rs' file!.

@Emoun
Copy link
Contributor

Emoun commented Feb 12, 2022

@eupn, do you have an ETA on when this might get merged and an eventual new release?
This PR and the changes currently in master would be really nice to get into a new release.

Thank you for all your hard work on this nice crate.

@eupn
Copy link
Owner Author

eupn commented Feb 12, 2022

@Emoun thanks for kind words! I'll try my best to publish a new release next week.

@Emoun
Copy link
Contributor

Emoun commented Jul 19, 2022

@eupn, sorry to bother again, but I'm still very much interested in this PR getting merged/released even without the last few fixes I highlighted above. I hope you can find some time to look into it.

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.

Feature Request: Expand fail
2 participants