in region, treat current (and future) item-likes alike #37920
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For example, the following lint could do: #![feature(plugin_registrar, rustc_private)]
extern crate syntax;
#[macro_use]
extern crate rustc;
extern crate rustc_plugin;
use rustc::lint::{LateContext, LintPass, LateLintPass, LintArray, LintContext};
use rustc::hir;
use rustc::hir::intravisit::FnKind;
use rustc::middle::region::CodeExtent;
use rustc::util::nodemap::FnvHashMap;
use syntax::ast::{self, NodeId};
use syntax::codemap::Span;
declare_lint!(REGION_HIERARCHY, Warn, "warn about bogus region hierarchy");
struct Pass {
map: FnvHashMap<CodeExtent, NodeId>
}
impl LintPass for Pass {
fn get_lints(&self) -> LintArray { lint_array!(REGION_HIERARCHY) }
}
impl LateLintPass for Pass {
fn check_fn(&mut self, cx: &LateContext,
fk: FnKind, _: &hir::FnDecl, expr: &hir::Block,
span: Span, node: ast::NodeId)
{
if let FnKind::Closure(..) = fk { return }
let mut extent = cx.tcx.region_maps.node_extent(expr.id);
while let Some(parent) = cx.tcx.region_maps.opt_encl_scope(extent) {
extent = parent;
}
if let Some(other) = self.map.insert(extent, node) {
cx.span_lint(REGION_HIERARCHY, span, &format!(
"different fns {:?}, {:?} with the same root extent {:?}",
cx.tcx.map.local_def_id(other),
cx.tcx.map.local_def_id(node),
extent));
}
}
}
#[plugin_registrar]
pub fn plugin_registrar(reg: &mut ::rustc_plugin::Registry) {
reg.register_late_lint_pass(Box::new(
Pass { map: FnvHashMap() }
));
} |
|
The fix looks good to me. Do you still want to experiment with adding a test case for this? |
I'm torn. The idea of a custom lint is clever. But it seems like a lot of effort for a data structure I would hope that we will overhaul in the not that distant future... |
|
Well, @arielb1's lint basically works right off, so I might as well add it =) |
|
The CI is failing due to some C++ compile error involving LLVM that I don't understand. |
nikomatsakis
force-pushed
the
compile-time-regression-37864
branch
from
cd027af
to
3218664
Compare
|
☔ The latest upstream changes (presumably #37918) made this pull request unmergeable. Please resolve the merge conflicts. |
arielb1
reviewed
Nov 29, 2016
| // aux-build:lint.rs | ||
|
|
||
| #![feature(plugin)] | ||
| #![plugin(lint)] |
There was a problem hiding this comment.
Don't you need #![deny(region_hierarchy)] so that the test will actually fail?
There was a problem hiding this comment.
I suppose I do...
The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After rust-lang#37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes rust-lang#37864.
nikomatsakis
force-pushed
the
compile-time-regression-37864
branch
from
3218664
to
6fe4bff
Compare
|
@bors r=mw |
|
📌 Commit 6fe4bff has been approved by |
bors
added a commit
that referenced
this pull request
Dec 2, 2016
in region, treat current (and future) item-likes alike The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After #37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes #37864. r? @michaelwoerister cc @pnkfelix -- can you think of a way to make a regr test?
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
tidy error: |
|
Bah humbug. |
|
@bors r=mw |
|
📌 Commit 0adb1b1 has been approved by |
|
⌛ Testing commit 0adb1b1 with merge 4ba1de1... |
|
💔 Test failed - auto-mac-64-opt |
|
@bors: retry
…On Fri, Dec 2, 2016 at 1:05 PM, bors ***@***.***> wrote:
💔 Test failed - auto-mac-64-opt
<https://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/11192>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#37920 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD95CVgvXsqWY3L8tAtb2_LUA_HV27Xks5rEIgsgaJpZM4K4iuy>
.
|
bors
added a commit
that referenced
this pull request
Dec 2, 2016
in region, treat current (and future) item-likes alike The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After #37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes #37864. r? @michaelwoerister cc @pnkfelix -- can you think of a way to make a regr test?
|
💔 Test failed - auto-linux-musl-64-opt |
The test needs to be in |
|
@bors r=mw |
|
📌 Commit f809706 has been approved by |
bors
added a commit
that referenced
this pull request
Dec 4, 2016
in region, treat current (and future) item-likes alike The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After #37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes #37864. r? @michaelwoerister cc @pnkfelix -- can you think of a way to make a regr test?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
visit_fncode mutates its surrounding context. Between items,this was saved/restored, but between impl items it was not. This meant
that we wound up with
CallSiteScopeentries with two parents (ormore!). As far as I can tell, this is harmless in actual type-checking,
since the regions you interact with are always from at most one of those
branches. But it can slow things down.
Before, the effect was limited, since it only applied to impl items
within an impl. After #37660, impl items are visisted all together at
the end, and hence this could create a very messed up
hierarchy. Isolating impl item properly solves both issues.
I cannot come up with a way to unit-test this; for posterity, however,
you can observe the messed up hierarchies with a test as simple as the
following, which would create a callsite scope with two parents both
before and after
Fixes #37864.
r? @michaelwoerister
cc @pnkfelix -- can you think of a way to make a regr test?