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

rustc: treat const bodies like fn bodies in middle::region. #41515

Merged
merged 1 commit into from
May 9, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 24, 2017

Allows T::ASSOC_CONST to be used without a T: 'static bound.

cc @rust-lang/compiler @rust-lang/lang

@rust-highfive
Copy link
Collaborator

r? @brson

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

@eddyb
Copy link
Member Author

eddyb commented Apr 24, 2017

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned brson Apr 24, 2017
@arielb1
Copy link
Contributor

arielb1 commented Apr 25, 2017

Can't this drop WF constraints on types in associated constants? A: no, it's a regionck method. But because we want to allow free fn calls etc. in associated constants, isn't it better to teach regionck that the "outer scope" of associated constants is not in fact 'static but rather some kind of anonymous free region?

@eddyb
Copy link
Member Author

eddyb commented Apr 25, 2017

I don't really understand what the point of checking substitutions is, can someone explain that?

@arielb1
Copy link
Contributor

arielb1 commented Apr 25, 2017

The idea is that computing <&'a T as Trait>::CONST requires that T: 'a at the use-site. This shouldn't be needed for soundness (because of "region truncation") but sounds like a good property.

We still want regionck to be smarter, tho (because of calls and the like).

@eddyb eddyb changed the title regionck: don't enforce scope validity of substs. rustc: treat const bodies like fn bodies in middle::region. Apr 25, 2017
@arielb1
Copy link
Contributor

arielb1 commented Apr 25, 2017

You need to fix "droplessness" - have temporary_scope of constants be None again.

[00:16:48] error: borrowed value does not live long enough
[00:16:48]    --> /checkout/src/libcore/char_private.rs:370:33
[00:16:48]     |
[00:16:48] 370 |   const NORMAL1: &'static [u8] = &[
[00:16:48]     |  _________________________________^
[00:16:48] 371 | |     0x5e, 0x22,
[00:16:48] 372 | |     0x7b, 0x05,
[00:16:48] 373 | |     0x03, 0x04,
[00:16:48] ...   |
[00:16:48] 521 | |     0x01, 0x86, 0x3f,
[00:16:48] 522 | | ];
[00:16:48]     | | -
[00:16:48]     | |_|
[00:16:48]     |   does not live long enough
[00:16:48]     |   temporary value only lives until here
[00:16:48]     |
[00:16:48]     = note: borrowed value must be valid for the static lifetime...
[00:16:48] 
[00:16:48] error: aborting due to 70 previous errors
[00:16:48] 
[00:16:48] error: Could not compile `core`.

@eddyb
Copy link
Member Author

eddyb commented Apr 25, 2017

@arielb1 Yeah, I was telling @nikomatsakis that I have no clue how I would do this.

@nikomatsakis
Copy link
Contributor

So I've got a rather meandering PR (that is now working, though) that refactors the region-maps stuff somewhat. (It converts them into queries keyed by the enclosing item-like, in particular.) It doesn't directly bear on this, I guess, though it certainly...interacts. It doesn't seem too hard to have the idea of a "static temporary scope", though.

Right now we have this "terminating scopes" set, I guess one way would be to make that into a map where the value is a Region, so that when we are searching for the innermost temporary scope, if we encounter a terminating scope, we use the value (which, for statics would be 'static, but otherwise would be a scope. Alternatively, we could skip the step of inserting into the terminating scopes set, and use the fact that we never find a terminating scope as a signal; this makes me mildly uncomfortable, but it could work.

One of the things I've been wondering about here:

For constant expressions like [foo; 3], I guess that the scopes involved with 3 ought to be distinct from the enclosing function, right?

@eddyb
Copy link
Member Author

eddyb commented Apr 25, 2017

@nikomatsakis Yeah I was wondering if I should have a visit_ty method that isolates nested expressions like that. I'll try removing the terminating scope for now, I guess?

@nikomatsakis
Copy link
Contributor

@eddyb i.e., specifically in this case? You'll have to make sure that there is no parent scope, as well, since otherwise we'll walk right into the enclosing function as we traverse the scope tree. But given that these are forced to be a 'constant context' that seems correct anyhow.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2017
@arielb1
Copy link
Contributor

arielb1 commented May 2, 2017

@eddyb - are you still working on this?

@eddyb
Copy link
Member Author

eddyb commented May 2, 2017

Yeah, I'm waiting for #41662 to land first.

@bors
Copy link
Contributor

bors commented May 2, 2017

☔ The latest upstream changes (presumably #41702) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented May 6, 2017

Okay, turns out I was close, but @nikomatsakis had to remind me that I had to update ParameterEnvironment::for_item so it uses the correct outermost scope for constants.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2017
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2017

📌 Commit 90af729 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented May 8, 2017

⌛ Testing commit 90af729 with merge f1140a3...

bors added a commit that referenced this pull request May 8, 2017
rustc: treat const bodies like fn bodies in middle::region.

Allows `T::ASSOC_CONST` to be used without a `T: 'static` bound.

cc @rust-lang/compiler @rust-lang/lang
@bors
Copy link
Contributor

bors commented May 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing f1140a3 to master...

@bors bors merged commit 90af729 into rust-lang:master May 9, 2017
@eddyb eddyb deleted the non-static-assoc-const branch May 9, 2017 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants