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

string fmt bug on nightly #43765

Closed
vitiral opened this issue Aug 9, 2017 · 6 comments
Closed

string fmt bug on nightly #43765

vitiral opened this issue Aug 9, 2017 · 6 comments
Labels
C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@vitiral
Copy link
Contributor

vitiral commented Aug 9, 2017

I develop artifact on nightly and publish using stable, so I am sensitive to changes that happen in nightly but not stable.

Right now the fmt functionality seems to be partially broken on nightly. this commit fails the unit test (running cargo test) cmd::tests::test_ls::test_cmd_check:

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.                                                                                                                            
stack backtrace:                                                                                                                                                                                                   
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace                                                                                                                                                     
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49                                                                                                                                        
   1: std::sys_common::backtrace::_print                                                                                                                                                                           
             at /checkout/src/libstd/sys_common/backtrace.rs:71                                                                                                                                                   
   2: std::panicking::default_hook::{{closure}}                                                                                                                                                                    
             at /checkout/src/libstd/sys_common/backtrace.rs:60                                                                                                                                                    
             at /checkout/src/libstd/panicking.rs:380                                                                                                                                                              
   3: std::panicking::default_hook                                                                                                                                                                                 
             at /checkout/src/libstd/panicking.rs:390                                                                                                                                                             
   4: std::panicking::rust_panic_with_hook                                                                                                                                                                        
             at /checkout/src/libstd/panicking.rs:610                                                    
   5: std::panicking::begin_panic                                                                                                                                                                                  
             at /checkout/src/libstd/panicking.rs:571                                                                                                                                                              
   6: std::panicking::begin_panic_fmt                                                                                                                                                                              
             at /checkout/src/libstd/panicking.rs:521                                                                                                                                                              
   7: artifact_app::cmd::tests::test_ls::test_cmd_check                                                                                                                                                            
             at src/cmd/tests/test_ls.rs:260                                                                                                                                                                      
   8: <F as test::FnBox<T>>::call_box                                                                                                                                                                              
             at /checkout/src/libtest/lib.rs:1477                                                                                                                                                                  
             at /checkout/src/libcore/ops/function.rs:143                                                                                                                                                          
             at /checkout/src/libtest/lib.rs:138                                                                                                                                                                   
   9: __rust_maybe_catch_panic                                                                                                                                                                                     
             at /checkout/src/libpanic_unwind/lib.rs:98                                                                                                                                                            
                                                                                             
                                                                                                                                                                                                                   
failures:                                                                                                                                                                                                          
    cmd::tests::test_ls::test_cmd_check                                                                                                                                                                            
                                                                                                                                                                                                                   
test result: FAILED. 34 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out                                                                                                                                    
                                                                                                                                                                                                                  
error: test failed, to rerun pass '--lib'               

The test prints the expected/result. That printout is:

Debug Result:                                                                                                                                                                                                      
                                                                                                                                                                                                                  
Found partof names that do not exist:                                                                                                                                                                              
- REQ-invalid-parts [../../reqs/foo.toml]: {REQ-DNE}                                                                                                                                                               
                                                                                                                                                                                                                   
Artifacts partof contains at least one recursive reference:                                                                                                                                                        
- SPC-unresolvable              : [SPC-UNRESOLVABLE-1-1]                                                                                                                                                          
- SPC-unresolvable-1            : [SPC-UNRESOLVABLE]                                                                                                                                                              
- SPC-unresolvable-1-1          : [SPC-UNRESOLVABLE-1]                                                   
                                                                                                                                                                                                                   
Found implementation links in the code that do not exist:                                                                                                                                                          
- ../../fake:                                                                                                                                                                                                      
  - [42] SPC-dne                                                                                                                                                                                                   
                                                                                                                                                                                                                   
Hanging artifacts found (top-level but not partof a higher type):                                                                                                                                                 
- ../../reqs/foo.toml: TST-line                                                                                                                                                                                    
                                                                                                                                                                                                                   
Repr Result:                                                                                                                                                                                                       
\x1b[1;31m\nFound partof names that do not exist:\n\x1b[0m\x1b[31m- REQ-invalid-parts [../../reqs/foo.toml]: {REQ-DNE}\n\x1b[0m\x1b[1;31m\nArtifacts partof contains at least one recursive reference:\n\x1b[0m- SP
C-unresolvable              : [SPC-UNRESOLVABLE-1-1]\n- SPC-unresolvable-1            : [SPC-UNRESOLVABLE]\n- SPC-unresolvable-1-1          : [SPC-UNRESOLVABLE-1]\n\x1b[1;31m\nFound implementation links in the c
ode that do not exist:\n\x1b[0m\x1b[31m- ../../fake:\n\x1b[0m\x1b[31m  - [42]\x1b[0m SPC-dne\n\x1b[1;31m\nHanging artifacts found (top-level but not partof a higher type):\n\x1b[0m- ../../reqs/foo.toml: TST-line
\n\n                                                                                         
--Result Repr DONE
Debug Expected:                                                                                                                                                                                                    
                                                                                                                                                                                                                   
Found partof names that do not exist:                                                                                                                                                                              
- REQ-invalid-parts [../../reqs/foo.toml]: {REQ-DNE}                                                                                                                                                               
                                                                                                                                                                                                                  
Artifacts partof contains at least one recursive reference:                                                                                                                                                        
- SPC-unresolvable              : [SPC-UNRESOLVABLE-1-1]                                                                                                                                                           
- SPC-unresolvable-1            : [SPC-UNRESOLVABLE]                                                                                                                                                               
- SPC-unresolvable-1-1          : [SPC-UNRESOLVABLE-1]                                                                                                                                                             
                                                                                                                                                                                                                  
Found implementation links in the code that do not exist:                                                                                                                                                         
- ../../fake:                                                                                            
  - [42] SPC-dne                                                                                                                                                                                                   
                                                                                                                                                                                                                   
Hanging artifacts found (top-level but not partof a higher type):                                                                                                                                                  
- ../../reqs/foo.toml           : TST-line                                                                                                                                                                         
                                                                                                                                                                                                                   
Repr Expected:                                                                                                                                                                                                    
\x1b[1;31m\nFound partof names that do not exist:\n\x1b[0m\x1b[31m- REQ-invalid-parts [../../reqs/foo.toml]: {REQ-DNE}\n\x1b[0m\x1b[1;31m\nArtifacts partof contains at least one recursive reference:\n\x1b[0m- SP
C-unresolvable              : [SPC-UNRESOLVABLE-1-1]\n- SPC-unresolvable-1            : [SPC-UNRESOLVABLE]\n- SPC-unresolvable-1-1          : [SPC-UNRESOLVABLE-1]\n\x1b[1;31m\nFound implementation links in the c
ode that do not exist:\n\x1b[0m\x1b[31m- ../../fake:\n\x1b[0m\x1b[31m  - [42]\x1b[0m SPC-dne\n\x1b[1;31m\nHanging artifacts found (top-level but not partof a higher type):\n\x1b[0m- ../../reqs/foo.toml          
 : TST-line\n\n                                                                                                                                                                                                    
--Expected Repr DONE

The difference between the two (that I can spot) is these two lines:

# result
- ../../reqs/foo.toml: TST-line
# expected                                                                                                                                         
- ../../reqs/foo.toml           : TST-line

I went into the code to find what generates this formatting, it is in src/cmd/check.rs line 234:

            write!(
                msg,
                "- {:<30}: {}\n",
                utils::relative_path(p, cwd).display(),
                h
            ).unwrap();

Oddly, there is a very similar line of code that is used to generate the Artifacts partof contains at least one recursive reference: section of the output. That line of code is in the same file, line 159:

write!(msg, "- {:<30}: {:?}\n", name.to_string(), partof).unwrap();

That line works, the other one doesn't. Why? That I have no clue about...

Meta

This passes (running cargo test) on

$ rustc --version --verbose   
rustc 1.19.0 (0ade33941 2017-07-17)                 
binary: rustc             
commit-hash: 0ade339411587887bf01bcfa2e9ae4414c8900d4                                                    
commit-date: 2017-07-17   
host: x86_64-unknown-linux-gnu                      
release: 1.19.0           
LLVM version: 4.0

And fails on

$ rustc --version --verbose                                                                                                                                                           
rustc 1.21.0-nightly (cbbe17aa7 2017-08-07)                                                                                                                                                                       
binary: rustc
commit-hash: cbbe17aa7f13f9568a652c2180de03fa6881b86a
commit-date: 2017-08-07
host: x86_64-unknown-linux-gnu
release: 1.21.0-nightly
LLVM version: 4.0

# and fails for this as well
$ rustc --version                                                                                                                                                                     
rustc 1.21.0-nightly (b75d1f0ce 2017-08-02)
@vitiral
Copy link
Contributor Author

vitiral commented Aug 9, 2017

it also passes for

rustc --version                                                                                                                                                                     
rustc 1.19.0-nightly (fe7227f6c 2017-06-16)

@sfackler sfackler added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Aug 9, 2017
@ExpHP
Copy link
Contributor

ExpHP commented Aug 9, 2017

bisection log

rustc 1.20.0-nightly (d84693b93 2017-07-09) ... test FAILED
rustc 1.20.0-nightly (37849a002 2017-06-30) ... test FAILED
rustc 1.19.0-nightly (04145943a 2017-06-19) ... test FAILED
rustc 1.19.0-nightly (78d8416ca 2017-06-17) ... test FAILED

@ExpHP
Copy link
Contributor

ExpHP commented Aug 9, 2017

...and that exhausts the nightlies. We're down to

fe7227f...78d8416

@alexcrichton alexcrichton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 9, 2017
@ollie27
Copy link
Member

ollie27 commented Aug 9, 2017

It looks like this was caused by #42613 cc @stepancheg. The Display impl for Path no longer takes formatting parameters into account. For example:

println!("a{:#<5}b", ::std::path::Path::new("").display());

Prints a#####b on stable and ab on beta and nightly.

@vitiral
Copy link
Contributor Author

vitiral commented Aug 9, 2017

thanks for diagnosing. It would be worth adding some unit tests around Path formatting as I imagine it is depended upon by a fair number of people (myself included!)

@arielb1 arielb1 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Aug 10, 2017
@alexcrichton
Copy link
Member

I've sent a PR for this at #43830

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 14, 2017
Historically many `Display` and `Debug` implementations for `OsStr`-like
abstractions have gone through `String::from_utf8_lossy`, but this was updated
in rust-lang#42613 to use an internal `Utf8Lossy` abstraction instead. This had the
unfortunate side effect of causing a regression (rust-lang#43765) in code which relied on
these `fmt` trait implementations respecting the various formatting flags
specified.

This commit opportunistically adds back interpretation of formatting trait flags
in the "common case" where where `OsStr`-like "thing" is all valid utf-8 and can
delegate to the formatting implementation for `str`. This doesn't entirely solve
the regression as non-utf8 paths will format differently than they did before
still (in that they will not respect formatting flags), but this should solve
the regression for all "real world" use cases of paths and such. The door's also
still open for handling these flags in the future!

Closes rust-lang#43765
@alexcrichton alexcrichton modified the milestone: 1.20 Aug 23, 2017
bors added a commit that referenced this issue Aug 23, 2017
std: Respect formatting flags for str-like OsStr

Historically many `Display` and `Debug` implementations for `OsStr`-like
abstractions have gone through `String::from_utf8_lossy`, but this was updated
in #42613 to use an internal `Utf8Lossy` abstraction instead. This had the
unfortunate side effect of causing a regression (#43765) in code which relied on
these `fmt` trait implementations respecting the various formatting flags
specified.

This commit opportunistically adds back interpretation of formatting trait flags
in the "common case" where where `OsStr`-like "thing" is all valid utf-8 and can
delegate to the formatting implementation for `str`. This doesn't entirely solve
the regression as non-utf8 paths will format differently than they did before
still (in that they will not respect formatting flags), but this should solve
the regression for all "real world" use cases of paths and such. The door's also
still open for handling these flags in the future!

Closes #43765
alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 23, 2017
Historically many `Display` and `Debug` implementations for `OsStr`-like
abstractions have gone through `String::from_utf8_lossy`, but this was updated
in rust-lang#42613 to use an internal `Utf8Lossy` abstraction instead. This had the
unfortunate side effect of causing a regression (rust-lang#43765) in code which relied on
these `fmt` trait implementations respecting the various formatting flags
specified.

This commit opportunistically adds back interpretation of formatting trait flags
in the "common case" where where `OsStr`-like "thing" is all valid utf-8 and can
delegate to the formatting implementation for `str`. This doesn't entirely solve
the regression as non-utf8 paths will format differently than they did before
still (in that they will not respect formatting flags), but this should solve
the regression for all "real world" use cases of paths and such. The door's also
still open for handling these flags in the future!

Closes rust-lang#43765
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants