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

Fix misc printing issues in emit=stable_mir #118364

Closed
wants to merge 2 commits into from

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Nov 27, 2023

Previously our } closed early and looked bit weird, now it closes when function call ends, and previously Adt printing was too verbose now it's similar to mir output

r? @celinval

@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 Nov 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

@compiler-errors
Copy link
Member

compiler-errors commented Nov 27, 2023

I've asked before but I will ask again -- is it possible for you to prioritize working on some sort of test?

It's kinda sketchy to be putting up PRs like this with absolutely no evidence that it's doing anything.

@ouz-a
Copy link
Contributor Author

ouz-a commented Nov 27, 2023

I've asked before but I will ask again -- is it possible for you to prioritize working on some sort of test?

It's kinda sketchy to be putting up PRs like this with absolutely no evidence that it's doing anything.

Alright will start working on a test immediately

@ouz-a
Copy link
Contributor Author

ouz-a commented Nov 27, 2023

Opened PR for the test #118375

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 27, 2023
…=compiler-errors

Add -Zunpretty=stable-mir output test

As strongly suggested here rust-lang#118364 (comment) this adds output test for `-Zunpretty=stable-mir`, added test shows almost all the functionality of the current printer.

r? `@compiler-errors`
@compiler-errors
Copy link
Member

Do you mind rebasing this on top of #118375? Then you can bless the tests in your commit and see the output changes due to these tweaks.

@ouz-a ouz-a force-pushed the misc_emit_stable_fixes branch from a67486e to fcc7002 Compare November 27, 2023 18:08
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Nov 27, 2023
@ouz-a
Copy link
Contributor Author

ouz-a commented Nov 27, 2023

Do you mind rebasing this on top of #118375? Then you can bless the tests in your commit and see the output changes due to these tweaks.

Just rebased and run bless

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I think this is a good opportunity to make more tweaks to the output since I can now see what's broken 😃 thanks for that

Also, plz squash the "rebase" commit into the "fix" commit.

tests/ui/stable-mir-print/basic_function.stdout Outdated Show resolved Hide resolved
tests/ui/stable-mir-print/basic_function.stdout Outdated Show resolved Hide resolved
tests/ui/stable-mir-print/basic_function.stdout Outdated Show resolved Hide resolved
tests/ui/stable-mir-print/basic_function.stdout Outdated Show resolved Hide resolved
@ouz-a
Copy link
Contributor Author

ouz-a commented Nov 27, 2023

I think this is a good opportunity to make more tweaks to the output since I can now see what's broken 😃 thanks for that

Also, plz squash the "rebase" commit into the "fix" commit.

No problem, it's better for someone other than me to also look at this and have some opinion about it 😄

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 27, 2023
…=compiler-errors

Add -Zunpretty=stable-mir output test

As strongly suggested here rust-lang#118364 (comment) this adds output test for `-Zunpretty=stable-mir`, added test shows almost all the functionality of the current printer.

r? ``@compiler-errors``
@ouz-a ouz-a force-pushed the misc_emit_stable_fixes branch from fcc7002 to bab21f3 Compare November 28, 2023 07:29
@rust-log-analyzer

This comment has been minimized.

@ouz-a ouz-a force-pushed the misc_emit_stable_fixes branch from bab21f3 to 844c493 Compare November 28, 2023 07:54
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

I noticed that the successor for Goto terminators is missing the bb in the label. For example, we're printing something like:

goto -> 5

when I expected:

goto -> bb5

compiler/stable_mir/src/mir/pretty.rs Outdated Show resolved Hide resolved
compiler/stable_mir/src/mir/pretty.rs Outdated Show resolved Hide resolved
@ouz-a ouz-a force-pushed the misc_emit_stable_fixes branch from ee889d9 to 88df8fa Compare December 1, 2023 06:18
@bors
Copy link
Contributor

bors commented Dec 2, 2023

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2023
…=celinval

Add -Zunpretty=stable-mir output test

As strongly suggested here rust-lang#118364 (comment) this adds output test for `-Zunpretty=stable-mir`, added test shows almost all the functionality of the current printer.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2023
…elinval

Add -Zunpretty=stable-mir output test

As strongly suggested here rust-lang#118364 (comment) this adds output test for `-Zunpretty=stable-mir`, added test shows almost all the functionality of the current printer.

r? `@compiler-errors`
@ouz-a ouz-a force-pushed the misc_emit_stable_fixes branch from 88df8fa to 7e3c287 Compare December 14, 2023 15:19
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2023
…elinval

Add -Zunpretty=stable-mir output test

As strongly suggested here rust-lang#118364 (comment) this adds output test for `-Zunpretty=stable-mir`, added test shows almost all the functionality of the current printer.

r? `@compiler-errors`
@bors
Copy link
Contributor

bors commented Dec 14, 2023

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

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 15, 2023
Add -Zunpretty=stable-mir output test

As strongly suggested here rust-lang/rust#118364 (comment) this adds output test for `-Zunpretty=stable-mir`, added test shows almost all the functionality of the current printer.

r? `@compiler-errors`
@ouz-a ouz-a force-pushed the misc_emit_stable_fixes branch from 7e3c287 to dc78b1c Compare December 17, 2023 06:59
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

This is looking so much better. I'm hoping we can merge this after the next revision. Btw, have you compared the expected file with the internal MIR dump? It might be helpful to spot what's missing or what's wrong.

_2 = 1 Add const 1_i32
assert(!move _2 bool),"attempt to compute `{} + {}`, which would overflow", 1, const 1_i32) -> [success: bb1, unwind continue]
_2 = CheckedAdd(_1, const 1_i32)
assert(!move _2 bool),"attempt to compute `{} + {}`, which would overflow", _1, const 1_i32) -> [success: bb1, unwind continue]
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's this bool coming from?

bb0: {
_3 = refShared1
_3 = &1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be _1?

_5 = refMut {
kind: TwoPhaseBorrow,
}2
_5 = &mut 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's _5 declaration?

}
fn bar(_0: &mut std::vec::Vec) -> std::vec::Vec {
let mut _0: std::vec::Vec;
let mut _1: &std::vec::Vec;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing _1 initialization?

@ouz-a
Copy link
Contributor Author

ouz-a commented Dec 21, 2023

This is looking so much better. I'm hoping we can merge this after the next revision. Btw, have you compared the expected file with the internal MIR dump? It might be helpful to spot what's missing or what's wrong.

Will go trough another pass and then compare with mir output

@bors
Copy link
Contributor

bors commented Jan 11, 2024

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

@apiraino
Copy link
Contributor

apiraino commented Feb 1, 2024

@ouz-a anything interesting to report w.r.t. to your prev. comment? No hurry, I'll just flip the review switch

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2024
@ouz-a
Copy link
Contributor Author

ouz-a commented Feb 1, 2024

@ouz-a anything interesting to report w.r.t. to your prev. comment? No hurry, I'll just flip the review switch

@rustbot author

I been meaning to look at this for some time but I been extremely busy with life/job in past couple months 😓

@celinval
Copy link
Contributor

Hey @ouz-a do you mind if I take a stab at this?

@ouz-a
Copy link
Contributor Author

ouz-a commented Mar 18, 2024

Hey @ouz-a do you mind if I take a stab at this?

Yeah, go ahead.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2024
…rrors

Fix misc printing issues in emit=stable_mir

Trying to continue the work that `@ouz-a` started here: rust-lang#118364

Few modifications beyond fixes:
1. I made the `pretty_*` functions private.
2. I added a function to print the instance body
3. Changed a bunch of signatures to write to the writer directly.
4. Added a function to translate the place to its internal representation, so we could use the internal debug implementation.
5. Also removed `pretty_ty`, replaced by Display implementation of Ty which uses the internal display.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 21, 2024
…rrors

Fix misc printing issues in emit=stable_mir

Trying to continue the work that ``@ouz-a`` started here: rust-lang#118364

Few modifications beyond fixes:
1. I made the `pretty_*` functions private.
2. I added a function to print the instance body
3. Changed a bunch of signatures to write to the writer directly.
4. Added a function to translate the place to its internal representation, so we could use the internal debug implementation.
5. Also removed `pretty_ty`, replaced by Display implementation of Ty which uses the internal display.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2024
…rrors

Fix misc printing issues in emit=stable_mir

Trying to continue the work that ```@ouz-a``` started here: rust-lang#118364

Few modifications beyond fixes:
1. I made the `pretty_*` functions private.
2. I added a function to print the instance body
3. Changed a bunch of signatures to write to the writer directly.
4. Added a function to translate the place to its internal representation, so we could use the internal debug implementation.
5. Also removed `pretty_ty`, replaced by Display implementation of Ty which uses the internal display.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2024
…rrors

Fix misc printing issues in emit=stable_mir

Trying to continue the work that ````@ouz-a```` started here: rust-lang#118364

Few modifications beyond fixes:
1. I made the `pretty_*` functions private.
2. I added a function to print the instance body
3. Changed a bunch of signatures to write to the writer directly.
4. Added a function to translate the place to its internal representation, so we could use the internal debug implementation.
5. Also removed `pretty_ty`, replaced by Display implementation of Ty which uses the internal display.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2024
Rollup merge of rust-lang#122801 - celinval:smir-pretty, r=compiler-errors

Fix misc printing issues in emit=stable_mir

Trying to continue the work that ````@ouz-a```` started here: rust-lang#118364

Few modifications beyond fixes:
1. I made the `pretty_*` functions private.
2. I added a function to print the instance body
3. Changed a bunch of signatures to write to the writer directly.
4. Added a function to translate the place to its internal representation, so we could use the internal debug implementation.
5. Also removed `pretty_ty`, replaced by Display implementation of Ty which uses the internal display.
@celinval
Copy link
Contributor

Closing this in favor of #122801

@celinval celinval closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants