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

Integrate privacy into autoderef loop with overloaded deref #12808

Closed
nikomatsakis opened this issue Mar 10, 2014 · 10 comments · Fixed by #31938
Closed

Integrate privacy into autoderef loop with overloaded deref #12808

nikomatsakis opened this issue Mar 10, 2014 · 10 comments · Fixed by #31938
Assignees
Labels
P-medium Medium priority

Comments

@nikomatsakis
Copy link
Contributor

Once PR #12610 lands, the autoderef loop will be generalized to permit dereferencing through user-defined structures that implement the Deref trait. As such, the termination condition becomes less clear around privacy. Currently, when searching for fields, we autoderef until finding a struct and then search for a field. In PR #12610, we will continue searching if no field with the desired name is found. Unfortunately, the PR does not consider privacy, and thus adding a field to a smart pointer can shadow fields in the referent. The same applies to methods. This is suboptimal.

The desired behavior is that private fields and methods are ignored as part of the autoderef loop, so long as the expression being autoderef'd is not part of the module where the field/method was defined. This is a relatively simple check: if the field/method is declared private, you find the module M containing the declaration. You then check the whether current module N is a equal to M or a submodule of M. If not, you ignore the field/method.

For better error messages, it would be nice to at least track whether a private field/method with a suitable name was seen, so that we can report an error like "no accessible field f found (note that private fields were found)" or something like that.

@nikomatsakis
Copy link
Contributor Author

cc @eddyb

@nikomatsakis
Copy link
Contributor Author

Nominating for 1.0 P-Backcompat-lang.

@pnkfelix
Copy link
Member

Assigning 1.0, P-backcompat-lang.

@pnkfelix pnkfelix added this to the 1.0 milestone Mar 13, 2014
@flaper87 flaper87 self-assigned this Apr 10, 2014
huonw added a commit to huonw/rust that referenced this issue May 25, 2014
Types that implement Deref can cause weird error messages due to their
private fields conflicting with a field of the type they deref to, e.g.,
previously

    struct Foo { x: int }

    let a: Arc<Foo> = ...;
    println!("{}", a.x);

would complain the the `x` field of `Arc` was private (since Arc has a
private field called `x`) rather than just ignoring it.

This patch doesn't fix that issue, but does mean one would have to write
`a._ptr` to hit the same error message, which seems far less
common. (This patch `_`-prefixes all private fields of
`Deref`-implementing types.)

cc rust-lang#12808
bors added a commit that referenced this issue May 25, 2014
Paper over privacy issues with Deref by changing field names.

Types that implement Deref can cause weird error messages due to their
private fields conflicting with a field of the type they deref to, e.g.,
previously

    struct Foo { x: int }

    let a: Arc<Foo> = ...;
    println!("{}", a.x);

would complain the the `x` field of `Arc` was private (since Arc has a
private field called `x`) rather than just ignoring it.

This patch doesn't fix that issue, but does mean one would have to write
`a._ptr` to hit the same error message, which seems far less
common. (This patch `_`-prefixes all private fields of
`Deref`-implementing types.)

cc #12808
@pcwalton
Copy link
Contributor

pcwalton commented Jul 6, 2014

Thinking about this, I don't believe this is backwards incompatible per se. The worst thing that can happen is that, under today's rules, the method resolver will be led astray by a method that is private, and the program will fail to compile.

To be sure, this will prevent us from adding new methods to smart pointers like Rc backwards compatibly post 1.0. However, say we released 1.0 with the method resolution rules as today. If we decided in (say) 1.1 to add new methods to Rc, we could gate 1.1 on this issue being fixed, and then no code would break. Therefore, I think this is P-high, but not P-backcompat-lang.

Marking this as not-blocking-1.0 reduces schedule risk because this is a fairly involved change that carves up privacy and moves large swathes of it to pre-typechecking and typechecking phases.

@pnkfelix
Copy link
Member

Downgradiing to P-high, leaving on the 1.0 milestone, following the reasoning outlined by @pcwalton in #12808 (comment)

@nikomatsakis : you may want to weigh in here on this recategorization.

@pnkfelix pnkfelix modified the milestone: 1.0 Jul 10, 2014
@aturon
Copy link
Member

aturon commented Jan 8, 2015

Nominating; I'm unclear on the status of this, but we should decide whether it merits a -beta milestone.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 8, 2015

leaving on 1.0 milestone. @nikomatsakis you may want to double-check whether that is good (versus promoting to 1.0-beta milestone).

@pnkfelix
Copy link
Member

pnkfelix commented Apr 2, 2015

@nikomatsakis would like to see this fixed by 1.1.

So, assinging to @nikomatsakis and putting on 1.1 milestone.

@pnkfelix pnkfelix added this to the 1.0 beta milestone Apr 2, 2015
@steveklabnik steveklabnik removed this from the 1.1 milestone May 21, 2015
jseyfried added a commit to jseyfried/rust that referenced this issue Mar 16, 2016
jseyfried added a commit to jseyfried/rust that referenced this issue Mar 30, 2016
bors added a commit that referenced this issue Mar 31, 2016
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 added a commit that referenced this issue Mar 31, 2016
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
tbu- added a commit to tbu-/rust that referenced this issue Apr 5, 2016
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 6, 2016
Remove strange names created by lack of privacy-conscious name lookup

The fixed issue that allowed this was rust-lang#12808.
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 7, 2016
Remove strange names created by lack of privacy-conscious name lookup

The fixed issue that allowed this was rust-lang#12808.
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 7, 2016
Remove strange names created by lack of privacy-conscious name lookup

The fixed issue that allowed this was rust-lang#12808.
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 3, 2017

There is a FIXME related to this issue,
https://github.com/rust-lang/rust/blob/master/src/test/run-pass/overloaded-autoderef-order.rs#L78
Now that the issue is closed can the FIXME be fix, or made more specific?

@eddyb
Copy link
Member

eddyb commented Sep 4, 2017

Looks like it.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 6, 2017
rust-lang#12808 is closed remove the FIXME

let's see if this can be cleaned up.

rust-lang#12808 (comment)
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 7, 2017
rust-lang#12808 is closed remove the FIXME

let's see if this can be cleaned up.

rust-lang#12808 (comment)
m-ou-se added a commit to fusion-engineering-forks/rust that referenced this issue Sep 24, 2020
The double underscores were used to work around issue rust-lang#12808, which was
solved in 2016.
lnicola pushed a commit to lnicola/rust that referenced this issue Aug 9, 2022
feat: Only flycheck workspace that belongs to saved file

Supercedes rust-lang/rust-analyzer#11038

There is still the problem that all the diagnostics are cleared, only clearing diagnostics of the relevant workspace isn't easily doable though I think, will have to dig into that
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants