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.

Sign up for GitHub

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

Jump to bottom

Integrate privacy into field and method selection #31938

Merged
merged 7 commits into from
Mar 31, 2016
Merged

Integrate privacy into field and method selection #31938

merged 7 commits into from
Mar 31, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Feb 27, 2016 •
edited
Loading

This PR integrates privacy checking into field and method selection so that an inaccessible field/method cannot stop an accessible field/method from being used (fixes #12808 and fixes #22684).
r? @eddyb

@jseyfried
Copy link
Contributor Author

cc @nikomatsakis @petrochenkov

@jseyfried jseyfried force-pushed the autoderef_privacy branch 4 times, most recently from 9be5455 to 5d2e258 Compare February 28, 2016 00:35
@nikomatsakis
Copy link
Contributor

On Sat, Feb 27, 2016 at 01:23:08AM -0800, Jeffrey Seyfried wrote:

cc @nikomatsakis @petrochenkov

you're on a roll! Just FYI I sat down over last few days to read over
some of your changes to resolve in detail, because I realize I've kind
of lost track of how things are working, and am starting to feel
uncomfortable reviewing :) hoping to finish that up tonight though

@jseyfried
Copy link
Contributor Author

@nikomatsakis Ok, let me know if you'd like me to explain or comment anything!

@jseyfried jseyfried force-pushed the autoderef_privacy branch 3 times, most recently from 2f835d1 to 12a209b Compare March 3, 2016 07:47
@jseyfried jseyfried mentioned this pull request Mar 6, 2016
Merged
@jseyfried
Copy link
Contributor Author

Fixes #22684

@eddyb
Copy link
Member

eddyb commented Mar 11, 2016

@jseyfried you need to put that in the description.

@jseyfried
Copy link
Contributor Author

@eddyb I updated the description.

@jseyfried jseyfried mentioned this pull request Mar 11, 2016
Closed
@jseyfried
Copy link
Contributor Author

I rebased and added a commit that refactors away most of the privacy visitor.

@bors
Copy link
Contributor

bors commented Mar 27, 2016

☔ The latest upstream changes (presumably #32432) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@jseyfried can you add some tests that show that the logic about private fields/methods is working? I was thinking it'd be nice to have a test where the private field appears in an outer deref. In other words, if the autoderef chain of types is A -> B -> C, then the field/method is on type A or B.

r=me with such a test.

@nikomatsakis nikomatsakis self-assigned this Mar 27, 2016
nikomatsakis
nikomatsakis reviewed Mar 27, 2016
View reviewed changes
src/test/compile-fail/autoderef-privacy.rs
fn f(bar: foo::Bar, baz: foo::Baz) {
let _ = bar.i;
let _ = baz.0;
let _ = bar.f();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you convert this to a run-pass test and somehow check the return value of f, so that we know the right function is being called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

@nikomatsakis
Copy link
Contributor

Sorry, let me clarify my previous comment. I'd like to see two things:

  1. Modify the existing test for private fields/methods to be run-pass, and using some logic that proves we are picking the right fields.
    • Maybe also extend the test to read from the fields in a context where it should pick the private one, and prove that this logic is working correctly.
  2. A test specifically targeting the case where only a private field/method is available, which would target the logic you put in around that case.

@jseyfried
Copy link
Contributor Author

@nikomatsakis

  1. Modify the existing test for private fields/methods to be run-pass, and using some logic that proves we are picking the right fields.
    • Maybe also extend the test to read from the fields in a context where it should pick the private one, and prove that this logic is working correctly.

Done.

A test specifically targeting the case where only a private field/method is available

Private inherent methods are tested in private-method-inherited (and others) and I amended struct-field-privacy to test private positional fields as well as private named fields.

I believe these tests are sufficient to check the logic you referenced, but if you want I can add a compile-fail test where there is only a private field/method in a non-trivial autoderef chain.

@nikomatsakis
Copy link
Contributor

@jseyfried I'll leave it to your judgement. I certainly lean towards more exhaustive testing but there is no reason to add duplicate tests either. R=me either way.

@jseyfried
Copy link
Contributor Author

Ok, thanks for reviewing!

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 31, 2016

📌 Commit 48c20b0 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 31, 2016

⌛ Testing commit 48c20b0 with merge 4f46ecb...

bors added a commit that referenced this pull request Mar 31, 2016
@bors
Auto merge of #31938 - jseyfried:autoderef_privacy, r=nikomatsakis
4f46ecb 
Integrate privacy into field and method selection

This PR integrates privacy checking into field and method selection so that an inaccessible field/method can not stop an accessible field/method from being used (fixes #12808 and fixes #22684).
r? @eddyb
@bors
Copy link
Contributor

bors commented Mar 31, 2016

💔 Test failed - auto-mac-32-opt

@jseyfried
Copy link
Contributor Author

@bors retry

@alexcrichton
Copy link
Member

@bors: retry force clean

@alexcrichton
Copy link
Member

@bors: retry force clean

maybe second time's the charm?

@bors
Copy link
Contributor

bors commented Mar 31, 2016

⌛ Testing commit 48c20b0 with merge 3399d19...

bors added a commit that referenced this pull request Mar 31, 2016
@bors
Auto merge of #31938 - jseyfried:autoderef_privacy, r=nikomatsakis
3399d19 
Integrate privacy into field and method selection

This PR integrates privacy checking into field and method selection so that an inaccessible field/method can not stop an accessible field/method from being used (fixes #12808 and fixes #22684).
r? @eddyb
@bors
Copy link
Contributor

bors commented Mar 31, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-freebsd, auto-linux-64-x-netbsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 48c20b0 into rust-lang:master Mar 31, 2016
This was referenced Mar 31, 2016
Merged
Merged
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 5, 2016
@jseyfried jseyfried deleted the autoderef_privacy branch September 27, 2016 08:44
@jseyfried jseyfried mentioned this pull request Dec 22, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A strange processing methods visibility Integrate privacy into autoderef loop with overloaded deref
7 participants
@jseyfried @nikomatsakis @eddyb @bors @alexcrichton @bluss @petrochenkov