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

Don't drag function signatures along function item types. #42417

Merged
merged 4 commits into from
Jun 28, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 4, 2017

This PR separates the signature of a function from the "function item type" (TyFnDef), leaving only the DefId and parameter Substs, making them even more like (captureless) closure types.

The motivation for this change is reducing typesystem complexity, and its consequences:

  • operating on the signature instead of just the parameters was less efficient
    • specifically, signatures can easily add several levels of depth on top of the parameter types
    • and the signatured were always substituted and normalized, so typically even more complex
  • it was the only type that was both nominal (identity) and structural (signature)
    • harder to model in Chalk than either a purely nominal or structural type
    • subtyping worked on the signature but parameters were always invariant
    • call type-checking was transforming signatures but keeping the nominal half intact
    • the signature could therefore get out of sync during type inference in several ways

That last point comes with a [breaking-change], because functions with 'static in their return types will now not be as usable as if they were using lifetime parameters instead:

// Will cause lifetime mismatch in main after this PR.
fn bar() -> &'static str { "bar" }
// Will continue to work fine, as every use can choose its own lifetime.
fn bar<'a>() -> &'a str { "bar" }

fn main() {
    let s = String::from("foo");
    Some(&s[..]).unwrap_or_else(bar);
}

r? @nikomatsakis

@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 4, 2017
@eddyb eddyb requested a review from nikomatsakis June 4, 2017 01:02
substs.fold_with(folder),
f.fold_with(folder))
ty::TyFnDef(def_id, substs) => {
ty::TyFnDef(def_id, substs.fold_with(folder))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay that signatures are not folded anymore? (Is it possible to fold something kept in the tables at all?)
It would be really nice if they were at least still visited by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole point (which I'll try to describe once I'm sure there's no fatal flaw with the approach) is to reduce the amount of work done for these types. Anything that relied on seeing the signature has to visit it manually. It's not a property of the type and it was never meant to be, it's just one way to use values of that type (i.e. calling them).

@Mark-Simulacrum
Copy link
Member

@bors try

This will get us artifacts for cargobomb.

@bors
Copy link
Contributor

bors commented Jun 4, 2017

⌛ Trying commit 198067e with merge c7ced21...

@bors
Copy link
Contributor

bors commented Jun 4, 2017

💔 Test failed - status-travis

@Mark-Simulacrum
Copy link
Member

Don't retry until the previous try finishes one way or another.

@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Jun 4, 2017

⌛ Trying commit 87a65f2 with merge ebf4c15...

@Mark-Simulacrum
Copy link
Member

Oh right

@bors retry

@bors
Copy link
Contributor

bors commented Jun 4, 2017

⌛ Trying commit 87a65f2 with merge ab0e704...

@bors
Copy link
Contributor

bors commented Jun 4, 2017

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

@eddyb
Copy link
Member Author

eddyb commented Jun 6, 2017

Started crater run (fingers crossed it's fixed now).
EDIT: it's not.

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

@bors try

1 similar comment
@eddyb
Copy link
Member Author

eddyb commented Jun 6, 2017

@bors try

@bors
Copy link
Contributor

bors commented Jun 6, 2017

⌛ Trying commit 2a64a2d with merge aa4570a...

bors added a commit that referenced this pull request Jun 6, 2017
Don't drag function signatures along function item types.

**TODO**: write description (made PR for crater/cargobomb shenanigans)

r? @nikomatsakis
@tomprince
Copy link
Member

I've started a cargobomb run of this.

@bors
Copy link
Contributor

bors commented Jun 7, 2017

💥 Test timed out

@alexcrichton alexcrichton removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 8, 2017
@bors
Copy link
Contributor

bors commented Jun 9, 2017

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

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2017

📌 Commit 69076f3 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 28, 2017

⌛ Testing commit 69076f3 with merge 4079e61...

bors added a commit that referenced this pull request Jun 28, 2017
Don't drag function signatures along function item types.

This PR separates the signature of a function from the "function item type" (`TyFnDef`), leaving only the `DefId` and parameter `Substs`, making them even more like (captureless) closure types.

The motivation for this change is reducing typesystem complexity, and its consequences:
* operating on the signature instead of just the parameters was less efficient
  * specifically, signatures can easily add several levels of depth on top of the parameter types
  * and the signatured were always substituted and normalized, so typically even more complex
* it was *the only* type that was *both* nominal (identity) and structural (signature)
  * harder to model in Chalk than either a purely nominal or structural type
  * subtyping worked on the signature but parameters were always invariant
  * call type-checking was transforming signatures but keeping the nominal half intact
  * the signature could therefore get out of sync during type inference in several ways

That last point comes with a `[breaking-change]`, because functions with `'static` in their return types will now *not* be as usable as if they were using lifetime parameters instead:
```rust
// Will cause lifetime mismatch in main after this PR.
fn bar() -> &'static str { "bar" }
// Will continue to work fine, as every use can choose its own lifetime.
fn bar<'a>() -> &'a str { "bar" }

fn main() {
    let s = String::from("foo");
    Some(&s[..]).unwrap_or_else(bar);
}
```

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jun 28, 2017

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

@jethrogb
Copy link
Contributor

jethrogb commented Aug 28, 2017

This is stabilizing in 1.20 (Thursday). I realize this is probably a little late, but can we add a better error message when people encounter this breaking change? You now just get a generic borrow check error. Easy to test with the example from the first PR comment.

@eddyb
Copy link
Member Author

eddyb commented Aug 28, 2017

@jethrogb Are you talking about the example itself or have you seen this in the wild in another form? And no, you can't detect this without reintroducing the complex representation removed here.

@jethrogb
Copy link
Contributor

@eddyb I just came here from the draft release notes. I haven't actually encountered any broken code myself. I do think it's fairly common for people to write &'static str as a function return type, so I think this error is likely to come up during development. The error message doesn't imply "you can fix this with a reborrow" which will likely be the case.

I don't exactly understand how this change makes detecting this harder but maybe some kind of heuristic (instead of a guaranteed detection) is good enough. Alternatively, maybe we can make a documentation change somewhere.

@eddyb
Copy link
Member Author

eddyb commented Aug 28, 2017

You only get an error if you pass such a function to another function that usually takes a closure.
Reborrows on immutable references don't usually do anything.

@jethrogb
Copy link
Contributor

I meant this is a way to fix the code above:

    Some(&s[..]).unwrap_or_else(||&*bar());

@eddyb
Copy link
Member Author

eddyb commented Aug 28, 2017

Oh and the canonical fix is to make the 'static into a lifetime parameter.
In your snippet you can remove the reborrow. || bar() is the fix there, the reborrow is redundant.

@jethrogb
Copy link
Contributor

jethrogb commented Aug 28, 2017

Oh and the canonical fix is to make the 'static into a lifetime parameter.

Sure but the user doesn't always have a choice in that regard. Popular crates use &'static str in their return types:
https://docs.rs/toml/0.4/toml/value/enum.Value.html#method.type_str
http://docs.diesel.rs/diesel/query_source/trait.Column.html#tymethod.name
https://docs.rs/slog/2.0.9/slog/struct.Record.html#method.file

@eddyb
Copy link
Member Author

eddyb commented Aug 28, 2017

Fair enough, the second best choice is introducing a closure (like you did, if you ignore the reborrow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.