Uh oh!
There was an error while loading. Please reload this page.
Fix #50865: ICE on impl-trait returning functions reaching private items - #53545
Conversation
| impl<Id: Hash + Eq> AccessLevels<Id> { | ||
| pub fn is_reachable(&self, id: Id) -> bool { | ||
| self.map.contains_key(&id) | ||
| // self.map.contains_key(&id) |
There was a problem hiding this comment.
do not leave commented out code
| @@ -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 | |||
| } | ||
| fn visit_item(&mut self, item: &'tcx hir::Item) { | ||
| debug!("Walked item {:?}", item); |
There was a problem hiding this comment.
Usually this would be something like: debug!("visit_item({:?})", item);, so it's easier to see where it comes from.
| // 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); |
There was a problem hiding this comment.
Perhaps just item_level = {:?} is sufficient?
This comment has been minimized.
This comment has been minimized.
FelixMcFelix
commented
Aug 21, 2018
Applied suggestions, thanks for the feedback. |
eddyb
commented
Aug 21, 2018
| name = "rustc_privacy" | ||
| version = "0.0.0" | ||
| dependencies = [ | ||
| "log 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", |
There was a problem hiding this comment.
Could you avoid introducing this new dependency?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
| let orig_level = self.prev_level; | ||
| self.prev_level = item_level; |
There was a problem hiding this comment.
Nit: let orig_level = mem::replace(&mut self.prev_level, item_level);
There was a problem hiding this comment.
Hmm, I'm not sure why these lines were moved from there to here actually.
There was a problem hiding this comment.
Ok. This was moved while I was figuring out how best to propagate the level from the ReachEverythingInTheInterfaceVisitor. Will move back and modify.
There was a problem hiding this comment.
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.
| if let Some(node_id) = self.tcx.hir.as_local_node_id(impl_trait_fn) { | ||
| self.update(node_id, Some(AccessLevel::ReachableFromImplTrait)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you move this piece of code from here to // Update levels of nested things below, it belongs there logically.
petrochenkov
commented
Aug 24, 2018
@bors r+ |
bors
commented
Aug 24, 2018
📌 Commit 85a05d1 has been approved by |
…ochenkov Fixrust-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. Fixesrust-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.)
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_reachablereturns false for such items (leaving stability analysis unaffected), these items may still be visible to the lints phase however.