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

Ensure that the type parameters passed to methods outlive the call expression #18940

Merged
merged 2 commits into from
Nov 20, 2014

Conversation

nikomatsakis
Copy link
Contributor

Ensure that the type parameters passed to methods outlive the call expression.

Fixes #18899.

This is yet another case of forgotten to consistently enforce the constraints in every instance where they apply. Might be nice to try and refactor to make this whole thing more DRY, but for now here's a targeted fix.

r? @pcwalton

@@ -481,11 +482,13 @@ struct LookupContext<'a, 'tcx: 'a> {
fcx: &'a FnCtxt<'a, 'tcx>,
span: Span,

call_expr: &'a ast::Expr,

// The receiver to the method call. Only `None` in the case of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of date now that self_expr isn't Option<_>

@nikomatsakis
Copy link
Contributor Author

@tomjakubowski thanks, fixed.

@nikomatsakis
Copy link
Contributor Author

This fix causes compilation failures in the libcollections unit tests. These look like inference failures, i.e., inference not taking into account all the constraints and hence inferring a region that is too small, but I could be wrong.

@nikomatsakis
Copy link
Contributor Author

(It's easy enough to fix the tests, but I want to be sure that we're not reporting spurious errors first.)

@nikomatsakis
Copy link
Contributor Author

Actually, I think the bitv test code was doing unsound things.

@nikomatsakis
Copy link
Contributor Author

Fixed. Might be worth trying to check with author of 665555d, which is the commit that introduced these opaque reads, but I think my change preserves the intent.

@nikomatsakis
Copy link
Contributor Author

Hello @lpy, can you please examine commit 5ec3806 to see if you believe that it will continue to ensure the benchmarks are correctly measuring what they intend to measure? Your original patch was only compiling due to a bug in the type checker which was undetected until recently. (It's explained in the 5ec3806 comment)

@nikomatsakis
Copy link
Contributor Author

OK, pushed a new revision to the bitv tests (as suggested by @huonw) that seems to hew closer to the original code.

…return a pointer to the value they were modifying, but this should not have been legal, since that pointer would have to outlive the closure, and the closure continues to modify the value during the execution. This return value was just passed to `black_box` so as to convince llvm that the value was live, so rather than returning a pointer, modify to just call `black_box` directly inside the fn.
@bors bors merged commit 2477bc4 into rust-lang:master Nov 20, 2014
@nikomatsakis nikomatsakis deleted the issue-18899 branch March 30, 2016 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closures in methods can return pointers with short lifetimes
4 participants