- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
Move CoercePointee to core::ops #147068
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
Move CoercePointee to core::ops #147068
Conversation
| Some changes occurred in src/tools/cargo cc @ehuss rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer | 
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
ca487ad    to
    6ee266e      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break rust-analyzer tests as these paths are pulled from our mini core which would also need to be adjusted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
6ee266e    to
    4304ead      
    Compare
  
    | This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. | 
| @rustbot ready | 
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
4304ead    to
    56c6372      
    Compare
  
    | The Miri subtree was changed cc @rust-lang/miri | 
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
        
          
                src/tools/miri/src/shims/files.rs
              
                Outdated
          
        
      | use std::fs::{File, Metadata}; | ||
| use std::io::{ErrorKind, IsTerminal, Seek, SeekFrom, Write}; | ||
| use std::marker::CoercePointee; | ||
| use std::ops::CoercePointee; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the path used by FileDescriptionRef so I don't understand why CoercePointee broke for FileDescriptionRef. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll need #[cfg(bootstrap)] std::marker::CoercePointee and #[cfg(not(bootstrap))] std::ops::CoercePointee so that miri can build stage 1 with an older std and stage 2 with a newer std. Check everything using ./x check --stage 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I thought we got rid of the bootstrap stuff recently, but I guess not everywhere. Thinking about it a bit more, I'm just going to add a re-export and remove it in the next rustc cycle.
I believe that also avoids the need to patch the Rust for Linux job, since it uses the macro too.
125b87c    to
    77edb32      
    Compare
  
    | @rustbot label I-lang-nominated I-libs-api-nominated This puts  In my view the common thread between traits in the  If we don't do this we should adopt another framing that explains where traits like the following should go. There is clearly ambiguity here which is how we ended up with  | 
| We discussed this in @rust-lang/libs-api, and found this change a little odd: we felt like this didn't fit in the current definition of  I also want to observe that there are more locations than just  | 
| I think there's an argument to be made that it should be in the same location as  | 
| While the behaviour of the trait is lang-relevant, I think where it lives is purely libs-api, and thus this should be entirely their call. (Obviously anyone interested should comment their opinions, but no need for lang to sign off on the organization.) | 
| @Darksonn I'd agree with that, but that one is unstable too. We should put them both in the same module, but we aren't constrained on which module that could be. | 
| We discussed this in today's @rust-lang/lang meeting, and broadly felt like 1) this was a @rust-lang/libs-api decision and 2) we'd love to see a clear design for where new traits should go. | 
| Closing as per #123430 (comment) that decided to keep this in  | 
This was suggested by @tmandry.
diff library/core/src/marker.rs library/core/src/ops/unsize.rs