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.

Sign up for GitHub

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

Jump to bottom

Add support to return value in StableMIR interface and not crash due to compilation error #115397

Merged
merged 4 commits into from
Sep 5, 2023
Merged

Add support to return value in StableMIR interface and not crash due to compilation error #115397

merged 4 commits into from
Sep 5, 2023

Conversation

celinval
Copy link
Contributor

Invoking StableMir::run() on a crate that has any compilation error was crashing the entire process. Instead, return a CompilerError so the user knows compilation did not succeed. I believe ICE will also be converted to CompilerError.

I'm also adding a possibility for the callback to return a value. I think it will be handy for users (at least it was for my current task of implementing a tool to validate stable-mir). However, if people disagree, I can remove that.

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

@celinval
Copy link
Contributor Author

r? @oli-obk @ouz-a

@rustbot rustbot assigned oli-obk and unassigned TaKO8Ki Aug 31, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ouz-a
Copy link
Contributor

ouz-a commented Aug 31, 2023 •
edited
Loading

Everything looks good, but maybe returning CompilerError + some error message like "Crate you are trying to compile can't be compiled due to ICE" or something would provide better user experience(It would for me).

@bors
Copy link
Contributor

bors commented Aug 31, 2023

☔ The latest upstream changes (presumably #115346) made this pull request unmergeable. Please resolve the merge conflicts.

@celinval
Copy link
Contributor Author

Everything looks good, but maybe returning CompilerError + some error message like "Crate you are trying to compile can't be compiled due to ICE" or something would provide better user experience(It would for me).

The compiler will still print its regular messages, so the ICE message should still print. I believe the panic hook will also run before we catch the error.

If we were to differentiate failure reasons in our APIs, we could use an enum instead of a struct, and have an ICE variant. I can do that now or we can do it later. What do you think?

@ouz-a
Copy link
Contributor

ouz-a commented Aug 31, 2023 •
edited
Loading

The compiler will still print its regular messages, so the ICE message should still print. I believe the panic hook will also run before we catch the error.

If we were to differentiate failure reasons in our APIs, we could use an enum instead of a struct, and have an ICE variant. I can do that now or we can do it later. What do you think?

Yeah compiler will definitely print out the regular ICE message but as you suggested expecting these with enum variants will feel more right.

@celinval celinval mentioned this pull request Sep 1, 2023
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2023

Please add a test for these code paths, they seem easy to regress

@bors
Copy link
Contributor

bors commented Sep 2, 2023

☔ The latest upstream changes (presumably #115469) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 4, 2023

@bors delegate+

r=me after a rebase

@bors
Copy link
Contributor

bors commented Sep 4, 2023

✌️ @celinval, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

@celinval
Copy link
Contributor Author

celinval commented Sep 4, 2023

I just realized I forgot to add the new test file to git. ☺️

@celinval
Adjust StableMIR interface to return and not crash
2db01be 
Invoking StableMir::run() on a crate that has any compilation error was
crashing the entire process. Instead, return a `CompilerError` so the
user knows compilation did not succeed.

I believe ICE will also be converted to `CompilerError`.

I'm also adding a return value to the callback, because I think it will
be handy for users (at least it was for my current task of implementing
a tool to validate stable-mir). However, if people disagree,
I can remove that.
@celinval
Diferentiate between ICE and compilation error
3b01f65
@celinval
SMIR: Allow users to pick if compilation continues
1a8a5d0 
Currently we stop compilation, but some users might want to keep going.
This is needed for us to test against rustc tests. Other tools, such as
Kani, also implements parts of their logic as a backend so it is
important for compilation to continue.
@celinval
Add tests and use ControlFlow
d10d829
@celinval
Copy link
Contributor Author

celinval commented Sep 5, 2023

@bors r=@oli-obk

@bors
Copy link
Contributor

bors commented Sep 5, 2023

📌 Commit d10d829 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 5, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2023
@bors
Auto merge of rust-lang#115579 - matthiaskrgr:rollup-n4ijxl7, r=matth…
f91c53d 
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#114794 (clarify safety documentation of ptr::swap and ptr::copy)
 - rust-lang#115397 (Add support to return value in StableMIR interface and not crash due to compilation error)
 - rust-lang#115559 (implied bounds: do not ICE on unconstrained region vars)

Failed merges:

 - rust-lang#115532 (Implement SMIR generic parameter instantiation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c7d5c12 into rust-lang:master Sep 5, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
7 participants
@celinval @rustbot @rust-log-analyzer @ouz-a @bors @oli-obk @TaKO8Ki