Fix #50865: ICE on impl-trait returning functions reaching private items#53545

FelixMcFelix · 2018-08-20T23:29:08Z

Adds a test case as suggested in #50865, and implements @petrochenkov's suggestion. Fixes #50865.

Impl-trait-returning functions are marked under a new (low) access level, which they propagate rather than AccessLevels::Reachable. AccessLevels::is_reachable returns false for such items (leaving stability analysis unaffected), these items may still be visible to the lints phase however.

estebank

lgtm

r? @eddyb

estebank · 2018-08-20T23:35:45Z

src/librustc/middle/privacy.rs

 impl<Id: Hash + Eq> AccessLevels<Id> {
    pub fn is_reachable(&self, id: Id) -> bool {
-        self.map.contains_key(&id)
+        // self.map.contains_key(&id)


do not leave commented out code

estebank · 2018-08-20T23:37:32Z

src/test/run-pass/issue-50865-private-impl-trait/main.rs

@@ -0,0 +1,20 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update year.

estebank · 2018-08-20T23:37:39Z

src/test/run-pass/issue-50865-private-impl-trait/auxiliary/lib.rs

@@ -0,0 +1,24 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT


Update year.

varkor · 2018-08-21T00:34:03Z

src/librustc_privacy/lib.rs

    }

    fn visit_item(&mut self, item: &'tcx hir::Item) {
+        debug!("Walked item {:?}", item);


Usually this would be something like: debug!("visit_item({:?})", item);, so it's easier to see where it comes from.

varkor · 2018-08-21T00:35:34Z

src/librustc_privacy/lib.rs

        // Update level of the item itself
        let item_level = self.update(item.id, inherited_item_level);

+        debug!("Its privacy is believed to be: {:?}", item_level);


Perhaps just item_level = {:?} is sufficient?

FelixMcFelix · 2018-08-21T00:52:01Z

Applied suggestions, thanks for the feedback.

eddyb · 2018-08-21T06:10:28Z

r? @petrochenkov

petrochenkov · 2018-08-22T23:36:57Z

src/Cargo.lock

 name = "rustc_privacy"
 version = "0.0.0"
 dependencies = [
+ "log 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",


Could you avoid introducing this new dependency?

Any debug!("...") calls would then also need to be removed. Is this version of log different from that used by the rest of the rustc_* crates?

debug!(...) logging used for a specific debugging session is usually useless for anything else in the future, so I mean yes, it is better removed.

petrochenkov · 2018-08-22T23:41:23Z

src/librustc_privacy/lib.rs

        }

+        let orig_level = self.prev_level;
+        self.prev_level = item_level;


Nit: let orig_level = mem::replace(&mut self.prev_level, item_level);

Hmm, I'm not sure why these lines were moved from there to here actually.

Ok. This was moved while I was figuring out how best to propagate the level from the ReachEverythingInTheInterfaceVisitor. Will move back and modify.

Actually, this was moved so that EmbargoVisitor::reach would be able to see whether the parent node was ReachableFromImplTrait or at least Reachable, to propagate the correct level.

petrochenkov · 2018-08-22T23:45:36Z

src/librustc_privacy/lib.rs

+                    if let Some(node_id) = self.tcx.hir.as_local_node_id(impl_trait_fn) {
+                        self.update(node_id, Some(AccessLevel::ReachableFromImplTrait));
+                    }
+                }


Could you move this piece of code from here to // Update levels of nested things below, it belongs there logically.

petrochenkov · 2018-08-24T14:59:14Z

@bors r+

bors · 2018-08-24T14:59:15Z

📌 Commit 85a05d1 has been approved by petrochenkov

@petrochenkov

…ochenkov Fix rust-lang#50865: ICE on impl-trait returning functions reaching private items Adds a test case as suggested in rust-lang#50865, and implements @petrochenkov's suggestion. Fixes rust-lang#50865. Impl-trait-returning functions are marked under a new (low) access level, which they propagate rather than `AccessLevels::Reachable`. `AccessLevels::is_reachable` returns false for such items (leaving stability analysis unaffected), these items may still be visible to the lints phase however.

Rollup of 16 pull requests Successful merges: - #53311 (Window Mutex: Document that we properly initialize the SRWLock) - #53503 (Discourage overuse of mem::forget) - #53545 (Fix #50865: ICE on impl-trait returning functions reaching private items) - #53559 (add macro check for lint) - #53562 (Lament the invincibility of the Turbofish) - #53563 (use String::new() instead of String::from(""), "".to_string(), "".to_owned() or "".into()) - #53592 (docs: minor stylistic changes to str/string docs) - #53594 (Update RELEASES.md to include clippy-preview) - #53600 (Fix a grammatical mistake in "expected generic arguments" errors) - #53614 (update nomicon and book) - #53617 (tidy: Stop requiring a license header) - #53618 (Add missing fmt examples) - #53636 (Prefer `.nth(n)` over `.skip(n).next()`.) - #53644 (Use SmallVec for SmallCStr) - #53664 (Remove unnecessary closure in rustc_mir/build/mod.rs) - #53666 (Added rustc_codegen_llvm to compiler documentation.)

FelixMcFelix added 4 commits July 20, 2018 14:12

Add test case for rust-lang#50865 regression.

0603a73

Logging for rustc_privacy.

f285876

Mark impl Trait Functions as reachable. (Fixes rust-lang#50865)

00b2606

New AccessLevel and accompanying propagation.

81684bf

estebank reviewed Aug 20, 2018

View reviewed changes

Fixes for code review.

54b096a

varkor reviewed Aug 21, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Further fixes.

3536359

rust-highfive assigned petrochenkov Aug 21, 2018

XAMPPRocky added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2018

petrochenkov reviewed Aug 22, 2018

View reviewed changes

petrochenkov 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 Aug 22, 2018

Light restructuring.

85a05d1

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 Aug 24, 2018

kennytm mentioned this pull request Aug 24, 2018

Rollup of 16 pull requests #53662

Merged

bors merged commit 85a05d1 into rust-lang:master Aug 24, 2018

FelixMcFelix deleted the fix-50865-beta branch August 28, 2018 14:50

jonas-schievink mentioned this pull request Oct 2, 2018

No warning for unused function using impl Trait on beta and nightly #54754

Closed

petrochenkov mentioned this pull request Dec 29, 2018

privacy: Use common DefId visiting infrastructure for all privacy visitors #56878

Merged

		@@ -0,0 +1,20 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

		@@ -0,0 +1,24 @@
		// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

Uh oh!

Conversation

FelixMcFelix commented Aug 20, 2018 • edited by varkor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

FelixMcFelix commented Aug 21, 2018

Uh oh!

eddyb commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Aug 24, 2018

Uh oh!

bors commented Aug 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

FelixMcFelix commented Aug 20, 2018 •

edited by varkor

Loading