fix for `Self` not respecting tuple Ctor privacy #111245

fee1-dead · 2023-05-05T10:53:48Z

This PR fixes #111220 by checking the privacy of tuple constructors using Self, so the following code now errors

mod my {
    pub struct Foo(&'static str);
}

impl AsRef<str> for my::Foo {
    fn as_ref(&self) -> &str {
        let Self(s) = self; // previously compiled, now errors correctly
        s
    }
}

rustbot · 2023-05-05T10:53:54Z

r? @TaKO8Ki

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

m-ou-se · 2023-05-05T11:04:18Z

compiler/rustc_privacy/src/lib.rs

+                            def: self.tcx.def_path_str(adt_def.did()),
+                        });
+                    }
+                    debug!("hello");


Noratrieb · 2023-05-05T11:27:18Z

Do we want a crater run here? I think it wouldn't hurt to see what happens, not that we won't ship a fix but maybe we want a grace period and an FCW (which, given that this was broken forever, we could afford). Gonna start a try build for that.
@bors try

bors · 2023-05-05T11:27:28Z

⌛ Trying commit d110c0096ebfbabed16d50f60fb008737d055238 with merge c7dcd44b00f701bf5d912cfabfad0f07883de539...

bors · 2023-05-05T13:07:18Z

☀️ Try build successful - checks-actions
Build commit: c7dcd44b00f701bf5d912cfabfad0f07883de539 (c7dcd44b00f701bf5d912cfabfad0f07883de539)

fee1-dead · 2023-05-05T13:08:57Z

@craterbot check

craterbot · 2023-05-05T13:09:06Z

👌 Experiment pr-111245 created and queued.
🤖 Automatically detected try build c7dcd44b00f701bf5d912cfabfad0f07883de539
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2023-05-05T13:09:09Z

🚧 Experiment pr-111245 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2023-05-06T10:56:45Z

🎉 Experiment pr-111245 is completed!
📊 1 regressed and 2 fixed (267036 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr · 2023-05-08T07:41:18Z

compiler/rustc_privacy/src/lib.rs

@@ -1309,6 +1309,23 @@ impl<'tcx> Visitor<'tcx> for TypePrivacyVisitor<'tcx> {
        let def = match qpath {
            hir::QPath::Resolved(_, path) => match path.res {
                Res::Def(kind, def_id) => Some((kind, def_id)),
+                // HACK: prevent using the `Self` ctor when the concrete type is private.
+                Res::SelfCtor(impl_) => {


it's very annoying, but we normalize before looking up the constructor iirc, so the following should still be an issue:

mod b { #[derive(Default)] pub struct A(u32); } pub trait B { fn f(&self); } pub trait Id { type Assoc; } impl<T> Id for T { type Assoc = Self; } impl B for <b::A as Id>::Assoc { fn f(&self) { let Self(a) = self; //~^ ERROR: tuple struct constructor `A` is private println!("{}", a); } } fn main() { let a = b::A::default(); a.f(); }

while ugly, it might be safer to do the check in astconv or sth 🤔 I am not familiar with this code

It doesn't allow projections:

error: the `Self` constructor can only be used with tuple or unit structs --> /home/gh-fee1-dead/rust/tests/ui/privacy/issue-111220-tuple-struct-fields.rs:28:13 | LL | let Self(a) = self; | ^^^^^^^ error[E0164]: expected tuple struct or tuple variant, found self constructor `Self` --> /home/gh-fee1-dead/rust/tests/ui/privacy/issue-111220-tuple-struct-fields.rs:28:13 | LL | let Self(a) = self; | ^^^^^^^ not a tuple struct or tuple variant

what? 😅 this compiles on nightly: https://rust.godbolt.org/z/nM6vzrc5Y

how does your code change this '^^ please add this as a test

Ahh, I turned b::A::Assoc into (b::A,) which isn't a tuple struct lol. This is indeed an issue, I will look into this when I get time.

I used try_normalize_erasing_regions which should fix this for impl Trait for Foo::AssocTy. Let me know if this should totally be at some other crate and not rustc_privacy. I mainly put this stuff here so I can use item_is_accessible.

I think normalizing it with the impl where bounds is still not enough as you can avoid it as follows (please add this as a test)

mod b { pub struct A(u32); } trait Id { type Assoc; } impl Id for b::A { type Assoc = b::A; } impl Id for u32 { type Assoc = u32; } trait Trait<T> { fn method(&self) where T: Id<Assoc = b::A>; } impl<T: Id> Trait<T> for <T as Id>::Assoc { fn method(&self) where T: Id<Assoc = b::A>, { let Self(a) = self; println!("{a}"); } }

You would have to normalize in the param_env of self.current_item I think but even then it feels somewhat dangerous.

Can you add a check to instantiate_value_path that the used ctor for is actually accessible 🤔 either completely move this check to there or at least delay_span_bug if we expect it to error.

Yeah, I think this could be moved to typeck.

fee1-dead · 2023-05-21T04:31:41Z

@rustbot author

fee1-dead · 2023-05-25T02:33:40Z

Moved to typeck.

@rustbot ready

lcnr · 2023-05-25T10:03:39Z

r=me after updating the PR description. I am also not sure whether I would consider this fix to be "temporary" 🤔

I guess we can mention this in the relnotes, even if it feels very unlikely that someone will depend on this. Crater did not show any regressions.

This PR fixes #111220 by checking the privacy of tuple constructors using Self, so the following code now errors

mod my {
    pub struct Foo(&'static str);
}

impl AsRef<str> for my::Foo {
    fn as_ref(&self) -> &str {
        let Self(s) = self; // previously compiled, now errors correctly
        s
    }
}

fee1-dead · 2023-05-25T14:02:49Z

@bors r=lcnr

bors · 2023-05-25T14:02:53Z

📌 Commit dee65a4 has been approved by lcnr

It is now in the queue for this repository.

…truct-field, r=lcnr fix for `Self` not respecting tuple Ctor privacy This PR fixes rust-lang#111220 by checking the privacy of tuple constructors using `Self`, so the following code now errors ```rust mod my { pub struct Foo(&'static str); } impl AsRef<str> for my::Foo { fn as_ref(&self) -> &str { let Self(s) = self; // previously compiled, now errors correctly s } } ```

bors · 2023-05-26T06:20:06Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout temp-fix-tuple-struct-field (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self temp-fix-tuple-struct-field --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Auto-merging compiler/rustc_hir_typeck/messages.ftl
CONFLICT (content): Merge conflict in compiler/rustc_hir_typeck/messages.ftl
Automatic merge failed; fix conflicts and then commit the result.

This fixes rust-lang#111220 by checking the privacy of tuple constructors using `Self`, so the following code now errors ```rust mod my { pub struct Foo(&'static str); } impl AsRef<str> for my::Foo { fn as_ref(&self) -> &str { let Self(s) = self; // previously compiled, now errors correctly s } } ```

lcnr · 2023-05-26T08:18:49Z

@bors r+

bors · 2023-05-26T08:18:51Z

📌 Commit be44860 has been approved by lcnr

It is now in the queue for this repository.

…truct-field, r=lcnr fix for `Self` not respecting tuple Ctor privacy This PR fixes rust-lang#111220 by checking the privacy of tuple constructors using `Self`, so the following code now errors ```rust mod my { pub struct Foo(&'static str); } impl AsRef<str> for my::Foo { fn as_ref(&self) -> &str { let Self(s) = self; // previously compiled, now errors correctly s } } ```

bors · 2023-05-27T01:27:04Z

⌛ Testing commit be44860 with merge 23040c4...

bors · 2023-05-27T04:20:36Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 23040c4 to master...

rust-timer · 2023-05-27T05:35:11Z

Finished benchmarking commit (23040c4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.9%	[5.9%, 5.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.5%, -1.0%]	2
All ❌✅ (primary)	5.9%	[5.9%, 5.9%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.182s -> 645.695s (0.23%)

rustbot assigned TaKO8Ki May 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 5, 2023

m-ou-se reviewed May 5, 2023

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023

fee1-dead force-pushed the temp-fix-tuple-struct-field branch from d110c00 to 587900b Compare May 5, 2023 13:29

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 6, 2023

lcnr reviewed May 8, 2023

View reviewed changes

fee1-dead force-pushed the temp-fix-tuple-struct-field branch from 587900b to 65ba58e Compare May 11, 2023 12:22

lcnr self-assigned this May 11, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2023

fee1-dead force-pushed the temp-fix-tuple-struct-field branch from 65ba58e to 3e715d9 Compare May 24, 2023 15:20

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2023

lcnr added the relnotes Marks issues that should be documented in the release notes of the next release. label May 25, 2023

fee1-dead changed the title ~~temporary fix for Self not respecting tuple Ctor privacy~~ fix for Self not respecting tuple Ctor privacy May 25, 2023

fee1-dead force-pushed the temp-fix-tuple-struct-field branch from 3e715d9 to dee65a4 Compare May 25, 2023 14:02

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 May 25, 2023

compiler-errors mentioned this pull request May 26, 2023

Rollup of 7 pull requests #111981

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 26, 2023

fee1-dead force-pushed the temp-fix-tuple-struct-field branch from dee65a4 to be44860 Compare May 26, 2023 06:24

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2023

Dylan-DPC mentioned this pull request May 26, 2023

Rollup of 6 pull requests #111993

Closed

compiler-errors mentioned this pull request May 26, 2023

Rollup of 7 pull requests #112003

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2023

bors merged commit 23040c4 into rust-lang:master May 27, 2023

rustbot added this to the 1.71.0 milestone May 27, 2023

fee1-dead deleted the temp-fix-tuple-struct-field branch May 27, 2023 05:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for `Self` not respecting tuple Ctor privacy #111245

fix for `Self` not respecting tuple Ctor privacy #111245

fee1-dead commented May 5, 2023 •

edited

Loading

rustbot commented May 5, 2023

m-ou-se May 5, 2023

BoxyUwU May 5, 2023

Noratrieb commented May 5, 2023 •

edited

Loading

bors commented May 5, 2023

bors commented May 5, 2023

fee1-dead commented May 5, 2023

craterbot commented May 5, 2023

craterbot commented May 5, 2023

craterbot commented May 6, 2023

lcnr May 8, 2023

fee1-dead May 8, 2023

lcnr May 8, 2023 •

edited

Loading

fee1-dead May 8, 2023

fee1-dead May 11, 2023

lcnr May 19, 2023

compiler-errors May 19, 2023

fee1-dead commented May 21, 2023

fee1-dead commented May 25, 2023

lcnr commented May 25, 2023

fee1-dead commented May 25, 2023

bors commented May 25, 2023

bors commented May 26, 2023

lcnr commented May 26, 2023

bors commented May 26, 2023

bors commented May 27, 2023

bors commented May 27, 2023

rust-timer commented May 27, 2023

fix for Self not respecting tuple Ctor privacy #111245

fix for Self not respecting tuple Ctor privacy #111245

Conversation

fee1-dead commented May 5, 2023 • edited Loading

rustbot commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented May 5, 2023 • edited Loading

bors commented May 5, 2023

bors commented May 5, 2023

fee1-dead commented May 5, 2023

craterbot commented May 5, 2023

craterbot commented May 5, 2023

craterbot commented May 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fee1-dead commented May 21, 2023

fee1-dead commented May 25, 2023

lcnr commented May 25, 2023

fee1-dead commented May 25, 2023

bors commented May 25, 2023

bors commented May 26, 2023

lcnr commented May 26, 2023

bors commented May 26, 2023

bors commented May 27, 2023

bors commented May 27, 2023

rust-timer commented May 27, 2023

Overall result: no relevant changes - no action needed

fix for `Self` not respecting tuple Ctor privacy #111245

fix for `Self` not respecting tuple Ctor privacy #111245

fee1-dead commented May 5, 2023 •

edited

Loading

Noratrieb commented May 5, 2023 •

edited

Loading

lcnr May 8, 2023 •

edited

Loading