Skip to content

Regression: issue #43941 (pointer methods) causes code in nvml crate to no longer compile #47159

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

Closed
raphaelcohn opened this issue Jan 3, 2018 · 3 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@raphaelcohn
Copy link

In this crate, there is an extension trait:-

pub trait PMEMblkpoolEx
{
...
    /// Returns false if the block has previously had its error condition set
	#[inline(always)]
	fn read(self, to: *mut c_void, zeroBasedBlockIndex: usize) -> bool;
...
}

This is then impl'd for an existing pointer:-

impl PMEMblkpoolEx for *mut PMEMblkpool
{
...
    
	#[inline(always)]
	fn read(self, to: *mut c_void, zeroBasedBlockIndex: usize) -> bool
	{
        ...
        }
...
}

Compiling this produces:-

error: use of unstable library feature 'pointer_methods' (see issue #43941)
  --> src/blockPool/BlockPool.rs:69:10
   |
69 |   self.0.read(to, zeroBasedBlockIndex)
   |          ^^^^
   |
   = help: add #![feature(pointer_methods)] to the crate attributes to enable

This is wrong on several levels. Firstly, adding a new feature to the std lib broke existing code. Secondly, the error message is decidedly incorrect. I'm actually in favour of the new pointer methods, so I'm happy to fix the code in this crate going forward (?would specialization help?), but I feel something got missed here in the implementation process. There's a decided lack of documentation around it - the unstable book contains no details, and the tracking issue is equally thin.

@eddyb
Copy link
Member

eddyb commented Jan 3, 2018

I don't think anyone expected methods on raw pointers - they're almost useless in general.
That is, you always have to wrap them to write any kind of safe code involving data behind them (and sometimes even unsafe code needs wrapping to avoid even worse misuse).

OTOH, this should've been caught in the process, I wonder if it's nightly-only and we haven't ran crater for this release yet (but I'm unaware of the exact procedure here).
cc @rust-lang/libs

@alexcrichton alexcrichton added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 3, 2018
@alexcrichton
Copy link
Member

This was most likely caused by #43964 where new inherent methods were added to raw pointers. It's definitely true though that the tracking issue is light on details!

I believe the PR in question has already made its way to stable (1.22.1) so there's not a huge sense of urgency to fix this per se, but would be good to discuss. We'll discuss this at a triage meeting for the libs team but it's pretty likely that we will leave the API as-is and won't change it.

@alexcrichton
Copy link
Member

Ok I've brought this up with the @rust-lang/libs team during triage today and the conclusion was that we're going to stay the current course and close this issue. @raphaelcohn if you have difficulty updating though please just let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants