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

Properly pass down immutability info for thread-locals. #47425

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

EdSchouten
Copy link
Contributor

For thread-locals we call into cat_rvalue_node() to create a CMT
(Category, Mutability, Type) that always has McDeclared. This is
incorrect for thread-locals that don't have the 'mut' keyword; we should
use McImmutable there.

Extend cat_rvalue_node() to have an additional mutability parameter. Fix
up all the callers to make use of that function. Also extend one of the
existing unit tests to cover this.

Fixes: #47053

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

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

@kennytm kennytm added T-lang Relevant to the language 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 Jan 14, 2018
@eddyb
Copy link
Member

eddyb commented Jan 15, 2018

r? @nikomatsakis

This is only needed because of how #[thread_local] is hackily handled in the old borrowck.
Does the MIR borrowck treat references to #[thread_local] statics as to function-local temporaries?

@nikomatsakis
Copy link
Contributor

This is indeed fixed in MIR borrowck, as you can see from this example. It may still be worth fixing in AST borrowck, if it's easy -- we're still going to need some version of the mem-cat / expr-use-visitor code for upvar inference, though I hope we'll be able to greatly simplify it.

@nikomatsakis
Copy link
Contributor

This is only needed because of how #[thread_local] is hackily handled in the old borrowck.

OK, I think I see what you are saying. I don't think I realized how this was being handled. Basically I guess thread-locals are being converted into rvalues so that borrowck will assign a more limited lifetime? That does indeed feel like rather a hack.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 16, 2018

Maybe I should take a look at what it might mean to integrate more "properly" into borrowck? It feels like that would likely be very easy to do (i.e., maybe even a smaller diff than what is here).
Or @EdSchouten if you feel like digging you can do some explring on your own. =)

I think the main thing is that this line has to change:

Categorization::StaticItem => {
Ok(())
}

This basically says that a loan of static content can have any lifetime. But in the case of a static item tagged with #[thread_local], we would want to check that the loan lifetime does not exceed current function (mostly, it should be a ReScope, but we might have to be careful around closures).

@EdSchouten
Copy link
Contributor Author

Hi @nikomatsakis! That does sound like a decent approach. That said, I've only been using Rust for the last four weeks. I was already quite happy to come up with this patch. I suspect it would be wiser for you to take a stab at this. Would that be all right?

@nikomatsakis
Copy link
Contributor

Hmm, in that case, I might be inclined to let this "fix itself" as we transition to the MIR-based borrowck.

@EdSchouten
Copy link
Contributor Author

EdSchouten commented Jan 17, 2018

That sounds reasonable to me. Before closing this pull request: do we want to add this as a unit test for the new borrowck?

@nikomatsakis
Copy link
Contributor

@EdSchouten yep, I've been tracking such bugs in #47366 -- I'll add #47053 to that list

@nikomatsakis
Copy link
Contributor

@EdSchouten maybe repurpose this PR for that purpose? #47366 has a bit of directions for how to go about it.

It is currently allowed to perform such assignments when not making use
of NLL. NLL already does this right, but let's add a test in place to
ensure it never regresses.
@EdSchouten
Copy link
Contributor Author

I've just reworked this PR to only add testing coverage for NLL. Does this look all right?

@nikomatsakis
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 22, 2018

📌 Commit e47cc69 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@EdSchouten looks great =)

@nikomatsakis nikomatsakis 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 Jan 22, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jan 23, 2018
…sakis

Properly pass down immutability info for thread-locals.

For thread-locals we call into cat_rvalue_node() to create a CMT
(Category, Mutability, Type) that always has McDeclared. This is
incorrect for thread-locals that don't have the 'mut' keyword; we should
use McImmutable there.

Extend cat_rvalue_node() to have an additional mutability parameter. Fix
up all the callers to make use of that function. Also extend one of the
existing unit tests to cover this.

Fixes: rust-lang#47053
bors added a commit that referenced this pull request Jan 23, 2018
Rollup of 14 pull requests

- Successful merges: #47423, #47425, #47440, #47541, #47549, #47554, #47558, #47610, #47635, #47655, #47661, #47662, #47667, #47672
- Failed merges:
@bors bors merged commit e47cc69 into rust-lang:master Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants