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

Better Debug for Vars and VarsOs #114132

Merged
merged 1 commit into from Aug 12, 2023
Merged

Better Debug for Vars and VarsOs #114132

merged 1 commit into from Aug 12, 2023

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Jul 27, 2023

Display actual vars instead of two dots.

The same was done for Args and ArgsOs in 275f9a0.

@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2023

r? @cuviper

(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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 27, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot assigned Amanieu and unassigned cuviper Jul 27, 2023
@tamird tamird force-pushed the better-env-debug-impls branch 2 times, most recently from 9f46040 to c9f89d1 Compare July 27, 2023 17:14
@tamird
Copy link
Contributor Author

tamird commented Aug 2, 2023

@cuviper @Amanieu ping.

@Amanieu
Copy link
Member

Amanieu commented Aug 4, 2023

This is missing Debug impls for all the other OSes. Search for pub struct Env in library/std/src/sys.

Perhaps it would be easier if the debug printing logic was in the common code and the underlying OS-specific iterator could just be cloned?

@tamird
Copy link
Contributor Author

tamird commented Aug 4, 2023

This is missing Debug impls for all the other OSes. Search for pub struct Env in library/std/src/sys.

Perhaps it would be easier if the debug printing logic was in the common code and the underlying OS-specific iterator could just be cloned?

I'd rather not introduce additional allocations. Done.

@Amanieu
Copy link
Member

Amanieu commented Aug 4, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2023

📌 Commit 3b5f41477d4c279c9870d567e31965619396d9f1 has been approved by Amanieu

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 Aug 4, 2023
@bors
Copy link
Contributor

bors commented Aug 4, 2023

⌛ Testing commit 3b5f41477d4c279c9870d567e31965619396d9f1 with merge 1ef1988cc8a8ac40375b19a25a71cf9546abdbc4...

@bors
Copy link
Contributor

bors commented Aug 4, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 4, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tamird
Copy link
Contributor Author

tamird commented Aug 4, 2023

Missed a necessary deref, should compile now.

@Amanieu
Copy link
Member

Amanieu commented Aug 4, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2023

📌 Commit 216edcb has been approved by Amanieu

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 4, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2023
@matthiaskrgr
Rollup merge of rust-lang#114132 - tamird:better-env-debug-impls, r=A…
2628780 
…manieu

Better Debug for Vars and VarsOs

Display actual vars instead of two dots.

The same was done for Args and ArgsOs in 275f9a0.
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 5, 2023
Closed
@matthiaskrgr
Copy link
Member

@bors r-
failed in #114506 (comment)
2023-08-05T11:51:16.2355078Z thread 'env::tests::vars_debug' panicked at library\std\src\env\tests.rs:110:5:

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 5, 2023
@tamird
Copy link
Contributor Author

tamird commented Aug 5, 2023

Looks like an environment variable containing a single quoted string on windows was the culprit:

 (\"VERBOSE_ARG\",
-\"'SilentlyContinue'\"),
+\"\\'SilentlyContinue\\'\"),

@tamird
Copy link
Contributor Author

tamird commented Aug 5, 2023

This is caused by <Vars as Debug>::fmt not converting to str internally and the slight difference between <str as Debug>::fmt and <OsString as Debug>::fmt. Repro: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9577395fe35685cbae74fc74185e28e9.

Will fix momentarily.

@tamird tamird mentioned this pull request Aug 7, 2023
Open
@tamird
Better Debug for Vars and VarsOs
35c0c03 
Display actual vars instead of two dots.

The same was done for Args and ArgsOs in 275f9a0.
@tamird
Copy link
Contributor Author

tamird commented Aug 7, 2023

This should be ready for another run.

Amanieu
Amanieu reviewed Aug 9, 2023
View reviewed changes
library/std/src/sys/hermit/os.rs
@@ -112,6 +112,34 @@ pub struct Env {
iter: vec::IntoIter<(OsString, OsString)>,
}

// FIXME(https://github.com/rust-lang/rust/issues/114583): Remove this when <OsStr as Debug>::fmt matches <str as Debug>::fmt.
pub struct EnvStrDebug<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Could we avoid the code duplication here and move EnvStrDebug into common code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not without introducing additional allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps I should elaborate. The various Env structures contain iterators - even when they are backed by vectors - so it isn't possible to implement Debug without iterating those iterators. This means the iterators must be cloned. In the case of a vector-backed iterator, cloning would allocate, which this approach avoids.

@Amanieu
Copy link
Member

Amanieu commented Aug 11, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 11, 2023

📌 Commit 35c0c03 has been approved by Amanieu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 11, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 12, 2023
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 12, 2023
@matthiaskrgr
Rollup merge of rust-lang#114132 - tamird:better-env-debug-impls, r=A…
7879823 
…manieu

Better Debug for Vars and VarsOs

Display actual vars instead of two dots.

The same was done for Args and ArgsOs in 275f9a0.
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 12, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2023
@bors
Auto merge of rust-lang#114756 - matthiaskrgr:rollup-4m7l4p6, r=matth…
cbb48a5 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#94455 (Partially stabilize `int_roundings`)
 - rust-lang#114132 (Better Debug for Vars and VarsOs)
 - rust-lang#114584 (E0277 nolonger points at phantom `.await`)
 - rust-lang#114667 (Record binder for bare trait object in LifetimeCollectVisitor)
 - rust-lang#114692 (downgrade `internal_features` to warn)
 - rust-lang#114703 (Cover ParamConst in smir)
 - rust-lang#114734 (Mark oli as "on vacation")

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8a997b1 into rust-lang:master Aug 12, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 12, 2023
@tamird tamird deleted the better-env-debug-impls branch August 13, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

@Amanieu Amanieu Amanieu left review comments

Assignees

@Amanieu Amanieu

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@tamird @rustbot @rust-log-analyzer @Amanieu @bors @matthiaskrgr @cuviper