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

Implement DerefImm and DerefMut #7141 #12491

Closed
wants to merge 3 commits into from
Closed

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 23, 2014

Add the Deref and DerefMut traits and implement overloading explicit dereferences.

@eddyb
Copy link
Member Author

eddyb commented Feb 23, 2014

*x and &*x both work as expected, and I've figured that DerefMut would only be used by an explicit dereference in a limited set of contexts: &mut *x, *x = y and *x OP= y, all of which are easy to detect in typeck.

But then there's record_candidate_traits_for_expr_if_necessary in resolve, which assigns one lang-item trait per overloadable expression, so I would have to implement the DerefImm/DerefMut choosing logic there and that doesn't seem right to me.

Oh, and how could #12402 happen while I can't seem to call a method on the right trait (if it's not in trait_map), which is also in the prelude? I'm a bit confused.

EDIT: I didn't realize it at first, but trait_map holds a vector for each entry, so I can have both DerefImm and DerefMut available (not that we should do this in resolve, but I don't want to fix #12402 right now).

@nikomatsakis
Copy link
Contributor

Is this ready for review?

@eddyb
Copy link
Member Author

eddyb commented Feb 26, 2014

Ended up fixing #12402 and removing callee_id in the process, I'm happy with how things are turning out.

@nikomatsakis
Copy link
Contributor

/me will review

@pnkfelix
Copy link
Member

@eddyb please put a description into the issue at some point, since bors will not pick up its title when it merges.

@nikomatsakis
Copy link
Contributor

This looks excellent. You're missing a few tests. Here is a patch to apply:

https://gist.github.com/nikomatsakis/9348385

r+ once the new tests are applied and nit corrected (and rebased, of course ;)

@nikomatsakis
Copy link
Contributor

Actually, the one other thing I'd like to see:

Some new run-pass tests that:

(1) do not depend on Rc (I prefer to avoid dependencies between tests and the standard library if at all possible, to isolate the tests from refactorings and so forth)

(2) are targeted at checking that the correct method (deref vs deref_mut) is invoked in the correct times. I'm envisioning a type which either modifies a static mut each time the deref vs deref_mut method is called, or perhaps has a Rc<Cell<int>>, something like that. The point is, I want the test to assert that the right method was called at the right times and only once per deref.

@nikomatsakis
Copy link
Contributor

Regarding the existing overloaded operator test, arguably it should be moved to #[test] blocks in Rc but... I don't know. I guess this is really not testing Rc so much as the compiler, even though it draws on Rc. It seems a bit silly to copy the Rc code into the test. So I'd leave the existing test as is.

@nikomatsakis
Copy link
Contributor

Updated my patch with some fns for the compile-fail tests.

bors added a commit that referenced this pull request Mar 5, 2014
Add the `Deref` and `DerefMut` traits and implement overloading explicit dereferences.
@bors bors closed this Mar 5, 2014
@eddyb eddyb deleted the deref branch March 5, 2014 08:42
bors added a commit that referenced this pull request Mar 13, 2014
Enables the dereference overloads introduced by #12491 to be applied wherever automatic dereferences would be used (field accesses, method calls and indexing).
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2024
fix span calculation for non-ascii in `needless_return`

Fixes rust-lang#12491

Probably fixes rust-lang#12328 as well, but that one has no reproducer, so 🤷

The bug was that the lint used `rfind()` for finding the byte index of the start of the previous non-whitespace character:
```
// abc\n     return;
     ^
```
... then subtracting one to get the byte index of the actual whitespace (the `\n` here).
(Subtracting instead of adding because it treats this as the length from the `return` token to the `\n`)

That's correct for ascii, like here, and will get us to the `\n`, however for non ascii, the `c` could be multiple bytes wide, which would put us in the middle of a codepoint if we simply subtract 1 and is what caused the ICE.

There's probably a lot of ways we could fix this.
This PR changes it to iterate backwards using bytes instead of characters, so that when `rposition()` finally finds a non-whitespace byte, we *know* that we've skipped exactly 1 byte. This was *probably*(?) what the code was intending to do

changelog: Fix ICE in [`needless_return`] when previous line end in a non-ascii character
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants