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

Fix wrong argument in autoderef process #71627

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Conversation

ldm0
Copy link
Contributor

@ldm0 ldm0 commented Apr 28, 2020

The overloaded_deref_ty is a function for derefencing a type which overloads the Deref trait. But actually this function never uses the parameter pushed in until this PR. -_-

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2020
@Dylan-DPC-zz
Copy link

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 29, 2020

📌 Commit 8d2f301 has been approved by Dylan-DPC

@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 Apr 29, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 29, 2020
Fix wrong argument in autoderef process

The `overloaded_deref_ty` is a function for derefencing a type which overloads the `Deref` trait. But actually this function never uses the parameter pushed in until this PR. -_-
This was referenced Apr 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#71507 (Document unsafety in core::ptr)
 - rust-lang#71572 (test iterator chain type length blowup)
 - rust-lang#71617 (Suggest `into` instead of `try_into` if possible with int types)
 - rust-lang#71627 (Fix wrong argument in autoderef process)
 - rust-lang#71678 (Add an index page for nightly rustc docs.)
 - rust-lang#71680 (Fix doc link to Eq trait from PartialEq trait)

Failed merges:

 - rust-lang#71597 (Rename Unique::empty() -> Unique::dangling())

r? @ghost
@bors bors merged commit 75561a5 into rust-lang:master Apr 29, 2020
@RalfJung
Copy link
Member

RalfJung commented May 6, 2020

Does this change behavior in any way? Is there a test case we could add to ensure it does not regress?

@ldm0
Copy link
Contributor Author

ldm0 commented May 6, 2020

@RalfJung I'm pretty sure this won't cause a regress. This just correct the function to do the thing it meant to do. Is this related to some bug? I can help reviewing.

@RalfJung
Copy link
Member

RalfJung commented May 6, 2020

Sorry, what I meant is -- this fixes a bug, right? If so, there should be a test to make sure that in the future, this does not get changed back to the old, wrong behavior that we had before your PR.

@ldm0
Copy link
Contributor Author

ldm0 commented May 6, 2020

@RalfJung This doesn't fix a bug directly. This actually fixes future bugs. The situation is like this:

a = 1
b = 2
add(a, b) {
    return a + 2
}
print(add(1, 2))

Here we get correct answer 3. But in the future when we calls add(1, 3), it quietly causes a bug (and it will be extremely hard to digout in such a large codebase). This PR just changes a + 2 to a + b.

And this function is an internal function and only called in one place so for a long time this bug is left undiscovered. I think for regression avoiding, a unit test is needed, but unit tests seem to be not exist in current compiler. :-(

@ldm0 ldm0 deleted the autoderefarg branch May 23, 2020 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants